Skip to content

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

Merged
vsajip merged 8 commits into
python: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 into
python:mainfrom
picnixz:fix/once-again-queue-handler-logging-124653

Conversation

@picnixz

@picnixz picnixz commented Oct 2, 2024

Copy link
Copy Markdown
Member

NOw we really have a minmal inteface, namely put_nowait and get. 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 is not the compatible with "queue.Queue" since it lacks some methods and neither is multiprocessing.SimpleQueue would never work since it does not even have the put_nowait method (so even before my PRs, this would just break at runtime due to a missing interface).

So it is correct to raise if the queue interface is multiprocessing.SimpleQueue.


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

@picnixz

picnixz commented Oct 2, 2024

Copy link
Copy Markdown
Member Author

cc @YvesDup @spacemanspiff2007

Comment thread Lib/test/test_logging.py Outdated
@picnixz

picnixz commented Oct 2, 2024

Copy link
Copy Markdown
Member Author

This entire issue makes me wonder whether multiprocessing.SimpleQueue should have a fake put_nowait method or not...

@YvesDup

YvesDup commented Oct 2, 2024

Copy link
Copy Markdown
Contributor

This entire issue makes me wonder whether multiprocessing.SimpleQueue should have a fake put_nowait method or not..

If it is necessary to use a multiprocessing.SimpleQueue in logging.QueueHandler, I suggest inheriting from this class and adding a put_nowait method to be consistant with the minimum queue interface.

Updated: perharps adding this case would be nice ?

@picnixz

picnixz commented Oct 2, 2024

Copy link
Copy Markdown
Member Author

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

@picnixz picnixz added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 2, 2024
@bedevere-bot

Copy link
Copy Markdown

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

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 2, 2024
@picnixz

picnixz commented Oct 3, 2024

Copy link
Copy Markdown
Member Author

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

Comment thread Lib/test/test_logging.py

@vsajip vsajip left a comment

Copy link
Copy Markdown
Member

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!

@picnixz picnixz added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Oct 5, 2024
Comment thread Lib/test/test_logging.py Outdated
@picnixz

picnixz commented Oct 7, 2024

Copy link
Copy Markdown
Member Author

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

@vsajip vsajip merged commit 7ffe94f into python:main Oct 7, 2024
@miss-islington-app

Copy link
Copy Markdown

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 request Oct 7, 2024
…dlers (pythonGH-124897)

(cherry picked from commit 7ffe94f)

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 request Oct 7, 2024
…dlers (pythonGH-124897)

(cherry picked from commit 7ffe94f)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@bedevere-app

bedevere-app Bot commented Oct 7, 2024

Copy link
Copy Markdown

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

@bedevere-app bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label Oct 7, 2024
@bedevere-app

bedevere-app Bot commented Oct 7, 2024

Copy link
Copy Markdown

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

@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only security fixes label Oct 7, 2024
@picnixz picnixz deleted the fix/once-again-queue-handler-logging-124653 branch October 7, 2024 18:43
@picnixz

picnixz commented Oct 7, 2024

Copy link
Copy Markdown
Member Author

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

@picnixz picnixz restored the fix/once-again-queue-handler-logging-124653 branch October 7, 2024 19:01
@picnixz

picnixz commented Oct 7, 2024

Copy link
Copy Markdown
Member Author

Seems the JIT is fine.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants