Queues

Queues

[A. Jesse Jiryu Davis, 2013-03-18 18:35:49]

For Issue 7: http://code.google.com/p/tulip/issues/detail?id=7

From my initial proposal:

Tulip already has Lock, EventWaiter, Condition, and Semaphore. I propose it
should have Queue, JoinableQueue, LifoQueue, and PriorityQueue. I offer to
implement them.

This patch is an initial implementation and a reasonably thorough test suite,
ported from my package for Tornado, called "Toro"
(https://github.com/ajdavis/toro).

[Nikolay Kim, 2013-03-18 18:58:43]

On 2013/03/18 18:35:49, A. Jesse Jiryu Davis wrote:
> For Issue 7: http://code.google.com/p/tulip/issues/detail?id=7
> 
> From my initial proposal:
> 
> Tulip already has Lock, EventWaiter, Condition, and Semaphore. I propose it
> should have Queue, JoinableQueue, LifoQueue, and PriorityQueue. I offer to
> implement them.
> 
> This patch is an initial implementation and a reasonably thorough test suite,
> ported from my package for Tornado, called "Toro"
> (https://github.com/ajdavis/toro).

Great! i think we need queues.

here is my thoughts:

 1. Should we reuse locks?
 2. Queue.put and Queue.get should be coroutines.
 3. "block" parameter should be removed from put and get methods.
    create separate methods for non-blocking calls.
 4. i dont think you need next_tick and _Waiter 
    in case if for put() and get() are coroutines.

[GvR, 2013-03-18 20:10:22]

This is not a full review, just a bunch of notes on style and a few questions
about possibly unnecessary functionality.  I agree with all Nikolay's points.

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py
File tulip/queues.py (right):

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode15
tulip/queues.py:15: def next_tick(event_loop, value):
Doesn't this need a leading underscore?

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode23
tulip/queues.py:23: """Internal queue utility class"""
Need a blank line below this.

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode25
tulip/queues.py:25: """Create a future with a timeout. If timeout is not None,
it is an int
I like to enforce the docstring style where, if there's more than one line, the
format is

"""Summary sentence that fits on one line.
<blank line>
One or more paragraphs of text.
"""

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode35
tulip/queues.py:35: self._event_loop.call_later(
As an optimization, consider saving the handle(r) that this returns and calling
cancel() on it when the result is set before the timeout is reached.

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode53
tulip/queues.py:53: """Create a queue object with a given maximum size.
Don't use a verb to describe a class.

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode55
tulip/queues.py:55: If `maxsize` is ``None`` (the default) the queue size is
unbounded.
Please, no markup in docstrings.

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode63
tulip/queues.py:63: interrupted between calling :meth:`qsize` and doing an
operation on the
Ah, this is cool. :-)

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode68
tulip/queues.py:68: - `initial`: Optional sequence of initial items.
How often is initial needed?  The standard queue doesn't have this; I'm curious
what the use case would be that makes it necessary to support this.

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode75
tulip/queues.py:75: raise ValueError("maxsize can't be negative")
I recommend single quotes for all strings except docstrings.

Maybe add repr(maxsize) to the error message -- the actual value may help the
programmer understand the bug better.

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode127
tulip/queues.py:127: def get_maxsize(self):
Maybe make this an internal method (_get_maxsize) so people won't call it
instead of referencing q.maxsize ?

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode159
tulip/queues.py:159: def set_maxsize(self, maxsize):
What's the use case for adjusting maxsize?

[A. Jesse Jiryu Davis, 2013-03-18 21:53:40]

On 2013/03/18 20:10:22, GvR wrote:
> This is not a full review, just a bunch of notes on style and a few questions
> about possibly unnecessary functionality.  I agree with all Nikolay's points.
> 
> https://codereview.appspot.com/7751044/diff/1/tulip/queues.py
> File tulip/queues.py (right):
> 
> https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode15
> tulip/queues.py:15: def next_tick(event_loop, value):
> Doesn't this need a leading underscore?
> 
> https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode23
> tulip/queues.py:23: """Internal queue utility class"""
> Need a blank line below this.
> 
> https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode25
> tulip/queues.py:25: """Create a future with a timeout. If timeout is not None,
> it is an int
> I like to enforce the docstring style where, if there's more than one line,
the
> format is
> 
> """Summary sentence that fits on one line.
> <blank line>
> One or more paragraphs of text.
> """
> 
> https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode35
> tulip/queues.py:35: self._event_loop.call_later(
> As an optimization, consider saving the handle(r) that this returns and
calling
> cancel() on it when the result is set before the timeout is reached.
> 
> https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode53
> tulip/queues.py:53: """Create a queue object with a given maximum size.
> Don't use a verb to describe a class.
> 
> https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode55
> tulip/queues.py:55: If `maxsize` is ``None`` (the default) the queue size is
> unbounded.
> Please, no markup in docstrings.
> 
> https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode63
> tulip/queues.py:63: interrupted between calling :meth:`qsize` and doing an
> operation on the
> Ah, this is cool. :-)
> 
> https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode68
> tulip/queues.py:68: - `initial`: Optional sequence of initial items.
> How often is initial needed?  The standard queue doesn't have this; I'm
curious
> what the use case would be that makes it necessary to support this.
> 
> https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode75
> tulip/queues.py:75: raise ValueError("maxsize can't be negative")
> I recommend single quotes for all strings except docstrings.
> 
> Maybe add repr(maxsize) to the error message -- the actual value may help the
> programmer understand the bug better.
> 
> https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode127
> tulip/queues.py:127: def get_maxsize(self):
> Maybe make this an internal method (_get_maxsize) so people won't call it
> instead of referencing q.maxsize ?
> 
> https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode159
> tulip/queues.py:159: def set_maxsize(self, maxsize):
> What's the use case for adjusting maxsize?

