gh-120868: Fix breaking change in `logging.config` when using `QueueHandler` by provinzkraut · Pull Request #120872 · python/cpython · GitHub

#120868 was caused by an eager construction of multiprocessing.Manager used to check if the queue passed in to the logging config was an instance of multiprocessing.Manager.Queue / multiprocessing.Manager.JoinableQueue. This could cause unexpected exceptions in environments where a multiprocessing.Manager couldn't be created.

The proposed fix moves the check for these classes, and creation of multiprocessing.Manager which is necessary for this, to a point where we've ruled out all other valid options for queue, and are certain that multiprocessing.Manager has been used to produce the object passed via queue.

Issue: 3.12.4 breakslogging.config.DictConfig with logging.handlers.QueueHandler on read-only file systems #120868

…ng.Manager was created eagerly

All commit authors signed the Contributor License Agreement.

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).

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?

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.

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

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.

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

"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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

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.

🤖 New build scheduled with the buildbot fleet by @vsajip for commit 6daf85e 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

Some buildbot failures, but they appear unrelated to this change.

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>

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.

…`QueueHandler` (GH-120872) (GH-121078) (cherry picked from commit 7d9c685)

…`QueueHandler` (GH-120872) (GH-121077) (cherry picked from commit 7d9c685)

…QueueHandler` (pythonGH-120872)

provinzkraut added 2 commits June 22, 2024 12:22

pythongh-120868 : Fix regression in logging.config where multiprocessi…

0723f49

…ng.Manager was created eagerly

Add News fragment

afcc79e

provinzkraut requested a review from vsajip as a code owner June 22, 2024 10:30

bedevere-app Bot mentioned this pull request Jun 22, 2024

3.12.4 breakslogging.config.DictConfig with logging.handlers.QueueHandler on read-only file systems #120868

Closed

bedevere-app Bot added the awaiting review label Jun 22, 2024

picnixz reviewed Jun 22, 2024

View reviewed changes

provinzkraut and others added 2 commits June 22, 2024 13:17

Update Misc/ACKS to make patchcheck happy

08bc4d6

Merge branch 'python:main' into fix-pythongh-120868

a828c0d

vsajip added the skip news label Jun 22, 2024

vsajip requested changes Jun 22, 2024

View reviewed changes

bedevere-app Bot removed the awaiting review label Jun 22, 2024

bedevere-app Bot added the awaiting changes label Jun 22, 2024

provinzkraut added 2 commits June 23, 2024 10:01

Revert "Add News fragment"

ccc0060

This reverts commit afcc79e.

Restructure as per review comment

6daf85e

bedevere-app Bot added awaiting change review and removed awaiting changes labels Jun 23, 2024

bedevere-app Bot requested a review from vsajip June 23, 2024 08:11

vsajip added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 25, 2024

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 25, 2024

vsajip approved these changes Jun 27, 2024

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting change review labels Jun 27, 2024

vsajip merged commit 7d9c685 into python:main Jun 27, 2024

bedevere-app Bot removed the awaiting merge label Jun 27, 2024

vsajip added the needs backport to 3.12 only security fixes label Jun 27, 2024

vsajip added the needs backport to 3.13 bugs and security fixes label Jun 27, 2024

bedevere-app Bot removed the needs backport to 3.12 only security fixes label Jun 27, 2024

bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label Jun 27, 2024

provinzkraut deleted the fix-gh-120868 branch June 27, 2024 07:35

vsajip pushed a commit that referenced this pull request Jun 27, 2024

[3.13] gh-120868 : Fix breaking change in logging.config when using …

9df7392

…`QueueHandler` (GH-120872) (GH-121078) (cherry picked from commit 7d9c685)

vsajip pushed a commit that referenced this pull request Jun 28, 2024

[3.12] gh-120868 : Fix breaking change in logging.config when using …

b31f7e2

…`QueueHandler` (GH-120872) (GH-121077) (cherry picked from commit 7d9c685)

mrahtz pushed a commit to mrahtz/cpython that referenced this pull request Jun 30, 2024

pythongh-120868 : Fix breaking change in logging.config when using `…

38c568b

…QueueHandler` (pythonGH-120872)

noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024

pythongh-120868 : Fix breaking change in logging.config when using `…

cd60720

…QueueHandler` (pythonGH-120872)

estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024

pythongh-120868 : Fix breaking change in logging.config when using `…

e9dd992

…QueueHandler` (pythonGH-120872)

provinzkraut mentioned this pull request Dec 11, 2024

Bug: Litestar crashes on initialization with Python 3.12.4 and a read-only root filesystem litestar-org/litestar#3591

Closed

4 tasks

+                                          # 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()

-                                          q = self.configure_custom(dict(qspec))
-                                      else:
+                                  if isinstance(qspec, str):

gh-120868: Fix breaking change in `logging.config` when using `QueueHandler` by provinzkraut · Pull Request #120872 · python/cpython · GitHub

Uh oh!

Uh oh!

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

gh-120868: Fix breaking change in logging.config when using QueueHandler#120872vsajip merged 6 commits intopython:mainpython/cpython:mainfrom provinzkraut:fix-gh-120868provinzkraut/cpython:fix-gh-120868Copy head branch name to clipboard

Conversation

provinzkraut commented Jun 22, 2024 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 22, 2024 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

picnixz Jun 22, 2024

Choose a reason for hiding this comment

Uh oh!

provinzkraut Jun 22, 2024

Choose a reason for hiding this comment

Uh oh!

picnixz Jun 22, 2024

Choose a reason for hiding this comment

Uh oh!

picnixz Jun 22, 2024

Choose a reason for hiding this comment

Uh oh!

provinzkraut Jun 22, 2024

Choose a reason for hiding this comment

Uh oh!

provinzkraut Jun 22, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vsajip Jun 22, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-app Bot commented Jun 22, 2024

Uh oh!

provinzkraut commented Jun 23, 2024

Uh oh!

bedevere-app Bot commented Jun 23, 2024

Uh oh!

bedevere-bot commented Jun 25, 2024

Uh oh!

vsajip left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington-app Bot commented Jun 27, 2024

Uh oh!

miss-islington-app Bot commented Jun 27, 2024

Uh oh!

bedevere-app Bot commented Jun 27, 2024

Uh oh!

bedevere-app Bot commented Jun 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gh-120868: Fix breaking change in logging.config when using QueueHandler#120872
vsajip merged 6 commits into
python:mainfrom
provinzkraut:fix-gh-120868

provinzkraut commented Jun 22, 2024 •
edited by bedevere-app Bot
Loading

ghost commented Jun 22, 2024 •
edited by ghost
Loading