-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
gh-116631: Fix race condition in test_shutdown_immediate_put_join
#116670
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
Conversation
…oin` The test case had a race condition: if `q.task_done()` was executed after `shutdown(immediate=True)`, then it would raise an exception because the immediate shutdown already emptied the queue. This happened rarely with the GIL (due to the switching interval), but frequently in the free-threaded build.
@EpicWink - I wasn't able to add you as a reviewer, but if you have time, I'd appreciate your feedback on the PR as well. |
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 don't understand this test well enough to approve the change -- we ought to wait for @EpicWink, since it's his code, but sometimes it takes a week to get his input (he's just a volunteer). If this race condition is causing you grief, go ahead and merge it. I have one naming nit.
Co-authored-by: Guido van Rossum <[email protected]>
@YvesDup wrote the tests, with minimal fixing by me before the PR was merged. I've found adding delays in the main thread usually surfaces the race conditions. I'll test when I'm on PC |
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.
Tests succeed even with random delays spotted around
result = q.get() | ||
self.assertEqual(result, "Y") | ||
q.task_done() | ||
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
Thanks @EpicWink for the review. @colesbury Go ahead and merge! (No need to credit me for that variable rename. :-)
@EpicWink, thanks for reviewing and testing this! |
…oin` (python#116670) The test case had a race condition: if `q.task_done()` was executed after `shutdown(immediate=True)`, then it would raise an exception because the immediate shutdown already emptied the queue. This happened rarely with the GIL (due to the switching interval), but frequently in the free-threaded build.
…oin` (python#116670) The test case had a race condition: if `q.task_done()` was executed after `shutdown(immediate=True)`, then it would raise an exception because the immediate shutdown already emptied the queue. This happened rarely with the GIL (due to the switching interval), but frequently in the free-threaded build.
…oin` (python#116670) The test case had a race condition: if `q.task_done()` was executed after `shutdown(immediate=True)`, then it would raise an exception because the immediate shutdown already emptied the queue. This happened rarely with the GIL (due to the switching interval), but frequently in the free-threaded build.
The test case had a race condition: if
q.task_done()
was executed aftershutdown(immediate=True)
, then it would raise an exception because the shutdown call already emptied the queue. This happened rarely with the GIL (due to the switching interval), but frequently in the free-threaded build with the GIL disabled.test_queue.test_shutdown_immediate_put_join
#116631