Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 29 additions & 17 deletions Lib/logging/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Flat is better than nested", so I agree that it's reasonable to rearrange the various tests on qspec in this way.

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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is an error with the creation, you could maybe wrap that one in an try-except block? (probably catching an OSError but I'm not sure).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we want to behave different in this case? I'm not sure what the expected behaviour would be here. The only thing I could think of that would make sense is to raise the usual TypeError from the error the instantiation raised, but that would still result in roughly the same traceback as when calling .configure, since it already re-raises exceptions.

Or are you suggesting we take a different path here when we encounter an error?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I didn't know about the exceptino being wrapped by configure so it may be fine. The thing is, I don't know whether we could be more precise in the error being raised so that you know that's is because multiprocessing queues are not supported.

For instance:

  • you use a bad BaseProxy object which is not a queue and MM().Queue() is fine -> you want the TypeError
  • let's say you have a queue-like object for your readonly system that inherits from MM().Queue() with some tweaks. In this case, you will be able to create your special queue object and expect it to work but MM().Queue() would fail. So instead of having some generic error due to that instantiation, the message could be something else. But I'm not sure whether this corner case is too convoluted or not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, I'll just leave the decision to Vinay because they're the maintainer and creator of the logging module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I'm not sure whether this corner case is too convoluted or not.

Yeah, I was worrying about that. I didn't want to put too much stuff in here just to handle this case. IMO the proper proper fix for this would be to add some level of introspection capabilities in BaseProxy, that allows to perform such checks without creating an instance of the type you want to check against first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would make sense to add something in the docs for this, as there's quite a few special cases now that might not be immediately intuitive

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):
Expand Down
44 changes: 44 additions & 0 deletions Lib/test/test_logging.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions Misc/ACKS
Original file line number Diff line number Diff line change
Expand Up @@ -1318,6 +1318,7 @@ Hrvoje Nikšić
Gregory Nofi
Jesse Noller
Bill Noon
Janek Nouvertné
Stefan Norberg
Tim Northover
Joe Norton
Expand Down
Original file line number Diff line number Diff line change
@@ -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.