gh-120868: Fix breaking change in logging.config when using QueueHandler
#120872
Conversation
…ng.Manager was created eagerly
|
All commit authors signed the Contributor License Agreement. |
Lib/logging/config.py
Outdated
| # 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() |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 theTypeError - 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 butMM().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.
There was a problem hiding this comment.
I think, I'll just leave the decision to Vinay because they're the maintainer and creator of the logging module.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Misc/NEWS.d/next/Library/2024-06-22-11-59-13.gh-issue-120868.XHJ12L.rst
Outdated
Show resolved
Hide resolved
| q = self.configure_custom(dict(qspec)) | ||
| else: | ||
|
|
||
| if isinstance(qspec, str): |
There was a problem hiding this comment.
"Flat is better than nested", so I agree that it's reasonable to rearrange the various tests on qspec in this way.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
This reverts commit afcc79e.
|
Thanks for the review @vsajip! I have made the requested changes; please review again :) |
|
Thanks for making the requested changes! @vsajip: please review the changes made to this pull request. |
|
Thanks @provinzkraut for the PR, and @vsajip for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12. |
|
Thanks @provinzkraut for the PR, and @vsajip for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13. |
…QueueHandler` (pythonGH-120872) (cherry picked from commit 7d9c685) Co-authored-by: Janek Nouvertné <provinzkraut@posteo.de>
…QueueHandler` (pythonGH-120872) (cherry picked from commit 7d9c685) Co-authored-by: Janek Nouvertné <provinzkraut@posteo.de>
|
GH-121077 is a backport of this pull request to the 3.12 branch. |
|
GH-121078 is a backport of this pull request to the 3.13 branch. |
#120868 was caused by an eager construction of
multiprocessing.Managerused to check if thequeuepassed in to the logging config was an instance ofmultiprocessing.Manager.Queue/multiprocessing.Manager.JoinableQueue. This could cause unexpected exceptions in environments where amultiprocessing.Managercouldn't be created.The proposed fix moves the check for these classes, and creation of
multiprocessing.Managerwhich is necessary for this, to a point where we've ruled out all other valid options forqueue, and are certain thatmultiprocessing.Managerhas been used to produce the object passed viaqueue.logging.config.DictConfigwithlogging.handlers.QueueHandleron read-only file systems #120868