Thanks guys, these are great critiques. I'll reply in detail tomorrow.

[A. Jesse Jiryu Davis, 2013-03-19 18:28:14]

Responses to Guido are inline.

Nikolay:


> 1. Should we reuse locks?

I tried that once with Toro and found it increased complexity substantially,
instead of reducing it.

> 2. Queue.put and Queue.get should be coroutines.

Thanks, done.

> 3. "block" parameter should be removed from put and get methods.
>    create separate methods for non-blocking calls.

Done -- the new methods are put_nowait and get_nowait, following Gevent.

> 4. i dont think you need next_tick and _Waiter 
>    in case if for put() and get() are coroutines.

I've removed next_tick; a simple yield works now. _Waiter is still useful for
managing the timeout.

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py
File tulip/queues.py (right):

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode15
tulip/queues.py:15: def next_tick(event_loop, value):
On 2013/03/18 20:10:22, GvR wrote:
> Doesn't this need a leading underscore?

Done.

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode23
tulip/queues.py:23: """Internal queue utility class"""
On 2013/03/18 20:10:22, GvR wrote:
> Need a blank line below this.

Done.

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode25
tulip/queues.py:25: """Create a future with a timeout. If timeout is not None,
it is an int
On 2013/03/18 20:10:22, GvR wrote:
> I like to enforce the docstring style where, if there's more than one line,
the
> format is
> 
> """Summary sentence that fits on one line.
> <blank line>
> One or more paragraphs of text.
> """

Done.

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode35
tulip/queues.py:35: self._event_loop.call_later(
On 2013/03/18 20:10:22, GvR wrote:
> As an optimization, consider saving the handle(r) that this returns and
calling
> cancel() on it when the result is set before the timeout is reached.

Thanks for the idea, done.

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode53
tulip/queues.py:53: """Create a queue object with a given maximum size.
On 2013/03/18 20:10:22, GvR wrote:
> Don't use a verb to describe a class.

Done.

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode55
tulip/queues.py:55: If `maxsize` is ``None`` (the default) the queue size is
unbounded.
On 2013/03/18 20:10:22, GvR wrote:
> Please, no markup in docstrings.

How surprising! I'm accustomed to shops that encourage the exuberant use of RST
in docstrings, for Sphinx. I'll remove it all.

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode68
tulip/queues.py:68: - `initial`: Optional sequence of initial items.
On 2013/03/18 20:10:22, GvR wrote:
> How often is initial needed?  The standard queue doesn't have this; I'm
curious
> what the use case would be that makes it necessary to support this.

