bpo-44645: Check for interrupts on any potentially backwards edge. #27167
Conversation
Lib/test/test_threading.py
Outdated
| def test_can_interrupt_tight_loops(self): | ||
| #Nothing to assert here. It just shouldn't hang. | ||
|
|
||
| cont = True | ||
| def worker(): | ||
| while cont: | ||
| pass | ||
|
|
||
| t = threading.Thread(target=worker) | ||
| t.start() | ||
| time.sleep(0.1) | ||
| cont = False | ||
| t.join() |
There was a problem hiding this comment.
Please, add a timeout to the join so if it hangs it doesn't hang the full test suite and decorate the function with @threading_helper.reap_threads. Also, ideally we don't use a sleep as a syncronization promitive because this is going to easily become a race. You can try to orchestrate the waiting with athreading.Event
There was a problem hiding this comment.
I don't think the timeout will help in this case. If no other threads can run, then we can never reach the join call.
There was a problem hiding this comment.
Then add a maximum number of iterations or something. The objective here is that the test suite cannot fully hang if the test fails.
|
When you're done making the requested changes, leave the comment: And if you don't make the requested changes, you will be poked with soft cushions! |
| @threading_helper.reap_threads | ||
| def test_can_interrupt_tight_loops(self): | ||
| cont = True | ||
| started = False | ||
| iterations = 100_000_000 | ||
|
|
||
| def worker(): | ||
| nonlocal iterations | ||
| nonlocal started | ||
| started = True | ||
| while cont: | ||
| if iterations: | ||
| iterations -= 1 | ||
| else: | ||
| return | ||
| pass | ||
|
|
||
| t = threading.Thread(target=worker) | ||
| t.start() | ||
| while not started: | ||
| pass | ||
| cont = False | ||
| t.join() | ||
| self.assertNotEqual(iterations, 0) |
There was a problem hiding this comment.
| @threading_helper.reap_threads | |
| def test_can_interrupt_tight_loops(self): | |
| cont = True | |
| started = False | |
| iterations = 100_000_000 | |
| def worker(): | |
| nonlocal iterations | |
| nonlocal started | |
| started = True | |
| while cont: | |
| if iterations: | |
| iterations -= 1 | |
| else: | |
| return | |
| pass | |
| t = threading.Thread(target=worker) | |
| t.start() | |
| while not started: | |
| pass | |
| cont = False | |
| t.join() | |
| self.assertNotEqual(iterations, 0) | |
| @threading_helper.reap_threads | |
| def test_can_interrupt_tight_loops(self): | |
| cont = True | |
| started = threading.Event() | |
| iterations = 100_000_000 | |
| def worker(): | |
| nonlocal iterations | |
| started.set() | |
| while cont: | |
| if iterations: | |
| iterations -= 1 | |
| else: | |
| return | |
| pass | |
| t = threading.Thread(target=worker) | |
| t.start() | |
| started.wait() | |
| cont = False | |
| t.join() | |
| self.assertNotEqual(iterations, 0) |
There was a problem hiding this comment.
We are testing whether tight loops can be interrupted. Adding all the thread event machinery could undermine that.
I'd rather keep the test as close to the original bug report as possible. A little bit of active waiting shouldn't be a problem.
There was a problem hiding this comment.
Ok, I don't want to hold this more but note that the event is outside the loop, so it should not interfere
|
Thanks @markshannon for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10. |
|
Sorry @markshannon, I had trouble checking out the |
…edge. (pythonGH-27167)" This reverts commit 000e70a.
…edge. (pythonGH-27167)" (pythonGH-27194) This reverts commit 000e70a. (cherry picked from commit c90c591) Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>
https://bugs.python.org/issue44645