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

Rewrite Semaphore.acquire/release #271

Closed
manipopopo wants to merge 5 commits into from
Closed

Rewrite Semaphore.acquire/release #271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2b255ad
Rewrite Semaphore.acquire/release
manipopopo Sep 27, 2015
dd99680
Allow Semaphore.acquire() returns immediately when Semaphore._value i…
manipopopo Sep 27, 2015
538dcf8
Fix a bug: pending tasks can be awoken by cancel() called outside the…
manipopopo Sep 28, 2015
1d71a9f
Make the behavior of asyncio.Semaphore follow the semantic of the se…
Sep 28, 2015
5f0cdba
Make Semaphore more resistant to unexpected error and restore the bla…
Sep 29, 2015
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
37 changes: 21 additions & 16 deletions asyncio/locks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,13 @@ def __repr__(self):
extra = '{},waiters:{}'.format(extra, len(self._waiters))
return '<{} [{}]>'.format(res[1:-1], extra)

def _wake_up_next(self):
while self._waiters:
waiter = self._waiters.popleft()
if not waiter.done():
waiter.set_result(None)
return

def locked(self):
"""Returns True if semaphore can not be acquired immediately."""
return self._value == 0
Expand All @@ -425,29 +432,27 @@ def acquire(self):
called release() to make it larger than 0, and then return
True.
"""
if not self._waiters and self._value > 0:
self._value -= 1
return True

fut = futures.Future(loop=self._loop)
self._waiters.append(fut)
try:
yield from fut
self._value -= 1
return True
finally:
self._waiters.remove(fut)
while self._value <= 0:
fut = futures.Future(loop=self._loop)
self._waiters.append(fut)
try:
yield from fut
except:

@gvanrossum gvanrossum Sep 29, 2015

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.

I've changed my mind on this (again). The except clause should just catch CancelledException and the fut.cancel() is unnecesary.

@gvanrossum gvanrossum Sep 29, 2015

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.

Also, can you add a comment saying something like "See similar code in Queue"?

@manipopopo manipopopo Sep 29, 2015

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.

When I was testing asyncio.Queue, there was only one time that an exception other than CancelledException was catch: when a pending task waiting for a future is deleted, GeneratorExit is raised.

Do we need to add assert fut.done() here?

@gvanrossum gvanrossum Sep 29, 2015

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.

Oh wow. Then please leave it as is. The GeneratorExit is worth a comment! This pretty much confirms the hunch I had about other exceptions possibly happening here...

# See the similar code in Queue.get.
fut.cancel()
if self._value > 0 and not fut.cancelled():
self._wake_up_next()
raise
self._value -= 1
return True

def release(self):
"""Release a semaphore, incrementing the internal counter by one.
When it was zero on entry and another coroutine is waiting for it to
become larger than zero again, wake up that coroutine.
"""
self._value += 1
for waiter in self._waiters:
if not waiter.done():
waiter.set_result(True)
break
self._wake_up_next()


class BoundedSemaphore(Semaphore):
Expand Down
53 changes: 43 additions & 10 deletions tests/test_locks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import asyncio
from asyncio import test_utils


STR_RGX_REPR = (
r'^<(?P<class>.*?) object at (?P<address>.*?)'
r'\[(?P<extras>'
Expand Down Expand Up @@ -673,7 +672,6 @@ def test_ambiguous_loops(self):


class SemaphoreTests(test_utils.TestCase):

def setUp(self):
self.loop = self.new_test_loop()

Expand Down Expand Up @@ -783,22 +781,20 @@ def c4(result):

test_utils.run_briefly(self.loop)
self.assertEqual(0, sem._value)
self.assertEqual([1, 2, 3], result)
self.assertEqual(3, len(result))

@gvanrossum gvanrossum Sep 29, 2015

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.

Can you change this to something like assertEqual({1, 2, 3}, set(result))?

@manipopopo manipopopo Sep 29, 2015

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.

But only 1 will definitely be in the list. The list may contains {1, 2, 3}, {1, 2, 4} or {1, 3, 4}.

@manipopopo manipopopo Sep 29, 2015

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.

@gvanrossum Do we need to add assertTrue(1 in result) or remove assertEqual(3, len(result)) here?

@gvanrossum gvanrossum Sep 29, 2015

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.

Oh, I see. Then I think what you have is fine. (Boy this is a complex test. :-)

self.assertTrue(sem.locked())
self.assertEqual(1, len(sem._waiters))
self.assertEqual(0, sem._value)

self.assertTrue(t1.done())
self.assertTrue(t1.result())
self.assertTrue(t2.done())
self.assertTrue(t2.result())
self.assertTrue(t3.done())
self.assertTrue(t3.result())
self.assertFalse(t4.done())
race_tasks = [t2, t3, t4]
done_tasks = [t for t in race_tasks if t.done() and t.result()]
self.assertTrue(2, len(done_tasks))

# cleanup locked semaphore
sem.release()
self.loop.run_until_complete(t4)
self.loop.run_until_complete(asyncio.gather(*race_tasks))

def test_acquire_cancel(self):
sem = asyncio.Semaphore(loop=self.loop)
Expand All @@ -809,7 +805,44 @@ def test_acquire_cancel(self):
self.assertRaises(
asyncio.CancelledError,
self.loop.run_until_complete, acquire)
self.assertFalse(sem._waiters)
self.assertTrue((not sem._waiters) or
all(waiter.done() for waiter in sem._waiters))

def test_acquire_cancel_before_awoken(self):
sem = asyncio.Semaphore(value=0, loop=self.loop)

t1 = asyncio.Task(sem.acquire(), loop=self.loop)
t2 = asyncio.Task(sem.acquire(), loop=self.loop)
t3 = asyncio.Task(sem.acquire(), loop=self.loop)
t4 = asyncio.Task(sem.acquire(), loop=self.loop)

test_utils.run_briefly(self.loop)

sem.release()
t1.cancel()
t2.cancel()

test_utils.run_briefly(self.loop)
num_done = sum(t.done() for t in [t3, t4])
self.assertEqual(num_done, 1)

t3.cancel()
t4.cancel()
test_utils.run_briefly(self.loop)

def test_acquire_hang(self):
sem = asyncio.Semaphore(value=0, loop=self.loop)

t1 = asyncio.Task(sem.acquire(), loop=self.loop)
t2 = asyncio.Task(sem.acquire(), loop=self.loop)

test_utils.run_briefly(self.loop)

sem.release()
t1.cancel()

test_utils.run_briefly(self.loop)
self.assertTrue(sem.locked())

def test_release_not_acquired(self):
sem = asyncio.BoundedSemaphore(loop=self.loop)
Expand Down