Skip to content

gh-120868: Fix breaking change in logging.config when using QueueHandler #120872

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
vsajip merged 6 commits into from
Jun 27, 2024
+81 −17
Merged

gh-120868: Fix breaking change in logging.config when using QueueHandler #120872

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0723f49
gh-120868: Fix regression in logging.config where multiprocessing.Man…
provinzkraut Jun 22, 2024
afcc79e
Add News fragment
provinzkraut Jun 22, 2024
08bc4d6
Update Misc/ACKS to make patchcheck happy
provinzkraut Jun 22, 2024
a828c0d
Merge branch 'python:main' into fix-gh-120868
provinzkraut Jun 22, 2024
ccc0060
Revert "Add News fragment"
provinzkraut Jun 23, 2024
6daf85e
Restructure as per review comment
provinzkraut Jun 23, 2024
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
Next Next commit
gh-120868: Fix regression in logging.config where multiprocessing.Man… 
…ager was created eagerly
  • Loading branch information
@provinzkraut
provinzkraut committed Jun 22, 2024
commit 0723f49d7f59625f9f6ab202aa77512ef2ede3c6
46 changes: 29 additions & 17 deletions Lib/logging/config.py
View file
Open in desktop
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
Copy Markdown
Member

@vsajip vsajip Jun 22, 2024

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
Copy Markdown
Member

@picnixz picnixz Jun 22, 2024

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
Copy Markdown
Contributor Author

@provinzkraut provinzkraut Jun 22, 2024

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
Copy Markdown
Member

@picnixz picnixz Jun 22, 2024

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
Copy Markdown
Member

@picnixz picnixz Jun 22, 2024

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
Copy Markdown
Contributor Author

@provinzkraut provinzkraut Jun 22, 2024

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
Copy Markdown
Contributor Author

@provinzkraut provinzkraut Jun 22, 2024

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

Comment thread
provinzkraut marked this conversation as resolved.
Outdated
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
View file
Open in desktop
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):
Comment thread
provinzkraut marked this conversation as resolved.
# 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