gh-121723: Relax constraints on queue objects for `logging.handlers.QueueHandler`. by picnixz · Pull Request #122154 · python/cpython

picnixz · 2024-07-23T09:19:03Z

Not sure if I need to explain what the Queue public API is. I think people are smart enough to understand that we expect a Queue-like object but tell me if you want me to put more details here.

Issue: multiprocessing.Pool in spawn mode breaks logging.handlers.QueueHandler configuration #121723

📚 Documentation preview 📚: https://cpython-previews--122154.org.readthedocs.build/

Any object implementing the Queue public API can be used for configuring a QueueHandler. Only the presence of the methods is checked, but not their signature.

vsajip

Looks good, but I have two thoughts:

Should the actual test for a queue protocol be factored out into an is_queue function? I know it'll only be used once, but it might make the overall flow slightly more readable.
Is it worth adding a test for the specific failure condition of #121723 (multiprocessing.set_start_method('spawn'))?

picnixz · 2024-07-24T07:13:46Z

Yes but I wasn't sure you wanted this in the module. AFAICT, the logging library likes to have long functions :)
This one requires a debug build (otherwise the assert is not executed) but I'll see if I can test it in debug only (I think the behaviour will become buggy in release mode).

bedevere-bot · 2024-07-27T10:48:15Z

🤖 New build scheduled with the buildbot fleet by @vsajip for commit 7729fb6 🤖

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

picnixz

