-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
gh-142414: Remove singleton design pattern in Lib/concurrent/interpreters/_crossinterp.py. #142577
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…est_interpreter_pool"
…THgP-.rst Co-authored-by: Peter Bierma <[email protected]>
We don't need to make singleton picklabe based on the current tests.
2b1d0f1 to
b128769
Compare
c7cf990 to
f74dcf9
Compare
Misc/NEWS.d/next/Library/2025-12-10-15-27-58.gh-issue-142414.zTHgP-.rst
Outdated
Show resolved
Hide resolved
…THgP-.rst Co-authored-by: Victor Stinner <[email protected]>
Misc/NEWS.d/next/Library/2025-12-10-15-27-58.gh-issue-142414.zTHgP-.rst
Outdated
Show resolved
Hide resolved
…THgP-.rst Co-authored-by: Victor Stinner <[email protected]>
| except queues.QueueEmpty: | ||
| # GH-142414: reloading the _queues module makes get to raise | ||
| # queues.QueueEmpty instead of interpreters.QueueEmpty. | ||
| pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like for this to be in a separate PR with a better test case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix is needed otherwise ./python Lib/test/test_runpy.py will run forever.
If a better test case is needed, we can introduce it in the same PR.
I haven’t yet figured out how the combination test causes this in a single test, I’ll need some more time to figure out that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't seem related to this PR. Can you reproduce the same infinite loop on main?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the main branch has no issue because there’s no unbound reload (unbound is directly declared in queues) before this fix. The loop happens along with this fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think twice.
The existing test suite should already provide sufficient coverage. Since test_interpreter_pool naturally fails with deadlock when the issue exists. So we don't need an additional dedicated test if the test modifications in the PR is considered a legitimate bug fix (not workarounds).
./python -m test test_interpreters test_concurrent_futures.test_interpreter_pool
1. Before this fix: test_interpreters failed
2. After this fix: test_concurrent_futures.test_interpreter_pool failed without this catch
3. After this fix with the catch: all passed
Test with
./python -m test test_interpreters test_struct./python -m test test_interpreters test_concurrent_futures.test_interpreter_pool./python -m test test_interpreters test_structfails with: KeyError: concurrent.interpreters._queues.UNBOUND #142414