I'll remove it, actually -- especially since it bypasses the usual code paths
and seems bug-prone. I forgot to check it against maxsize....

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode75
tulip/queues.py:75: raise ValueError("maxsize can't be negative")
On 2013/03/18 20:10:22, GvR wrote:
> I recommend single quotes for all strings except docstrings.
> 
> Maybe add repr(maxsize) to the error message -- the actual value may help the
> programmer understand the bug better.

In this case there's a single-quote within the string so I advocate keeping the
string double-quoted. I'll add the maxsize, though.

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode127
tulip/queues.py:127: def get_maxsize(self):
On 2013/03/18 20:10:22, GvR wrote:
> Maybe make this an internal method (_get_maxsize) so people won't call it
> instead of referencing q.maxsize ?

Done.

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode159
tulip/queues.py:159: def set_maxsize(self, maxsize):
On 2013/03/18 20:10:22, GvR wrote:
> What's the use case for adjusting maxsize?

That's a very good question. Perhaps you're implementing a bounded message queue
(like RabbitMQ) with Tulip, and you learn that total capacity has increased
(e.g. someone added a backend server). Perhaps you want to increase the queue's
maxsize in response, since you think your system can now work through a larger
backlog in the same amount of time?

I'm going to leave the feature in for a bit while we think about it, but I'm
willing to remove it before we complete this review.

[A. Jesse Jiryu Davis, 2013-03-19 18:35:39]

Second patch uploaded. This has:

* Nikolay's idea to separate blocking and nonblocking methods
* Nikolay's idea to make get() and put() coroutines
* remove next_tick()
* all Guido's style corrections
* Guido's optimization, call cancel() on the timeout handler
* remove 'initial' parameter as Guido recommended
* make _Waiter a subclass of Future

The only recommended changes I haven't made are:

* remove _Waiter -- I think it's still useful
* make maxsize immutable -- changing maxsize *could* be useful??

[GvR, 2013-03-19 20:55:15]

Good revision!  Did you sign the PSF contrib agreement yet?  I'm still pushing
back on the two issues you left open... :-)

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py
File tulip/queues.py (right):

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode75
tulip/queues.py:75: raise ValueError("maxsize can't be negative")
On 2013/03/19 18:28:15, A. Jesse Jiryu Davis wrote:
> On 2013/03/18 20:10:22, GvR wrote:
> > I recommend single quotes for all strings except docstrings.
> > 
> > Maybe add repr(maxsize) to the error message -- the actual value may help
the
> > programmer understand the bug better.
> 
> In this case there's a single-quote within the string so I advocate keeping
the
> string double-quoted. I'll add the maxsize, though.

D'oh. Good call. :-)

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode159
tulip/queues.py:159: def set_maxsize(self, maxsize):
On 2013/03/19 18:28:15, A. Jesse Jiryu Davis wrote:
> On 2013/03/18 20:10:22, GvR wrote:
> > What's the use case for adjusting maxsize?
> 
> That's a very good question. Perhaps you're implementing a bounded message
queue
> (like RabbitMQ) with Tulip, and you learn that total capacity has increased
> (e.g. someone added a backend server). Perhaps you want to increase the
queue's
> maxsize in response, since you think your system can now work through a larger
> backlog in the same amount of time?
> 
> I'm going to leave the feature in for a bit while we think about it, but I'm
> willing to remove it before we complete this review.

Have you had an actual use case like that in the queue you implemented for
Tornado?  If so, then we should probably leave it in -- with a big comment
explaining the use case.  If it is more of theoretical importance, then I
recommend leaving it out -- it's much easier to add functionality later than to
delete functionality once it has been released.

https://codereview.appspot.com/7751044/diff/8001/tests/queues_test.py
File tests/queues_test.py (right):

https://codereview.appspot.com/7751044/diff/8001/tests/queues_test.py#newcode4
tests/queues_test.py:4: from queue import Empty, Full  # exceptions from
standard lib's queue
I'd rather see you import the module and then reference these with the module
name as prefix (e.g. queue.Empty).

