bpo-37658: Actually return result in race condition #29202
Conversation
| @@ -1101,24 +1096,6 @@ async def inner(): | |||
| with self.assertRaises(FooException): | |||
| loop.run_until_complete(foo()) | |||
|
|
|||
| def test_wait_for_raises_timeout_error_if_returned_during_cancellation(self): | |||
There was a problem hiding this comment.
This test literally tests that we don't return a value when it has completed successfully...
I can't see any reason for this test, the issue linked to the PR does not mention anything that warrants this test existing: https://bugs.python.org/issue40607
So, as far as I can tell, this test should be binned and the behaviour should be as defined in the above changes.
| yield 0.1 | ||
| yield 0.1 | ||
| async def inner(): | ||
| with contextlib.suppress(asyncio.CancelledError): |
There was a problem hiding this comment.
Initially I just emulated the race condition in the bug report, but because it's a race condition, it seems that the test occasionally failed due to actually hitting the timeout.
Instead, I've just suppressed the cancellation, which makes the behaviour 100% reproducible.
There was a problem hiding this comment.
This is not an equivalent test. As I understand, the whole point of that test was to inject the cancellation at the exact point to cause the race on the return to ‘await waiter’ once fut is done. The new test is not checking this race at all but instead checking that a cancellation ignoring task can skip the timeout.
There was a problem hiding this comment.
It's not an equivalent test, because the previous test is completely wrong. It does not emulate a race condition, and it does not reproduce the issue in the bug report.
I wrote a test which actually reproduces the race condition as given in the bug report, and the issue was not fixed. As mentioned above, I had to use this suppress cancellation trick to make the test reproducible (it will still run things in the same order as the race condition), the original test worked correctly everytime I ran it, but failed in one of the CI runs, and we can't have flaky tests.
There was a problem hiding this comment.
You can see the old version which was timing dependent in the commit history:
169294e
|
This PR is stale because it has been open for 30 days with no activity. |
|
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 couldn't run the I have made the requested changes; please review again |
|
Thanks for making the requested changes! @asvetlov: please review the changes made to this pull request. |
|
Thanks! |
|
Thanks @Dreamsorcerer for the PR, and @asvetlov for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10. |
|
GH-29832 is a backport of this pull request to the 3.9 branch. |
(cherry picked from commit 934a826) Co-authored-by: Sam Bull <aa6bs0@sambull.org>
(cherry picked from commit 934a826) Co-authored-by: Sam Bull <aa6bs0@sambull.org>
|
GH-29831 is a backport of this pull request to the 3.10 branch. |
Splitting this out from #28149 to ease review.
The original test written for this issue is bogus and fails to reproduce the bug report. I've replaced it with a test that reproduces the error, and have actually fixed the problem reported.
https://bugs.python.org/issue37658