Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-124653: relax (again) detection of queue API for logging handlers#124897

Merged
vsajip merged 8 commits intopython:mainfrom
picnixz:fix/once-again-queue-handler-logging-124653
Oct 7, 2024
Merged

gh-124653: relax (again) detection of queue API for logging handlers#124897
vsajip merged 8 commits intopython:mainfrom
picnixz:fix/once-again-queue-handler-logging-124653

Conversation

@picnixz
Copy link
Member

@picnixzpicnixz commentedOct 2, 2024
edited by github-actionsbot
Loading

NOw we really have a minmal inteface, namelyput_nowait andget. More than that is not needed for the default handlers and listeners.

I think, we had a misunderstanding between what the docs told (namely a queue API) and what we expected at runtime.SimpleQueue isnot the compatible with "queue.Queue" since it lacks some methods and neither ismultiprocessing.SimpleQueue would never work since it does not even have theput_nowait method (so even before my PRs, this would just break at runtime due to a missing interface).

So itis correct to raise if the queue interface ismultiprocessing.SimpleQueue.


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

@picnixz
Copy link
MemberAuthor

cc@YvesDup@spacemanspiff2007

YvesDup reacted with heart emoji

@picnixz
Copy link
MemberAuthor

This entire issue makes me wonder whethermultiprocessing.SimpleQueue should have a fakeput_nowait method or not...

@YvesDup
Copy link
Contributor

YvesDup commentedOct 2, 2024
edited
Loading

This entire issue makes me wonder whethermultiprocessing.SimpleQueue should have a fakeput_nowait method or not..

If it is necessary to use amultiprocessing.SimpleQueue inlogging.QueueHandler, I suggest inheriting from this class and adding aput_nowait method to be consistant with the minimum queue interface.

Updated: perharps adding this case would be nice ?

@picnixz
Copy link
MemberAuthor

Updated: perharps adding this case would be nice ?

I didn't add it because I think it's a bit straightforward. If we add such method in the future to the base class, the test will detect that the class is no longer deemed invalid and if we do not, then the case is already covered by the minimal interface (I think this should be enough).

@picnixzpicnixz added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelOct 2, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@picnixz for commitc1d1f16 🤖

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

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelOct 2, 2024
@picnixz
Copy link
MemberAuthor

So we only have a single build bot failure which we already know the reason for. Ithink this issue will not haunt me again :D (just waiting for@vsajip's green light).

Copy link
Member

@vsajipvsajip left a comment

Choose a reason for hiding this comment

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

Thank you for this PR!

@picnixzpicnixz added needs backport to 3.12only security fixes needs backport to 3.13bugs and security fixes labelsOct 5, 2024
@picnixz
Copy link
MemberAuthor

@vsajip friendly ping in case you forgot to merge after approving the PR :')

@vsajipvsajip merged commit7ffe94f intopython:mainOct 7, 2024
@miss-islington-app
Copy link

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestOct 7, 2024
…dlers (pythonGH-124897)(cherry picked from commit7ffe94f)Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestOct 7, 2024
…dlers (pythonGH-124897)(cherry picked from commit7ffe94f)Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@bedevere-app
Copy link

GH-125059 is a backport of this pull request to the3.13 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.13bugs and security fixes labelOct 7, 2024
@bedevere-app
Copy link

GH-125060 is a backport of this pull request to the3.12 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.12only security fixes labelOct 7, 2024
@picnixzpicnixz deleted the fix/once-again-queue-handler-logging-124653 branchOctober 7, 2024 18:43
@picnixz
Copy link
MemberAuthor

Huh... I think I forgot to update the JIT ignored files... I hope the test won't make the JIT fail.

@picnixzpicnixz restored the fix/once-again-queue-handler-logging-124653 branchOctober 7, 2024 19:01
@picnixz
Copy link
MemberAuthor

Seems the JIT is fine.

@picnixzpicnixz deleted the fix/once-again-queue-handler-logging-124653 branchOctober 7, 2024 21:52
vsajip pushed a commit that referenced this pull requestOct 8, 2024
vsajip pushed a commit that referenced this pull requestOct 8, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@vsajipvsajipvsajip approved these changes

+2 more reviewers

@zuozuozuo left review comments

@YvesDupYvesDupYvesDup left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@picnixz@YvesDup@bedevere-bot@vsajip@zuo

Comments


[8]ページ先頭

©2009-2026 Movatter.jp