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

Fix queues #269

Closed
gvanrossum wants to merge 5 commits into from
Closed

Fix queues #269

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
c8d4ce5
Add failing tests representing #265 and #268.
Sep 26, 2015
4b0ef04
Rewrite Queue.get/put/get_nowait/put_nowait.
Sep 26, 2015
cbfd161
Be more parsimonious in how many waiters we wake up.
Sep 26, 2015
652a3ee
Simplify.
Sep 26, 2015
603b1f8
Add comments to explain what is happening in the except clauses.
Sep 27, 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
152 changes: 41 additions & 111 deletions asyncio/queues.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ def __init__(self, maxsize=0, *, loop=None):

# Futures.
self._getters = collections.deque()
# Futures
# Futures.
self._putters = collections.deque()
self._unfinished_tasks = 0
self._finished = locks.Event(loop=self._loop)
Expand All @@ -67,10 +67,13 @@ def _put(self, item):

# End of the overridable methods.

def __put_internal(self, item):
self._put(item)
self._unfinished_tasks += 1
self._finished.clear()
def _wakeup_next(self, waiters):
# Wake up the next waiter (if any) that isn't cancelled.
while waiters:
waiter = waiters.popleft()
if not waiter.done():
waiter.set_result(None)
break

def __repr__(self):
return '<{} at {:#x} {}>'.format(
Expand All @@ -91,16 +94,6 @@ def _format(self):
result += ' tasks={}'.format(self._unfinished_tasks)
return result

def _consume_done_getters(self):
# Delete waiters at the head of the get() queue who've timed out.
while self._getters and self._getters[0].done():
self._getters.popleft()

def _consume_done_putters(self):
# Delete waiters at the head of the put() queue who've timed out.
while self._putters and self._putters[0].done():
self._putters.popleft()

def qsize(self):
"""Number of items in the queue."""
return len(self._queue)
Expand Down Expand Up @@ -134,47 +127,31 @@ def put(self, item):

This method is a coroutine.
"""
self._consume_done_getters()
if self._getters:
assert not self._queue, (
'queue non-empty, why are getters waiting?')

getter = self._getters.popleft()
self.__put_internal(item)

# getter cannot be cancelled, we just removed done getters
getter.set_result(self._get())

elif self._maxsize > 0 and self._maxsize <= self.qsize():
waiter = futures.Future(loop=self._loop)

self._putters.append(waiter)
yield from waiter
self._put(item)

else:
self.__put_internal(item)
while self.full():
putter = futures.Future(loop=self._loop)
self._putters.append(putter)
try:
yield from putter
except:
putter.cancel() # Just in case putter is not done yet.
if not self.full() and not putter.cancelled():

@gjcarneiro gjcarneiro Sep 27, 2015

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I do not quite understand why the and not putter.cancelled() part. Surely, if the putter was just cancelled in the line above, then not putter.cancelled() must always be true?...

@manipopopo manipopopo Sep 27, 2015

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The _state of an awoken putter is _FINISHED. If the task containing the awoken putter is cancelled, putter.cancelled() will still return False.

I don't understand why putter.cancelled() needs to be checked.

@gvanrossum gvanrossum Sep 27, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm sorry, I should have added a comment, because this is complicated (and we've had bugs around this issue before). The except clause is mainly there to catch CancelledException.

There are two separate scenarios: (1) the putter has not been awoken by a getter, and the putter is cancelled by its caller (perhaps because of a timeout). In this case the putter.cancel() call is a no-op and the following if does nothing. So the whole except clause is a no-op (and the exception is re-raised of course).

But there is also scenario (2) which is more complicated:

  • The putter's future is made ready by a getter (a slot freed up)
  • Before the putter's task can run in the event loop, it is cancelled or times out
  • Once the putter's task runs, it receives a CancelledError

In this case, if there is another putter waiting, we need to awaken it so it can use the slot (because we only wake up the first putter, it hasn't been woken up yet). The putter.cancel() call is still a no-op, but the following if triggers and wakes up the next putter. (And then the exception is re-raised.)

So what is the putter.cancel() call for? It's because I don't trust that I've analyzed all possible scenarios. There might be a (rare) case where we get an exception at this point and the future is not marked done. If that were to happen, the putter would still be in the self._putters deque, and just to make sure that the queue doesn't get stuck I think we should cancel the putter here so it will be ignored when a getter tries to wake up the next putter. In this case we don't need to wake up another putter, and indeed the if clause skips that (because putter is cancelled).

I'll try to think of a brief comment to add to the code.

# We were woken up by get_nowait(), but can't take
# the call. Wake up the next in line.
self._wakeup_next(self._putters)
raise
return self.put_nowait(item)

def put_nowait(self, item):
"""Put an item into the queue without blocking.

If no free slot is immediately available, raise QueueFull.
"""
self._consume_done_getters()
if self._getters:
assert not self._queue, (
'queue non-empty, why are getters waiting?')

getter = self._getters.popleft()
self.__put_internal(item)

# getter cannot be cancelled, we just removed done getters
getter.set_result(self._get())

elif self._maxsize > 0 and self._maxsize <= self.qsize():
if self.full():
raise QueueFull
else:
self.__put_internal(item)
self._put(item)
self._unfinished_tasks += 1
self._finished.clear()
self._wakeup_next(self._getters)

@coroutine
def get(self):
Expand All @@ -184,77 +161,30 @@ def get(self):

This method is a coroutine.
"""
self._consume_done_putters()
if self._putters:
assert self.full(), 'queue not full, why are putters waiting?'
putter = self._putters.popleft()

# When a getter runs and frees up a slot so this putter can
# run, we need to defer the put for a tick to ensure that
# getters and putters alternate perfectly. See
# ChannelTest.test_wait.
self._loop.call_soon(putter._set_result_unless_cancelled, None)

return self._get()

elif self.qsize():
return self._get()
else:
waiter = futures.Future(loop=self._loop)
self._getters.append(waiter)
while self.empty():
getter = futures.Future(loop=self._loop)
self._getters.append(getter)
try:
return (yield from waiter)
except futures.CancelledError:
# if we get CancelledError, it means someone cancelled this
# get() coroutine. But there is a chance that the waiter
# already is ready and contains an item that has just been
# removed from the queue. In this case, we need to put the item
# back into the front of the queue. This get() must either
# succeed without fault or, if it gets cancelled, it must be as
# if it never happened.
if waiter.done():
self._put_it_back(waiter.result())
yield from getter
except:
getter.cancel() # Just in case getter is not done yet.
if not self.empty() and not getter.cancelled():
# We were woken up by put_nowait(), but can't take
# the call. Wake up the next in line.
self._wakeup_next(self._getters)
raise

def _put_it_back(self, item):
"""
This is called when we have a waiter to get() an item and this waiter
gets cancelled. In this case, we put the item back: wake up another
waiter or put it in the _queue.
"""
self._consume_done_getters()
if self._getters:
assert not self._queue, (
'queue non-empty, why are getters waiting?')

getter = self._getters.popleft()
self.__put_internal(item)

# getter cannot be cancelled, we just removed done getters
getter.set_result(item)
else:
self._queue.appendleft(item)
return self.get_nowait()

def get_nowait(self):
"""Remove and return an item from the queue.

Return an item if one is immediately available, else raise QueueEmpty.
"""
self._consume_done_putters()
if self._putters:
assert self.full(), 'queue not full, why are putters waiting?'
putter = self._putters.popleft()
# Wake putter on next tick.

# getter cannot be cancelled, we just removed done putters
putter.set_result(None)

return self._get()

elif self.qsize():
return self._get()
else:
if self.empty():
raise QueueEmpty
item = self._get()
self._wakeup_next(self._putters)
return item

def task_done(self):
"""Indicate that a formerly enqueued task is complete.
Expand Down
55 changes: 48 additions & 7 deletions tests/test_queues.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,29 @@ def test_get_with_waiting_putters(self):
self.assertEqual(self.loop.run_until_complete(q.get()), 'a')
self.assertEqual(self.loop.run_until_complete(q.get()), 'b')

def test_why_are_getters_waiting(self):
# From issue #268.

@asyncio.coroutine
def consumer(queue, num_expected):
for _ in range(num_expected):
yield from queue.get()

@asyncio.coroutine
def producer(queue, num_items):
for i in range(num_items):
yield from queue.put(i)

queue_size = 1
producer_num_items = 5
q = asyncio.Queue(queue_size, loop=self.loop)

self.loop.run_until_complete(
asyncio.gather(producer(q, producer_num_items),
consumer(q, producer_num_items),
loop=self.loop),
)


class QueuePutTests(_QueueTestBase):

Expand Down Expand Up @@ -377,13 +400,8 @@ def gen():

loop.run_until_complete(reader3)

# reader2 will receive `2`, because it was added to the
# queue of pending readers *before* put_nowaits were called.
self.assertEqual(reader2.result(), 2)
# reader3 will receive `1`, because reader1 was cancelled
# before is had a chance to execute, and `2` was already
# pushed to reader2 by second `put_nowait`.
self.assertEqual(reader3.result(), 1)
# It is undefined in which order concurrent readers receive results.
self.assertEqual({reader2.result(), reader3.result()}, {1, 2})

def test_put_cancel_drop(self):

Expand Down Expand Up @@ -479,6 +497,29 @@ def test_put_with_waiting_getters(self):
self.loop.run_until_complete(q.put('a'))
self.assertEqual(self.loop.run_until_complete(t), 'a')

def test_why_are_putters_waiting(self):
# From issue #265.

queue = asyncio.Queue(2, loop=self.loop)

@asyncio.coroutine
def putter(item):
yield from queue.put(item)

@asyncio.coroutine
def getter():
yield
num = queue.qsize()
for _ in range(num):
item = queue.get_nowait()

t0 = putter(0)
t1 = putter(1)
t2 = putter(2)
t3 = putter(3)
self.loop.run_until_complete(
asyncio.gather(getter(), t0, t1, t2, t3, loop=self.loop))


class LifoQueueTests(_QueueTestBase):

Expand Down