From 0723f49d7f59625f9f6ab202aa77512ef2ede3c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Janek=20Nouvertn=C3=A9?= <25355197+provinzkraut@users.noreply.github.com> Date: Sat, 22 Jun 2024 11:48:26 +0200 Subject: [PATCH 1/5] gh-120868: Fix regression in logging.config where multiprocessing.Manager was created eagerly --- Lib/logging/config.py | 46 +++++++++++++++++++++++++--------------- Lib/test/test_logging.py | 44 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 17 deletions(-) diff --git a/Lib/logging/config.py b/Lib/logging/config.py index d2f23e53f35c57..a864b2f8175dc1 100644 --- a/Lib/logging/config.py +++ b/Lib/logging/config.py @@ -780,25 +780,37 @@ def configure_handler(self, config): # if 'handlers' not in config: # raise ValueError('No handlers specified for a QueueHandler') if 'queue' in config: - from multiprocessing.queues import Queue as MPQueue - from multiprocessing import Manager as MM - proxy_queue = MM().Queue() - proxy_joinable_queue = MM().JoinableQueue() qspec = config['queue'] - if not isinstance(qspec, (queue.Queue, MPQueue, - type(proxy_queue), type(proxy_joinable_queue))): - if isinstance(qspec, str): - q = self.resolve(qspec) - if not callable(q): - raise TypeError('Invalid queue specifier %r' % qspec) - q = q() - elif isinstance(qspec, dict): - if '()' not in qspec: - raise TypeError('Invalid queue specifier %r' % qspec) - q = self.configure_custom(dict(qspec)) - else: + + if isinstance(qspec, str): + q = self.resolve(qspec) + if not callable(q): raise TypeError('Invalid queue specifier %r' % qspec) - config['queue'] = q + config['queue'] = q() + elif isinstance(qspec, dict): + if '()' not in qspec: + raise TypeError('Invalid queue specifier %r' % qspec) + config['queue'] = self.configure_custom(dict(qspec)) + else: + from multiprocessing.queues import Queue as MPQueue + + if not isinstance(qspec, (queue.Queue, MPQueue)): + # Safely check if 'qspec' is an instance of Manager.Queue + # / Manager.JoinableQueue + + from multiprocessing import Manager as MM + from multiprocessing.managers import BaseProxy + + # if it's not an instance of BaseProxy, it also can't be + # an instance of Manager.Queue / Manager.JoinableQueue + if isinstance(qspec, BaseProxy): + proxy_queue = MM().Queue() + proxy_joinable_queue = MM().JoinableQueue() + if not isinstance(qspec, (type(proxy_queue), type(proxy_joinable_queue))): + raise TypeError('Invalid queue specifier %r' % qspec) + else: + raise TypeError('Invalid queue specifier %r' % qspec) + if 'listener' in config: lspec = config['listener'] if isinstance(lspec, type): diff --git a/Lib/test/test_logging.py b/Lib/test/test_logging.py index 5192ce252a4d4c..242c8474c24152 100644 --- a/Lib/test/test_logging.py +++ b/Lib/test/test_logging.py @@ -3928,6 +3928,50 @@ def test_config_queue_handler(self): msg = str(ctx.exception) self.assertEqual(msg, "Unable to configure handler 'ah'") + @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 + + from multiprocessing import Queue as MQ + + q1 = {"()": "queue.Queue", "maxsize": -1} + q2 = MQ() + q3 = queue.Queue() + + for qspec in (q1, q2, q3): + 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 + + with self.assertRaises(ValueError): + self.apply_config( + { + "version": 1, + "handlers": { + "queue_listener": { + "class": "logging.handlers.QueueHandler", + "queue": object(), + }, + }, + } + ) + manager.assert_not_called() + @support.requires_subprocess() def test_multiprocessing_queues(self): # See gh-119819 From afcc79ed4ca34517757a74e16035532c287dbe27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Janek=20Nouvertn=C3=A9?= <25355197+provinzkraut@users.noreply.github.com> Date: Sat, 22 Jun 2024 12:22:24 +0200 Subject: [PATCH 2/5] Add News fragment --- .../Library/2024-06-22-11-59-13.gh-issue-120868.XHJ12L.rst | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2024-06-22-11-59-13.gh-issue-120868.XHJ12L.rst diff --git a/Misc/NEWS.d/next/Library/2024-06-22-11-59-13.gh-issue-120868.XHJ12L.rst b/Misc/NEWS.d/next/Library/2024-06-22-11-59-13.gh-issue-120868.XHJ12L.rst new file mode 100644 index 00000000000000..7c51d26f7f7f69 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-06-22-11-59-13.gh-issue-120868.XHJ12L.rst @@ -0,0 +1,6 @@ +Fix a bug in :mod:`logging.config` that would eagerly create an instance of +:class:`multiprocessing.Manager` when configuring a queue_listener with +:class:`logging.handlers.QueueHandler`. This could cause issue in environments where +``multiprocessing.Manager`` does not work. The new implementation will only check for +these types when it has been verified that ``multiprocessing.Manager`` has been used to +create the ``queue`` in the logging configuration in question. From 08bc4d6d039e3e8758d8ddb666561a9ca88ae608 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Janek=20Nouvertn=C3=A9?= <25355197+provinzkraut@users.noreply.github.com> Date: Sat, 22 Jun 2024 13:17:00 +0200 Subject: [PATCH 3/5] Update Misc/ACKS to make patchcheck happy --- Misc/ACKS | 1 + 1 file changed, 1 insertion(+) diff --git a/Misc/ACKS b/Misc/ACKS index a406fca8744a5f..53258dbfd9f478 100644 --- a/Misc/ACKS +++ b/Misc/ACKS @@ -1318,6 +1318,7 @@ Hrvoje Nikšić Gregory Nofi Jesse Noller Bill Noon +Janek Nouvertné Stefan Norberg Tim Northover Joe Norton From ccc0060dfb6d5479d1c78f6f6d911734aae8621e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Janek=20Nouvertn=C3=A9?= <25355197+provinzkraut@users.noreply.github.com> Date: Sun, 23 Jun 2024 10:01:40 +0200 Subject: [PATCH 4/5] Revert "Add News fragment" This reverts commit afcc79ed4ca34517757a74e16035532c287dbe27. --- .../Library/2024-06-22-11-59-13.gh-issue-120868.XHJ12L.rst | 6 ------ 1 file changed, 6 deletions(-) delete mode 100644 Misc/NEWS.d/next/Library/2024-06-22-11-59-13.gh-issue-120868.XHJ12L.rst diff --git a/Misc/NEWS.d/next/Library/2024-06-22-11-59-13.gh-issue-120868.XHJ12L.rst b/Misc/NEWS.d/next/Library/2024-06-22-11-59-13.gh-issue-120868.XHJ12L.rst deleted file mode 100644 index 7c51d26f7f7f69..00000000000000 --- a/Misc/NEWS.d/next/Library/2024-06-22-11-59-13.gh-issue-120868.XHJ12L.rst +++ /dev/null @@ -1,6 +0,0 @@ -Fix a bug in :mod:`logging.config` that would eagerly create an instance of -:class:`multiprocessing.Manager` when configuring a queue_listener with -:class:`logging.handlers.QueueHandler`. This could cause issue in environments where -``multiprocessing.Manager`` does not work. The new implementation will only check for -these types when it has been verified that ``multiprocessing.Manager`` has been used to -create the ``queue`` in the logging configuration in question. From 6daf85e18ed2844654bebdb08be881ed1f55314d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Janek=20Nouvertn=C3=A9?= <25355197+provinzkraut@users.noreply.github.com> Date: Sun, 23 Jun 2024 10:06:02 +0200 Subject: [PATCH 5/5] Restructure as per review comment --- Lib/logging/config.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/Lib/logging/config.py b/Lib/logging/config.py index a864b2f8175dc1..95e129ae988c24 100644 --- a/Lib/logging/config.py +++ b/Lib/logging/config.py @@ -804,8 +804,15 @@ def configure_handler(self, config): # if it's not an instance of BaseProxy, it also can't be # an instance of Manager.Queue / Manager.JoinableQueue if isinstance(qspec, BaseProxy): - proxy_queue = MM().Queue() - proxy_joinable_queue = MM().JoinableQueue() + # Sometimes manager or queue creation might fail + # (e.g. see issue gh-120868). In that case, any + # exception during the creation of these queues will + # propagate up to the caller and be wrapped in a + # `ValueError`, whose cause will indicate the details of + # the failure. + mm = MM() + proxy_queue = mm.Queue() + proxy_joinable_queue = mm.JoinableQueue() if not isinstance(qspec, (type(proxy_queue), type(proxy_joinable_queue))): raise TypeError('Invalid queue specifier %r' % qspec) else: