Feature #18982
closedAdd an `exception: false` argument for Queue#push, Queue#pop, SizedQueue#push and SizedQueue#pop
Description
This replaces [Feature #18965]
Currently these methods raise in three occasions:
-
ThreadError(queue empty)
for#pop
innonblock=true
mode, and the operation would block. -
ThreadError(queue full)
forSizedQueue#push
innonblock=true
mode, and the operation would block. -
ClosedQueueError
if trying to#push
in a closed queue.
I see several reasons to prefer a nil
return value.
- Queue is often used in conjunction with threads, so you have to be very careful not to rescue an unrelated
ThreadError
. - Queue if often used for low level code, deep in the stack, so exceptions are costly.
I propose that passing exception: false
would cause the method to return nil
instead of raising in the three cases listed above.
The argument exception: true
is consistent with various other methods such as IO#read_nonblock(exception: false)
.
Updated by byroot (Jean Boussier) about 2 years ago
- Related to Feature #18965: Further Thread::Queue improvements added
Updated by byroot (Jean Boussier) about 2 years ago
Proposed patch: https://github.com/ruby/ruby/pull/6299
Updated by Eregon (Benoit Daloze) about 2 years ago
Sounds good :+1:
Updated by shyouhei (Shyouhei Urabe) about 2 years ago
+1 for avoiding exceptions but nil
can be problematic? Because a closed queue would also return nil
for pop
. You cannot distinguish if a queue is closed or would just block.
Updated by byroot (Jean Boussier) about 2 years ago
but nil can be problematic?
This was discussed in #18774:
-
#pop
on closed queue already returnsnil
with no exception.Queue.new.tap(&:close).pop # => nil
-
nil
make sense because you can use it in a loop such aswhile item = queue.pop
. - There's not really any other value that make sense.
Overall it's really not that complicated to handle the various cases in which nil is returned. I have an real world example here: https://github.com/Shopify/statsd-instrument/blob/0af8a1e2e74fc24d1de8088ee995940033c1fe42/lib/statsd/instrument/batched_udp_sink.rb#L111-L138.
Updated by jeremyevans0 (Jeremy Evans) about 2 years ago
shyouhei (Shyouhei Urabe) wrote in #note-4:
+1 for avoiding exceptions but
nil
can be problematic? Because a closed queue would also returnnil
forpop
. You cannot distinguish if a queue is closed or would just block.
We could add another keyword argument for the exception value, and have that value returned instead of raising an exception (the keyword argument would default to nil
). I'm not sure if it's worth supporting that, but it is a simple approach.
Updated by shyouhei (Shyouhei Urabe) about 2 years ago
I don't think people want to exit from a while item = queue.pop
loop because the queue would block. for (;;) { if (errno != EAGAIN) break; ... }
is a C idiom (people often break from a loop on error, except EAGAIN).
Updated by ko1 (Koichi Sasada) about 2 years ago
Queue#pop(timeout:0)
returns nil
if Queue is empty. Is it enough? (and forget optional nonblock argument)
Updated by byroot (Jean Boussier) about 2 years ago
Is it enough?
Interesting, I never thought of that.
I think I can work with that, but then I think this behavior should be documented and speced.
Because timeout: 0
in some APIs (not necessarily ruby) can mean no timeout.
Updated by ko1 (Koichi Sasada) about 2 years ago
Because timeout: 0 in some APIs (not necessarily ruby) can mean no timeout.
Already the implementation do -> "Queue#pop(timeout:0) returns nil if Queue is empty" so no problem?
Updated by byroot (Jean Boussier) about 2 years ago
Works for me. I'll leave this ticket open until I add some more spec and documentation to the existing method.
Updated by byroot (Jean Boussier) about 2 years ago
- Status changed from Open to Closed
Applied in changeset git|60defe0a68a40d1b3225cf6b971ea195e19ae2e2.
thread_sync.c: Clarify and document the behavior of timeout == 0
[Feature #18982]
Instead of introducing an exception: false
argument to have non_block
return nil rather than raise, we can clearly document that a timeout of 0
immediately returns.
The code is refactored a bit to avoid doing a time calculation in
such case.