Skip to content
This repository was archived by the owner on Nov 23, 2017. It is now read-only.

locks: Fix inconsistency cancelling Condition.wait. #346

Closed
dcoles wants to merge 1 commit into from
Closed

locks: Fix inconsistency cancelling Condition.wait. #346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion asyncio/locks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,13 @@ def wait(self):
self._waiters.remove(fut)

finally:
yield from self.acquire()
# Must reacquire lock even if wait is cancelled
while True:
try:
yield from self.acquire()
break
except futures.CancelledError:
pass

@coroutine
def wait_for(self, predicate):
Expand Down
25 changes: 25 additions & 0 deletions tests/test_locks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,31 @@ def test_wait_cancel(self):
self.assertFalse(cond._waiters)
self.assertTrue(cond.locked())

def test_wait_cancel_contested(self):
cond = asyncio.Condition(loop=self.loop)

self.loop.run_until_complete(cond.acquire())
self.assertTrue(cond.locked())

wait_task = asyncio.Task(cond.wait(), loop=self.loop)
test_utils.run_briefly(self.loop)
self.assertFalse(cond.locked())

# Notify, but contest the lock before cancelling
self.loop.run_until_complete(cond.acquire())
self.assertTrue(cond.locked())
cond.notify()
self.loop.call_soon(wait_task.cancel)
self.loop.call_soon(cond.release)

try:
self.loop.run_until_complete(wait_task)
except asyncio.CancelledError:

@dcoles dcoles May 16, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# Should not happen, since no cancellation points
pass

@1st1 1st1 Jun 11, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why do we have the try..except here (if it shouldn't happen)? Maybe instead of pass you want self.fail('explanation')?

@dcoles dcoles Jun 11, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason this was just a pass rather than a hard-fail is so that we reach the assertTrue(cond.locked()) to ensure that this patch resolves the issue (failing before the patch is applied and then not afterwards). With a fail in the exception handler, we wouldn't be showing the existence of issue 22970.

@1st1 1st1 Jun 11, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should that try-except be replaced with self.assertRaises?

Sent from my iPhone

On Jun 11, 2016, at 1:56 PM, David Coles notifications@github.com wrote:

In tests/test_locks.py:

  •    wait_task = asyncio.Task(cond.wait(), loop=self.loop)

  •    test_utils.run_briefly(self.loop)

  •    self.assertFalse(cond.locked())

  •    # Notify, but contest the lock before cancelling

  •    self.loop.run_until_complete(cond.acquire())

  •    self.assertTrue(cond.locked())

  •    cond.notify()

  •    self.loop.call_soon(wait_task.cancel)

  •    self.loop.call_soon(cond.release)

  •    try:

  •        self.loop.run_until_complete(wait_task)

  •    except asyncio.CancelledError:

  •        # Should not happen, since no cancellation points

  •        pass
    The reason this was just a pass rather than a hard-fail is so that we reach the assertTrue(cond.locked()) to ensure that this patch resolves the issue (failing before the patch is applied and then not afterwards). With a fail in the exception handler, we wouldn't be showing the existence of issue 22970.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

@dcoles dcoles Jun 11, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately you then get the opposite problem where no exception is raised once bug is fixed. Stuck between these two unfortunate alternatives, I decided that using pass was cleanest.


self.assertTrue(cond.locked())

def test_wait_unacquired(self):
cond = asyncio.Condition(loop=self.loop)
self.assertRaises(
Expand Down