From d93b5f490b3ec64be906419520228610d2434e36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Wed, 2 Oct 2024 15:12:58 +0200 Subject: [PATCH 1/8] Relax constraints on the Queue API checks for logging. --- Lib/logging/config.py | 14 +++++++------- Lib/test/test_logging.py | 23 +++++++++++++++++------ 2 files changed, 24 insertions(+), 13 deletions(-) diff --git a/Lib/logging/config.py b/Lib/logging/config.py index 3781cb1aeb9ae2..6a6a7f726f7e0c 100644 --- a/Lib/logging/config.py +++ b/Lib/logging/config.py @@ -499,7 +499,7 @@ def as_tuple(self, value): def _is_queue_like_object(obj): """Check that *obj* implements the Queue API.""" - if isinstance(obj, queue.Queue): + if isinstance(obj, (queue.Queue, queue.SimpleQueue)): return True # defer importing multiprocessing as much as possible from multiprocessing.queues import Queue as MPQueue @@ -516,13 +516,13 @@ def _is_queue_like_object(obj): # Ideally, we would have wanted to simply use strict type checking # instead of a protocol-based type checking since the latter does # not check the method signatures. - queue_interface = [ - 'empty', 'full', 'get', 'get_nowait', - 'put', 'put_nowait', 'join', 'qsize', - 'task_done', - ] + # + # Note that only 'put_nowait' and 'get' are required by the logging + # queue handler and queue listener (see gh-124653) and that other + # methods are either optional or unused. + minimal_queue_interface = ['put_nowait', 'get'] return all(callable(getattr(obj, method, None)) - for method in queue_interface) + for method in minimal_queue_interface) class DictConfigurator(BaseConfigurator): """ diff --git a/Lib/test/test_logging.py b/Lib/test/test_logging.py index 230ba954cd286d..5a908af01922de 100644 --- a/Lib/test/test_logging.py +++ b/Lib/test/test_logging.py @@ -2377,16 +2377,22 @@ def __getattr__(self, attribute): return getattr(queue, attribute) class CustomQueueFakeProtocol(CustomQueueProtocol): - # An object implementing the Queue API (incorrect signatures). + # An object implementing the minimial Queue API for + # the logging module but with incorrect signatures. + # # The object will be considered a valid queue class since we # do not check the signatures (only callability of methods) # but will NOT be usable in production since a TypeError will - # be raised due to a missing argument. - def empty(self, x): + # be raised due to the extra argument in 'put_nowait'. + def put_nowait(self): pass class CustomQueueWrongProtocol(CustomQueueProtocol): - empty = None + put_nowait = None + +class MinimalQueueProtocol: + def put_nowait(self, x): pass + def get(self): pass def queueMaker(): return queue.Queue() @@ -3963,6 +3969,7 @@ def test_config_queue_handler_does_not_create_multiprocessing_manager(self, mana # between usability and the work we need to do in order to safely # check that the queue object correctly implements the API. q4 = CustomQueueFakeProtocol() + q5 = MinimalQueueProtocol() for qspec in (q1, q2, q3, q4): self.apply_config( @@ -3980,9 +3987,13 @@ def test_config_queue_handler_does_not_create_multiprocessing_manager(self, mana @patch("multiprocessing.Manager") def test_config_queue_handler_invalid_config_does_not_create_multiprocessing_manager(self, manager): - # gh-120868, gh-121723 + # gh-120868, gh-121723, gh-124653 + + from multiprocessing import SimpleQueue as MPSimpleQueue + # MPSimpleQueue does not have the 'put_nowait' method and thus + # cannot be used as a queue-like object here. - for qspec in [object(), CustomQueueWrongProtocol()]: + for qspec in [object(), CustomQueueWrongProtocol(), MPSimpleQueue()]: with self.assertRaises(ValueError): self.apply_config( { From 43872bc21d60a66a924d6cf516ce27ead683016d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Wed, 2 Oct 2024 15:13:02 +0200 Subject: [PATCH 2/8] update docs --- Doc/library/logging.config.rst | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/Doc/library/logging.config.rst b/Doc/library/logging.config.rst index 317ca8728248c8..0e9dc33ae2123a 100644 --- a/Doc/library/logging.config.rst +++ b/Doc/library/logging.config.rst @@ -753,16 +753,17 @@ The ``queue`` and ``listener`` keys are optional. If the ``queue`` key is present, the corresponding value can be one of the following: -* An object implementing the :class:`queue.Queue` public API. For instance, - this may be an actual instance of :class:`queue.Queue` or a subclass thereof, - or a proxy obtained by :meth:`multiprocessing.managers.SyncManager.Queue`. +* An object implementing the :meth:`Queue.put_nowait ` + and :meth:`Queue.get ` public API. For instance, this may be + an actual instance of :class:`queue.Queue` or a subclass thereof, or a proxy + obtained by :meth:`multiprocessing.managers.SyncManager.Queue`. This is of course only possible if you are constructing or modifying the configuration dictionary in code. * A string that resolves to a callable which, when called with no arguments, returns - the :class:`queue.Queue` instance to use. That callable could be a - :class:`queue.Queue` subclass or a function which returns a suitable queue instance, + the queue instance to use. That callable could be a :class:`queue.Queue` subclass + or a function which returns a suitable queue instance, such as ``my.module.queue_factory()``. * A dict with a ``'()'`` key which is constructed in the usual way as discussed in From 52a456ab5eb6bb929432c67b202c45ca8b2820f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Wed, 2 Oct 2024 15:13:04 +0200 Subject: [PATCH 3/8] blurb --- .../next/Library/2024-10-02-15-05-45.gh-issue-124653.tqsTu9.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2024-10-02-15-05-45.gh-issue-124653.tqsTu9.rst diff --git a/Misc/NEWS.d/next/Library/2024-10-02-15-05-45.gh-issue-124653.tqsTu9.rst b/Misc/NEWS.d/next/Library/2024-10-02-15-05-45.gh-issue-124653.tqsTu9.rst new file mode 100644 index 00000000000000..6f5ad12d2c2981 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-10-02-15-05-45.gh-issue-124653.tqsTu9.rst @@ -0,0 +1,2 @@ +Fix detection of the minimal Queue API needed by the :mod:`logging` module. +Patch by Bénédikt Tran. From fe6c25d27298f1d7ac93830d8db9ace8a42c1712 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Wed, 2 Oct 2024 16:03:52 +0200 Subject: [PATCH 4/8] fix test --- Lib/test/test_logging.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_logging.py b/Lib/test/test_logging.py index 5a908af01922de..96bafb0cb61f5f 100644 --- a/Lib/test/test_logging.py +++ b/Lib/test/test_logging.py @@ -3971,7 +3971,7 @@ def test_config_queue_handler_does_not_create_multiprocessing_manager(self, mana q4 = CustomQueueFakeProtocol() q5 = MinimalQueueProtocol() - for qspec in (q1, q2, q3, q4): + for qspec in (q1, q2, q3, q4, q5): self.apply_config( { "version": 1, From 985d3c38f74dda297aaa347c537fc553b87b840e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Wed, 2 Oct 2024 16:11:45 +0200 Subject: [PATCH 5/8] fortify tests --- Lib/test/test_logging.py | 87 ++++++++++++++++++++++------------------ 1 file changed, 49 insertions(+), 38 deletions(-) diff --git a/Lib/test/test_logging.py b/Lib/test/test_logging.py index 96bafb0cb61f5f..a7e3592697eafc 100644 --- a/Lib/test/test_logging.py +++ b/Lib/test/test_logging.py @@ -3956,45 +3956,28 @@ def test_config_queue_handler(self): @support.requires_subprocess() @patch("multiprocessing.Manager") def test_config_queue_handler_does_not_create_multiprocessing_manager(self, manager): - # gh-120868, gh-121723 - - from multiprocessing import Queue as MQ - - q1 = {"()": "queue.Queue", "maxsize": -1} - q2 = MQ() - q3 = queue.Queue() - # CustomQueueFakeProtocol passes the checks but will not be usable - # since the signatures are incompatible. Checking the Queue API - # without testing the type of the actual queue is a trade-off - # between usability and the work we need to do in order to safely - # check that the queue object correctly implements the API. - q4 = CustomQueueFakeProtocol() - q5 = MinimalQueueProtocol() - - for qspec in (q1, q2, q3, q4, q5): - self.apply_config( - { - "version": 1, - "handlers": { - "queue_listener": { - "class": "logging.handlers.QueueHandler", - "queue": qspec, - }, - }, - } - ) - manager.assert_not_called() - - @patch("multiprocessing.Manager") - def test_config_queue_handler_invalid_config_does_not_create_multiprocessing_manager(self, manager): # gh-120868, gh-121723, gh-124653 - from multiprocessing import SimpleQueue as MPSimpleQueue - # MPSimpleQueue does not have the 'put_nowait' method and thus - # cannot be used as a queue-like object here. - - for qspec in [object(), CustomQueueWrongProtocol(), MPSimpleQueue()]: - with self.assertRaises(ValueError): + import multiprocessing + + for qspec in [ + {"()": "queue.Queue", "maxsize": -1}, + queue.Queue(), + # queue.SimpleQueue does not inherit from queue.Queue + queue.SimpleQueue(), + # CustomQueueFakeProtocol passes the checks but will not be usable + # since the signatures are incompatible. Checking the Queue API + # without testing the type of the actual queue is a trade-off + # between usability and the work we need to do in order to safely + # check that the queue object correctly implements the API. + CustomQueueFakeProtocol(), + MinimalQueueProtocol(), + # multiprocessing.Queue() is a valid queue for the logging module + # multiprocessing.SimpleQueue() is NOT valid because it lacks the + # 'put_nowait' method which is needed by the logging queue handler. + multiprocessing.Queue(), + ]: + with self.subTest(qspec=qspec): self.apply_config( { "version": 1, @@ -4006,7 +3989,35 @@ def test_config_queue_handler_invalid_config_does_not_create_multiprocessing_man }, } ) - manager.assert_not_called() + manager.assert_not_called() + + @patch("multiprocessing.Manager") + def test_config_queue_handler_invalid_config_does_not_create_multiprocessing_manager(self, manager): + # gh-120868, gh-121723, gh-124653 + + import multiprocessing + + for qspec in [ + object(), + CustomQueueWrongProtocol(), + # multiprocessing.SimpleQueue does not implement the 'put_nowait' + # method and thus cannot be used as a queue-like object here. + multiprocessing.SimpleQueue(), + ]: + with self.subTest(qspec=qspec): + with self.assertRaises(ValueError): + self.apply_config( + { + "version": 1, + "handlers": { + "queue_listener": { + "class": "logging.handlers.QueueHandler", + "queue": qspec, + }, + }, + } + ) + manager.assert_not_called() @skip_if_tsan_fork @support.requires_subprocess() From 87379b1915009826a4b97cf5eb890a72b8a40b88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Wed, 2 Oct 2024 16:13:00 +0200 Subject: [PATCH 6/8] update comments --- Lib/test/test_logging.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_logging.py b/Lib/test/test_logging.py index a7e3592697eafc..f8e0f59ba562b1 100644 --- a/Lib/test/test_logging.py +++ b/Lib/test/test_logging.py @@ -3973,8 +3973,8 @@ def test_config_queue_handler_does_not_create_multiprocessing_manager(self, mana CustomQueueFakeProtocol(), MinimalQueueProtocol(), # multiprocessing.Queue() is a valid queue for the logging module - # multiprocessing.SimpleQueue() is NOT valid because it lacks the - # 'put_nowait' method which is needed by the logging queue handler. + # but multiprocessing.SimpleQueue() is NOT valid because it lacks + # the 'put_nowait' method needed by the logging queue handler. multiprocessing.Queue(), ]: with self.subTest(qspec=qspec): From c1d1f16ddcdc391f0ffc7bdafcf1454e41816956 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Wed, 2 Oct 2024 17:37:14 +0200 Subject: [PATCH 7/8] fix WASI tests --- Lib/test/test_logging.py | 72 +++++++++++++++++++--------------------- 1 file changed, 35 insertions(+), 37 deletions(-) diff --git a/Lib/test/test_logging.py b/Lib/test/test_logging.py index f8e0f59ba562b1..1b96dc54d97563 100644 --- a/Lib/test/test_logging.py +++ b/Lib/test/test_logging.py @@ -3952,14 +3952,23 @@ def test_config_queue_handler(self): msg = str(ctx.exception) self.assertEqual(msg, "Unable to configure handler 'ah'") + def _apply_simple_queue_listener_configuration(self, qspec): + self.apply_config({ + "version": 1, + "handlers": { + "queue_listener": { + "class": "logging.handlers.QueueHandler", + "queue": qspec, + }, + }, + }) + @threading_helper.requires_working_threading() @support.requires_subprocess() @patch("multiprocessing.Manager") def test_config_queue_handler_does_not_create_multiprocessing_manager(self, manager): # gh-120868, gh-121723, gh-124653 - import multiprocessing - for qspec in [ {"()": "queue.Queue", "maxsize": -1}, queue.Queue(), @@ -3972,52 +3981,41 @@ def test_config_queue_handler_does_not_create_multiprocessing_manager(self, mana # check that the queue object correctly implements the API. CustomQueueFakeProtocol(), MinimalQueueProtocol(), - # multiprocessing.Queue() is a valid queue for the logging module - # but multiprocessing.SimpleQueue() is NOT valid because it lacks - # the 'put_nowait' method needed by the logging queue handler. - multiprocessing.Queue(), ]: with self.subTest(qspec=qspec): - self.apply_config( - { - "version": 1, - "handlers": { - "queue_listener": { - "class": "logging.handlers.QueueHandler", - "queue": qspec, - }, - }, - } - ) + self._apply_simple_queue_listener_configuration(qspec) manager.assert_not_called() @patch("multiprocessing.Manager") def test_config_queue_handler_invalid_config_does_not_create_multiprocessing_manager(self, manager): # gh-120868, gh-121723, gh-124653 + for qspec in [object(), CustomQueueWrongProtocol()]: + with self.subTest(qspec=qspec), self.assertRaises(ValueError): + self._apply_simple_queue_listener_configuration(qspec) + manager.assert_not_called() + + @skip_if_tsan_fork + @support.requires_subprocess() + @unittest.skipUnless(support.Py_DEBUG, "requires a debug build for testing" + " assertions in multiprocessing") + def test_config_reject_simple_queue_handler_multiprocessing_context(self): + # multiprocessing.SimpleQueue does not implement 'put_nowait' + # and thus cannot be used as a queue-like object (gh-124653) + import multiprocessing - for qspec in [ - object(), - CustomQueueWrongProtocol(), - # multiprocessing.SimpleQueue does not implement the 'put_nowait' - # method and thus cannot be used as a queue-like object here. - multiprocessing.SimpleQueue(), - ]: - with self.subTest(qspec=qspec): + if support.MS_WINDOWS: + start_methods = ['spawn'] + else: + start_methods = ['spawn', 'fork', 'forkserver'] + + for start_method in start_methods: + with self.subTest(start_method=start_method): + ctx = multiprocessing.get_context(start_method) + qspec = ctx.SimpleQueue() with self.assertRaises(ValueError): - self.apply_config( - { - "version": 1, - "handlers": { - "queue_listener": { - "class": "logging.handlers.QueueHandler", - "queue": qspec, - }, - }, - } - ) - manager.assert_not_called() + self._apply_simple_queue_listener_configuration(qspec) @skip_if_tsan_fork @support.requires_subprocess() From e2323ee5bf258342dfce92e0fad96bbde86f5131 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?B=C3=A9n=C3=A9dikt=20Tran?= <10796600+picnixz@users.noreply.github.com> Date: Sat, 5 Oct 2024 02:10:45 +0200 Subject: [PATCH 8/8] Update Lib/test/test_logging.py --- Lib/test/test_logging.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_logging.py b/Lib/test/test_logging.py index 1b96dc54d97563..d4ceb7c8dc0b41 100644 --- a/Lib/test/test_logging.py +++ b/Lib/test/test_logging.py @@ -3988,7 +3988,7 @@ def test_config_queue_handler_does_not_create_multiprocessing_manager(self, mana @patch("multiprocessing.Manager") def test_config_queue_handler_invalid_config_does_not_create_multiprocessing_manager(self, manager): - # gh-120868, gh-121723, gh-124653 + # gh-120868, gh-121723 for qspec in [object(), CustomQueueWrongProtocol()]: with self.subTest(qspec=qspec), self.assertRaises(ValueError):