Repository navigation
Conversation
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.
picnixz
left a comment
picnixz
left a comment
There was a problem hiding this comment.
I don't understand this change. _PyDeadline_Get is called once per loop now. Why this change?
|
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 |
|
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. |
|
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 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. |
|
Ideally I just want the same code as in main but backported (so storing the value first and using Py_MAX afterwards). WDYT @ZeroIntensity? |
|
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 |
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 insidePy_MAX(). If the deadline passes between the two reads, the negative second value becomes the timeout, which means no timeout, andThread.join(timeout)blocks forever. This makes the single read explicit, withoutPy_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