@vsajip Before merging, I'll need to fix some typos so don't enable the automerge (I'm waiting for the build bots to finish).

Lib/logging/config.py

Lib/test/test_logging.py

picnixz · 2024-07-27T18:53:51Z

I'm waiting for the build bots to finish

Good thing I didn't wait in front of my screen, thinking it would finish FAST! I'll update my branch tomorrow.

bedevere-bot · 2024-07-31T08:33:25Z

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

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

picnixz · 2024-07-31T14:32:07Z

Ok, so https://buildbot.python.org/#/builders/1295/builds/224 is a legit failure (it's just that queue is empty, but I suspect it's either an issue with the FT build or the timeout is not just enough).
The other on AMD64 Arch Linux Usan 3.x/4532 is something I found on other PRs as well.

vsajip · 2024-08-01T07:50:40Z

or the timeout is not just enough

Do you want to try increasing the timeout?

picnixz · 2024-08-01T07:54:28Z

Do you want to try increasing the timeout?

For the sake of the test, I'll try but I'm not sure whether it's really a timeout issue or not. I'll put it at 60s (if it fails, then it's probably something else but it's probably something related to multiprocessing then).

picnixz · 2024-08-01T07:55:38Z

Can you run a single build bot actually? namely AMD64 Windows Server 2022 NoGIL PR/224? it appears that there are also other issues with multiprocessing for it but I'm not sure if they are related...

vsajip · 2024-08-02T08:57:52Z

I've asked that bot to rerun. I assume it'll pick up your latest changes - https://buildbot.python.org/#/builders/1295/builds/226

picnixz · 2024-08-02T09:28:16Z

It appears that the only failure is not related to this PR:

FAIL: test_search_major_2 (test.test_launcher.TestLauncher.test_search_major_2)
FAIL: test_search_major_3 (test.test_launcher.TestLauncher.test_search_major_3)
Re-running test_launcher in verbose mode (matching: test_search_major_2, test_search_major_3)
FAIL: test_search_major_2 (test.test_launcher.TestLauncher.test_search_major_2)
FAIL: test_search_major_3 (test.test_launcher.TestLauncher.test_search_major_3)

miss-islington-app · 2024-08-02T11:16:35Z

Thanks @picnixz for the PR, and @vsajip for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

…lers.QueueHandler`. (pythonGH-122154) (cherry picked from commit fb864c7) Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

bedevere-app · 2024-08-02T11:16:45Z

GH-122603 is a backport of this pull request to the 3.13 branch.

…lers.QueueHandler`. (pythonGH-122154) (cherry picked from commit fb864c7) Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

bedevere-app · 2024-08-02T11:16:50Z

GH-122604 is a backport of this pull request to the 3.12 branch.

…dlers.QueueHandler`. (GH-122154) (GH-122604) (cherry picked from commit fb864c7)

…dlers.QueueHandler`. (GH-122154) (GH-122603) (cherry picked from commit fb864c7)

…lers.QueueHandler`. (pythonGH-122154)

picnixz added 4 commits July 23, 2024 11:13

Relax constraints on queue objects for queue handlers.

6ab2257

Any object implementing the Queue public API can be used for configuring a QueueHandler. Only the presence of the methods is checked, but not their signature.

add tests

9af323b

update docs

78936a1

blurb

5fc3d5a

picnixz added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Jul 23, 2024

picnixz requested a review from vsajip as a code owner July 23, 2024 09:19

bedevere-app bot mentioned this pull request Jul 23, 2024

multiprocessing.Pool in spawn mode breaks logging.handlers.QueueHandler configuration #121723

Closed

bedevere-app bot added the awaiting review label Jul 23, 2024

vsajip reviewed Jul 23, 2024

View reviewed changes

picnixz added 5 commits July 24, 2024 11:19

refactor into a helper

04c7813

add regression test

e5cdb1c

fix start methods selection on MS-Windows

b770787

fixup refs

65a8987

increase the timeout (maybe it works like this?)

7729fb6

picnixz requested a review from vsajip July 27, 2024 10:20

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

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

picnixz commented Jul 27, 2024

View reviewed changes

Lib/logging/config.py Outdated Show resolved Hide resolved

Lib/test/test_logging.py Outdated Show resolved Hide resolved

Lib/test/test_logging.py Outdated Show resolved Hide resolved

picnixz added 5 commits July 28, 2024 10:44

fixup! typos

57d7190

fixup! typos

e4800c4

reduce line numbers

bd12893

renaming

70d69e5

fixup! refs

d6b6ef3

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

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

increase timeout (again)

d2bfdc2

vsajip approved these changes Aug 2, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting review labels Aug 2, 2024

vsajip merged commit fb864c7 into python:main Aug 2, 2024

bedevere-app bot removed the awaiting merge label Aug 2, 2024

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Aug 2, 2024

bedevere-app bot removed the needs backport to 3.12 only security fixes label Aug 2, 2024

picnixz deleted the queue-protocol-in-logging branch August 2, 2024 11:19

vsajip pushed a commit that referenced this pull request Aug 2, 2024

[3.12] gh-121723: Relax constraints on queue objects for `logging.han…

c47943a

…dlers.QueueHandler`. (GH-122154) (GH-122604) (cherry picked from commit fb864c7)

vsajip pushed a commit that referenced this pull request Aug 2, 2024

[3.13] gh-121723: Relax constraints on queue objects for `logging.han…

56435a8

…dlers.QueueHandler`. (GH-122154) (GH-122603) (cherry picked from commit fb864c7)

brandtbucher pushed a commit to brandtbucher/cpython that referenced this pull request Aug 7, 2024

pythongh-121723: Relax constraints on queue objects for `logging.hand…

e242fe7

…lers.QueueHandler`. (pythonGH-122154)

blhsing pushed a commit to blhsing/cpython that referenced this pull request Aug 22, 2024

pythongh-121723: Relax constraints on queue objects for `logging.hand…

abfd969

…lers.QueueHandler`. (pythonGH-122154)

YvesDup mentioned this pull request Oct 2, 2024

Python 3.12+ breaks backwards compatibility for logging QueueHandler with queue.SimpleQueue #124653

Closed

Uh oh!

Conversation

picnixz commented Jul 23, 2024 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsajip left a comment

Choose a reason for hiding this comment

Uh oh!

picnixz commented Jul 24, 2024

Uh oh!

bedevere-bot commented Jul 27, 2024

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

picnixz commented Jul 27, 2024

Uh oh!

bedevere-bot commented Jul 31, 2024

Uh oh!

picnixz commented Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsajip commented Aug 1, 2024

Uh oh!

picnixz commented Aug 1, 2024

Uh oh!

picnixz commented Aug 1, 2024

Uh oh!

vsajip commented Aug 2, 2024

Uh oh!

picnixz commented Aug 2, 2024

Uh oh!

miss-islington-app bot commented Aug 2, 2024

Uh oh!

bedevere-app bot commented Aug 2, 2024

Uh oh!

bedevere-app bot commented Aug 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

picnixz commented Jul 23, 2024 •

edited by github-actions bot

Loading

picnixz commented Jul 31, 2024 •

edited

Loading