Project

General

Profile

Actions

Feature #18982

closed

Add an `exception: false` argument for Queue#push, Queue#pop, SizedQueue#push and SizedQueue#pop

Added by byroot (Jean Boussier) over 1 year ago. Updated over 1 year ago.

Status:
Closed
Assignee:
-
Target version:
-
[ruby-core:109756]

Description

This replaces [Feature #18965]

Currently these methods raise in three occasions:

  • ThreadError(queue empty) for #pop in nonblock=true mode, and the operation would block.
  • ThreadError(queue full) for SizedQueue#push in nonblock=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).


Related issues 1 (0 open1 closed)

Related to Ruby master - Feature #18965: Further Thread::Queue improvementsRejectedActions
Actions #1

Updated by byroot (Jean Boussier) over 1 year ago

Updated by Eregon (Benoit Daloze) over 1 year ago

Sounds good :+1:

Updated by shyouhei (Shyouhei Urabe) over 1 year 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) over 1 year ago

but nil can be problematic?

This was discussed in #18774:

  • #pop on closed queue already returns nil with no exception. Queue.new.tap(&:close).pop # => nil
  • nil make sense because you can use it in a loop such as while 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) over 1 year ago

shyouhei (Shyouhei Urabe) wrote in #note-4:

+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.

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) over 1 year 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) over 1 year ago

Queue#pop(timeout:0) returns nil if Queue is empty. Is it enough? (and forget optional nonblock argument)

Updated by byroot (Jean Boussier) over 1 year 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) over 1 year 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) over 1 year ago

Works for me. I'll leave this ticket open until I add some more spec and documentation to the existing method.

Actions #12

Updated by byroot (Jean Boussier) over 1 year 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.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0