Also, as a general comment, please start your sentences with a capital letter
and terminate them with a period.

https://codereview.appspot.com/7751044/diff/8001/tests/queues_test.py#newcode10
tests/queues_test.py:10: from tulip.tasks import coroutine, task, Task
Here, also, I would rather say "from tulip import tasks".

https://codereview.appspot.com/7751044/diff/8001/tests/queues_test.py#newcode13
tests/queues_test.py:13: # TODO: looks like Tulip needs a more convenient means
of pausing
You missed tulip.tasks.sleep(). :-)

https://codereview.appspot.com/7751044/diff/8001/tests/queues_test.py#newcode247
tests/queues_test.py:247: return (yield from q.get(timeout=.1))
.1 -> 0.1 everywhere.

https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py
File tulip/queues.py (right):

https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode15
tulip/queues.py:15: def _next_tick(event_loop, value):
This is now unused, actually.

https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode32
tulip/queues.py:32: self.handler = self._event_loop.call_later(
Sorry, we've just renamed handler to handle...

https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode59
tulip/queues.py:59: item is delivered. (This is unlike the standard Queue, where
0 means
standard library Queue

https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode64
tulip/queues.py:64: interrupted between calling qsize() and doing an operation
on the Queue.
Maybe also add that you can change maxsize dynamically?

https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode70
tulip/queues.py:70: raise ValueError("maxsize can't be negative")
You didn't add the maxsize value though.

https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode120
tulip/queues.py:120: """Number of items allowed in the queue.
When I first saw this docstring I wondered why it was describing setting
maxsize.  I wonder if maybe it would be more appropriate to use

property(_get_maxsize, _set_maxsize, None, <docstring>)

below?

https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode189
tulip/queues.py:189: *before* the coroutine waiting for put() resumes.
Is this relative timing an important property to guarantee?  I could imagine all
sorts of situations where it might be hard to guarantee this ordering, or where
the user can't depend on it anyway because things are done a bit indirectly, and
I don't have a good grasp on when it would be important to make this guarantee.

https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode207
tulip/queues.py:207: yield
Hm... This use of a bare yield is actually undocumented and I'm not sure I want
to keep it. Also, again, I'm curious why it is so important to yield here.  I
would rather have the fast path through put() not yield to the scheduler at all
-- a scheduler roundtrip is kind of expensive.  (Read my recent post about yield
vs. yield-from for details.)

https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode216
tulip/queues.py:216: # The getter resumes now; the putter resumes on next tick
Ditto.

[Nikolay Kim, 2013-03-19 21:56:45]

i think _Waiter is quite useful on its own. lets rename it to
something like TimedFuture or TimeoutFuture and make it part of public api.

[gvrpython, 2013-03-19 22:04:45]

On Tue, Mar 19, 2013 at 2:56 PM, <fafhrd91@gmail.com> wrote:

> i think _Waiter is quite useful on its own. lets rename it to
> something like TimedFuture or TimeoutFuture and make it part of public
> api.
>

Agreed. But I was going to wait suggesting that refactoring until queues.py
is committed. Possibly it should just be an extra parameter on the basic
Future class? At some point in the past I had a Task class that also had a
timeout parameter, so it might be useful for that too.



> https://codereview.appspot.com/7751044/
>



-- 
--Guido van Rossum (python.org/~guido)

[Nikolay Kim, 2013-03-19 22:24:36]

On 2013/03/19 22:04:45, gvrpython wrote:
> On Tue, Mar 19, 2013 at 2:56 PM, <mailto:fafhrd91@gmail.com> wrote:
> 
> > i think _Waiter is quite useful on its own. lets rename it to
> > something like TimedFuture or TimeoutFuture and make it part of public
> > api.
> >
> 
> Agreed. But I was going to wait suggesting that refactoring until queues.py
> is committed. Possibly it should just be an extra parameter on the basic
> Future class? At some point in the past I had a Task class that also had a
> timeout parameter, so it might be useful for that too.

that was my initial idea, but i thought it would be too much :)

right now i have to write something like that, if i dont miss something:

  def coroutine(...):
     ...
     raise InternalError


  done, pending = yield from tasks.wait([tasks.Task(coroutine())], timeout=...)
  if done:
     try:
       res = done.pop().result()
     except InternalError:
       # process internal error
       ...
  else:
     # process timeout error
     ...


it could be more straightforward with timeout:

  try:
     res = yield from tasks.Tasks(coroutine(), timeout=...)
  except TimeoutError:
     # process timeout error
     ...
  except InternalError:
     # process internal error
     ...

[GvR, 2013-03-19 22:29:42]

On 2013/03/19 22:24:36, Nikolay Kim wrote:
> On 2013/03/19 22:04:45, gvrpython wrote:
> > On Tue, Mar 19, 2013 at 2:56 PM, <mailto:fafhrd91@gmail.com> wrote:
> > 
> > > i think _Waiter is quite useful on its own. lets rename it to
> > > something like TimedFuture or TimeoutFuture and make it part of public
> > > api.
> > >
> > 
> > Agreed. But I was going to wait suggesting that refactoring until queues.py
> > is committed. Possibly it should just be an extra parameter on the basic
> > Future class? At some point in the past I had a Task class that also had a
> > timeout parameter, so it might be useful for that too.
> 
> that was my initial idea, but i thought it would be too much :)
> 
> right now i have to write something like that, if i dont miss something:
> 
>   def coroutine(...):
>      ...
>      raise InternalError
> 
> 
>   done, pending = yield from tasks.wait([tasks.Task(coroutine())],
timeout=...)
>   if done:
>      try:
>        res = done.pop().result()
>      except InternalError:
>        # process internal error
>        ...
>   else:
>      # process timeout error
>      ...
> 
> 
> it could be more straightforward with timeout:
> 
>   try:
>      res = yield from tasks.Tasks(coroutine(), timeout=...)
>   except TimeoutError:
>      # process timeout error
>      ...
>   except InternalError:
>      # process internal error
>      ...

Sounds good if you can pull this off.  In the mean time let's encourage Jesse to
get his current patch in shape (it's pretty close).

[Nikolay Kim, 2013-03-20 18:28:38]

i've just added timeout support to Future.

[gvrpython, 2013-03-20 18:46:37]

(So, what Nikolay says, you can hg pull -u your repo, and then get rid of
your _Waiter class, in favor of the regular Future. Should be fun!)


On Wed, Mar 20, 2013 at 11:28 AM, <fafhrd91@gmail.com> wrote:

> i've just added timeout support to Future.
>
>
https://codereview.appspot.**com/7751044/<https://codereview.appspot.com/7751...
>



-- 
--Guido van Rossum (python.org/~guido)

[Nikolay Kim, 2013-03-20 22:11:29]

> https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode207
> tulip/queues.py:207: yield
> Hm... This use of a bare yield is actually undocumented and I'm not sure I
want
> to keep it. Also, again, I'm curious why it is so important to yield here.  I
> would rather have the fast path through put() not yield to the scheduler at
all
> -- a scheduler roundtrip is kind of expensive.  (Read my recent post about
yield
> vs. yield-from for details.)

probably we should create tasks.call_next_tick() api. 

BTW "yield from [None]" should work as well :)

[A. Jesse Jiryu Davis, 2013-03-20 22:23:46]

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py
File tulip/queues.py (right):

https://codereview.appspot.com/7751044/diff/1/tulip/queues.py#newcode159
tulip/queues.py:159: def set_maxsize(self, maxsize):
On 2013/03/19 20:55:15, GvR wrote:
> On 2013/03/19 18:28:15, A. Jesse Jiryu Davis wrote:
> > On 2013/03/18 20:10:22, GvR wrote:
> > > What's the use case for adjusting maxsize?
> > 
> > That's a very good question. Perhaps you're implementing a bounded message
> queue
> > (like RabbitMQ) with Tulip, and you learn that total capacity has increased
> > (e.g. someone added a backend server). Perhaps you want to increase the
> queue's
> > maxsize in response, since you think your system can now work through a
larger
> > backlog in the same amount of time?
> > 
> > I'm going to leave the feature in for a bit while we think about it, but I'm
> > willing to remove it before we complete this review.
> 
> Have you had an actual use case like that in the queue you implemented for
> Tornado?  If so, then we should probably leave it in -- with a big comment
> explaining the use case.  If it is more of theoretical importance, then I
> recommend leaving it out -- it's much easier to add functionality later than
to
> delete functionality once it has been released.

OK, I'm convinced and I'll remove it. This code can be put unmodified in a
subclass outside the standard library if anyone needs it.

I'll change it from a property to a method, also: queue.maxsize(). That's
consistent with qsize(), empty(), etc. Do you agree or should they all be
properties?

https://codereview.appspot.com/7751044/diff/8001/tests/queues_test.py
File tests/queues_test.py (right):

https://codereview.appspot.com/7751044/diff/8001/tests/queues_test.py#newcode4
tests/queues_test.py:4: from queue import Empty, Full  # exceptions from
standard lib's queue
On 2013/03/19 20:55:15, GvR wrote:
> I'd rather see you import the module and then reference these with the module
> name as prefix (e.g. queue.Empty).
> 
> Also, as a general comment, please start your sentences with a capital letter
> and terminate them with a period.

Done.

https://codereview.appspot.com/7751044/diff/8001/tests/queues_test.py#newcode10
tests/queues_test.py:10: from tulip.tasks import coroutine, task, Task
On 2013/03/19 20:55:15, GvR wrote:
> Here, also, I would rather say "from tulip import tasks".

Done.

https://codereview.appspot.com/7751044/diff/8001/tests/queues_test.py#newcode13
tests/queues_test.py:13: # TODO: looks like Tulip needs a more convenient means
of pausing
On 2013/03/19 20:55:15, GvR wrote:
> You missed tulip.tasks.sleep(). :-)

D'oh.

https://codereview.appspot.com/7751044/diff/8001/tests/queues_test.py#newcode247
tests/queues_test.py:247: return (yield from q.get(timeout=.1))
On 2013/03/19 20:55:15, GvR wrote:
> .1 -> 0.1 everywhere.

Done.

https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py
File tulip/queues.py (right):

https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode15
tulip/queues.py:15: def _next_tick(event_loop, value):
On 2013/03/19 20:55:15, GvR wrote:
> This is now unused, actually.

Done.

https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode59
tulip/queues.py:59: item is delivered. (This is unlike the standard Queue, where
0 means
On 2013/03/19 20:55:15, GvR wrote:
> standard library Queue

Done.

https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode70
tulip/queues.py:70: raise ValueError("maxsize can't be negative")
On 2013/03/19 20:55:15, GvR wrote:
> You didn't add the maxsize value though.

Oops. But now irrelevant -- in my next patch I won't validate maxsize at all,
same as stdlib doesn't validate it.

https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode120
tulip/queues.py:120: """Number of items allowed in the queue.
On 2013/03/19 20:55:15, GvR wrote:
> When I first saw this docstring I wondered why it was describing setting
> maxsize.  I wonder if maybe it would be more appropriate to use
> 
> property(_get_maxsize, _set_maxsize, None, <docstring>)
> 
> below?

Done.

https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode189
tulip/queues.py:189: *before* the coroutine waiting for put() resumes.
On 2013/03/19 20:55:15, GvR wrote:
> Is this relative timing an important property to guarantee?  I could imagine
all
> sorts of situations where it might be hard to guarantee this ordering, or
where
> the user can't depend on it anyway because things are done a bit indirectly,
and
> I don't have a good grasp on when it would be important to make this
guarantee.

Honestly, I don't have a good grasp either. This guarantee is made by Gevent's
queues, at least in the circumstances Gevent's unittests cover, so I copied the
guarantee in Toro and now in Tulip. I was assuming Denis Bilenko had a good
reason for it, but I'm reconsidering.

Note that the upcoming Gevent 1.0 release will change the behavior of
"Queue(0)": it will be an unlimited queue, the same as "Queue(None)". This is
the same API is the standard library Queue. The "channel" behavior of Gevent's
old Queue(0) will move into a separate Channel class in Gevent 1.0.

I've asked in the Gevent mailing list what the point of Channel is, and why
coroutines awakened by put() or get() must resume *before* the calling
coroutine.

In any case, in my next patch I'll just make tulip's Queue act less like
Gevent's and more like the standard library's: 0 means unbounded, and the
ordering guarantee is dropped.

https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode207
tulip/queues.py:207: yield
On 2013/03/19 20:55:15, GvR wrote:
> Hm... This use of a bare yield is actually undocumented and I'm not sure I
want
> to keep it. Also, again, I'm curious why it is so important to yield here.  I
> would rather have the fast path through put() not yield to the scheduler at
all
> -- a scheduler roundtrip is kind of expensive.  (Read my recent post about
yield
> vs. yield-from for details.)

Where can I see that post? Anyway, I'm going to remove this.

[A. Jesse Jiryu Davis, 2013-03-20 22:27:00]

This patch might be ready to merge (no outstanding issues I know of).

* _Waiter is gone

* maxsize() is now immutable, now a method instead of property

* Queue is now more like the standard library Queue and less like Gevent's:
Queue(0) now means "unbounded Queue" whereas in Gevent "Queue(0)" means
"channel." Queue(0) is now the default, rather than Queue(None). Queue(None) no
longer means anything.

* put() now continues if possible, instead of immediately handing control to any
coroutine that's paused at "yield from q.get()"

* same for get(): it now continues instead of handing control to a coroutine
yielding from put()

* no more raw yields

I *have* signed the contributor agreement.

[Nikolay Kim, 2013-03-20 22:59:20]

On 2013/03/20 22:27:00, A. Jesse Jiryu Davis wrote:
> This patch might be ready to merge (no outstanding issues I know of).
> 
> * _Waiter is gone
> 
> * maxsize() is now immutable, now a method instead of property
> 
> * Queue is now more like the standard library Queue and less like Gevent's:
> Queue(0) now means "unbounded Queue" whereas in Gevent "Queue(0)" means
> "channel." Queue(0) is now the default, rather than Queue(None). Queue(None)
no
> longer means anything.
> 
> * put() now continues if possible, instead of immediately handing control to
any
> coroutine that's paused at "yield from q.get()"
> 
> * same for get(): it now continues instead of handing control to a coroutine
> yielding from put()
> 
> * no more raw yields
> 
> I *have* signed the contributor agreement.

i can't apply latest patch. seems you did some local commits.

[GvR, 2013-03-21 00:16:48]

Just one thing left, really -- maxsize needs to be a property.  Feel free to
commit after fixing this (and the task->coroutine thing in the tests), doing hg
pull -u (and possibly fixing conflicts), and running all tests once more.

(Nikolay: the problems with applying the patch are probably because Jesse is way
behind compared to the tip.)

https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py
File tulip/queues.py (right):

https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode189
tulip/queues.py:189: *before* the coroutine waiting for put() resumes.
On 2013/03/20 22:23:46, A. Jesse Jiryu Davis wrote:
> On 2013/03/19 20:55:15, GvR wrote:
> > Is this relative timing an important property to guarantee?  I could imagine
> all
> > sorts of situations where it might be hard to guarantee this ordering, or
> where
> > the user can't depend on it anyway because things are done a bit indirectly,
> and
> > I don't have a good grasp on when it would be important to make this
> guarantee.
> 
> Honestly, I don't have a good grasp either. This guarantee is made by Gevent's
> queues, at least in the circumstances Gevent's unittests cover, so I copied
the
> guarantee in Toro and now in Tulip. I was assuming Denis Bilenko had a good
> reason for it, but I'm reconsidering.
> 
> Note that the upcoming Gevent 1.0 release will change the behavior of
> "Queue(0)": it will be an unlimited queue, the same as "Queue(None)". This is
> the same API is the standard library Queue. The "channel" behavior of Gevent's
> old Queue(0) will move into a separate Channel class in Gevent 1.0.
> 
> I've asked in the Gevent mailing list what the point of Channel is, and why
> coroutines awakened by put() or get() must resume *before* the calling
> coroutine.
> 
> In any case, in my next patch I'll just make tulip's Queue act less like
> Gevent's and more like the standard library's: 0 means unbounded, and the
> ordering guarantee is dropped.

Thanks!  You can add a Channel class later.

https://codereview.appspot.com/7751044/diff/8001/tulip/queues.py#newcode207
tulip/queues.py:207: yield
On 2013/03/20 22:23:46, A. Jesse Jiryu Davis wrote:
> On 2013/03/19 20:55:15, GvR wrote:
> > Hm... This use of a bare yield is actually undocumented and I'm not sure I
> want
> > to keep it. Also, again, I'm curious why it is so important to yield here. 
I
> > would rather have the fast path through put() not yield to the scheduler at
> all
> > -- a scheduler roundtrip is kind of expensive.  (Read my recent post about
> yield
> > vs. yield-from for details.)
> 
> Where can I see that post? Anyway, I'm going to remove this.

In the python-tulip Google Group.

https://codereview.appspot.com/7751044/diff/23001/tests/queues_test.py
File tests/queues_test.py (right):

https://codereview.appspot.com/7751044/diff/23001/tests/queues_test.py#newcod...
tests/queues_test.py:126: @tasks.task
You can now make this use coroutine instead of task.

https://codereview.appspot.com/7751044/diff/23001/tulip/queues.py
File tulip/queues.py (right):

https://codereview.appspot.com/7751044/diff/23001/tulip/queues.py#newcode78
tulip/queues.py:78: def maxsize(self):
This needs to be a property to be compatible with the stdlib -- it has maxsize
as an attribute, but qsize() and empty() as methods (because the latter need to
acquire the lock).

[A. Jesse Jiryu Davis, 2013-03-21 00:50:26]

https://codereview.appspot.com/7751044/diff/23001/tests/queues_test.py
File tests/queues_test.py (right):

https://codereview.appspot.com/7751044/diff/23001/tests/queues_test.py#newcod...
tests/queues_test.py:126: @tasks.task
On 2013/03/21 00:16:48, GvR wrote:
> You can now make this use coroutine instead of task.

Cool; I'll use coroutine throughout the test suite, then. When I need a task
I'll do task.Task() at the call site, rather than the definition site.

https://codereview.appspot.com/7751044/diff/23001/tulip/queues.py
File tulip/queues.py (right):

https://codereview.appspot.com/7751044/diff/23001/tulip/queues.py#newcode78
tulip/queues.py:78: def maxsize(self):
On 2013/03/21 00:16:48, GvR wrote:
> This needs to be a property to be compatible with the stdlib -- it has maxsize
> as an attribute, but qsize() and empty() as methods (because the latter need
to
> acquire the lock).

Done.

[A. Jesse Jiryu Davis, 2013-03-21 01:02:08]

On 2013/03/21 00:50:26, A. Jesse Jiryu Davis wrote:
> https://codereview.appspot.com/7751044/diff/23001/tests/queues_test.py
> File tests/queues_test.py (right):
> 
>
https://codereview.appspot.com/7751044/diff/23001/tests/queues_test.py#newcod...
> tests/queues_test.py:126: @tasks.task
> On 2013/03/21 00:16:48, GvR wrote:
> > You can now make this use coroutine instead of task.
> 
> Cool; I'll use coroutine throughout the test suite, then. When I need a task
> I'll do task.Task() at the call site, rather than the definition site.
> 
> https://codereview.appspot.com/7751044/diff/23001/tulip/queues.py
> File tulip/queues.py (right):
> 
> https://codereview.appspot.com/7751044/diff/23001/tulip/queues.py#newcode78
> tulip/queues.py:78: def maxsize(self):
> On 2013/03/21 00:16:48, GvR wrote:
> > This needs to be a property to be compatible with the stdlib -- it has
maxsize
> > as an attribute, but qsize() and empty() as methods (because the latter need
> to
> > acquire the lock).
> 
> Done.

Thanks Guido and Nikolay, this has been an insanely great code review.

[GvR, 2013-03-21 01:46:23]

On 2013/03/21 01:02:08, A. Jesse Jiryu Davis wrote:
> Thanks Guido and Nikolay, this has been an insanely great code review.

Thanks Jesse for the contribution!!  I hope you will make more.

(Did you get the email about using parameter-less super()?)