-
-
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?
Changes from all commits
d5060f7
fb22cf6
7c3245f
32617a0
5730fd5
fcb3385
9a85aad
4a7e0ee
b6a206f
b128769
f74dcf9
d08f96e
2534bbf
abb3dfe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -427,6 +427,10 @@ def run(taskid, ready, blocker): | |
| ready.get(timeout=1) # blocking | ||
| except interpreters.QueueEmpty: | ||
| pass | ||
| except queues.QueueEmpty: | ||
| # GH-142414: reloading the _queues module makes get to raise | ||
| # queues.QueueEmpty instead of interpreters.QueueEmpty. | ||
| pass | ||
|
Comment on lines
+430
to
+433
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). |
||
| else: | ||
| done += 1 | ||
| pending -= done | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Fix spurious :exc:`KeyError` when :mod:`!concurrent.interpreters._queues` is reloaded after import. |
Uh oh!
There was an error while loading. Please reload this page.