GeezerVison is showing github.com — gh-161712: Read the clock once in ThreadHandle_join() by aozmekik · Pull Request #161794 · python/cpython
Conversation
aozmekik
commented
Nov 11, 2026
•
edited by bedevere-app
Bot
Loading
Uh oh!
There was an error while loading. Please reload this page.
main already reads the clock once in ThreadHandle_join() since gh-157495, but that was part of a cleanup and wasn't backported, so 3.15 and 3.14 still evaluate _PyDeadline_Get() twice inside Py_MAX(). If the deadline passes between the two reads, the negative second value becomes the timeout, which means no timeout, and Thread.join(timeout) blocks forever. This makes the single read explicit, without Py_MAX(), and says why, so it stays that way and can be backported, as suggested in #161771.
The window is too small for a test, so I forced it under gdb: a signal wakes the join loop of join(3.0) after a second, and the second clock read in that loop reports the deadline as passed. On 3.15 and 3.14 as they are, the join was still blocked after 30 seconds; with this change it returns after 3 seconds. test_threading and test_thread pass.
cc @ZeroIntensity
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
main already reads the clock once since pythongh-157495, but only as part of
a cleanup. Make it explicit, with a comment, so that the double read
doesn't come back, and so that the fix can be backported: on 3.15 and
3.14, Py_MAX() still evaluates _PyDeadline_Get() twice, and a negative
second value means no timeout, so Thread.join(timeout) can block
forever.
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 change. _PyDeadline_Get is called once per loop now. Why this change?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.
Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Whether you do it with Py_MAX or with an explicit if, in both cases you can have a race. And as you said, you needed to do some gdb hack to test that case.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Right, on main it's already a single read since gh-157495, so this doesn't fix anything there. The bug is only on 3.15 and 3.14, where it's Py_MAX(_PyDeadline_Get(deadline), 0) and the macro calls _PyDeadline_Get() twice: the comparison can see a positive value and the second call return a negative one, which then becomes the timeout. I first opened the backport directly (#161771) and @ZeroIntensity asked for a main PR first, for consistency and in case GH-157591 gets reverted, so here it's only about making the single read explicit so it can be backported.
About the race: with one read into a local, the value that's checked is the value that's used, so the timeout can't end up negative. The deadline can still pass right after the read, but then it only waits that short positive time and the next loop gets 0. The gdb part was only to hit the window on 3.15/3.14. If you'd rather not touch main, I'm fine keeping just the comment here, or closing this and going back to the backports.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Ideally I just want the same code as in main but backported (so storing the value first and using Py_MAX afterwards). WDYT @ZeroIntensity?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Yeah, that's exactly what I wanted. I didn't realize that main already had the same code as in #161771; I thought it just wasn't an issue because we made Py_MAX only evaluate the arguments once. I'll reopen the other PR and land that. Sorry about the noise!
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|