locks: Fix inconsistency cancelling Condition.wait. #346
Conversation
Issue #22970: Cancelling wait() after notification leaves Condition in an inconsistent state. This can occur if the waiting task has been notified but has not reacquired the associated lock. Instead of cancelling waiting for the notification, the cancel call will cancel the lock reacquisition, returning control to the calling task without the lock being held (a violation of Condition.wait contract).
|
|
||
| try: | ||
| self.loop.run_until_complete(wait_task) | ||
| except asyncio.CancelledError: |
There was a problem hiding this comment.
Originally I had a self.assertRaises(asyncio.CancelledError, ...) here, but since we successfully notified this task before cancellation no exception should be raises. However if the fix is not applied then a CancelledError will be raised so we have to be prepared to handle it so we can check the lock state afterwards.
|
Not a problem. Thanks taking the time in reviewing and looking into the issue. In the future when you merge a contribution, could you please preserve the authorship information by either directly merging this branch, cherry picking or using --author flag when committing. Besides preserving contribution information, my employer requires the use of my work email address when contributing to open source projects. |
Next time please indicate that as early as possible. Your patch was merged in the same way how 99% patches usually get merged to CPython and asyncio. |
|
Hm, for the asyncio repo here don't we usually use the "Merge pull request" button on GitHub? That would have done what David asked for IIUC. The PR is still open so apparently it didn't happen that way. |
|
I usually merge by hand -- fetching the repo, rebasing the commit to avoid merges, and then pushing it (authorship is preserved that way). This time I simply downloaded this pr as a patch and committed it to cpython and asyncio attributing David in the commit message... I can revert my commit and push again FWIW. Sent from my iPhone
|
|
GitHub has a relatively new "squash merge" feature in the UI that avoids
the annoying merges. I rarely merge by hand any more (only when there's
something I want to clean up and the author is not responsive).
|
|
I think I've enabled it for asyncio repo. Ok, I'll try that button next time :) Sent from my iPhone
|
|
Since you asked me to create a PR here on github, I was under the assumption that that's the way it would have been merged. Since my employer claims copyright on pretty much any code I write (but are fine with contributions to open source software) they'd like to see contributions from my [email protected] address. There's also at least one other prominent 'David Coles' contributing to open source. (Plus it gives me a bit of a warm fuzzy feeling to see git/github tell me I've contributed to Python/Asyncio 😄 ). I think the reason for the missing authorship is, as you said, just directly applying the patch (to avoid the merge commit). No harm done. I'm mostly happy to see that this will be fixed in 3.5.2! |
|
Sorry again, David, and thanks a lot for your contribution. I think that once we migrate CPython repo to github, it will be an established practice to merge commits preserving the authorship. |
3 participants
Issue #22970: Cancelling wait() after notification leaves Condition in an
inconsistent state.
This can occur if the waiting task has been notified but has not reacquired the
associated lock. Instead of cancelling waiting for the notification, the cancel
call will cancel the lock reacquisition, returning control to the calling task
without the lock being held (a violation of Condition.wait contract).