Feature #18774
closedAdd Queue#pop(timeout:)
Description
This has been mentioned many times but somehow was never added.
It is useful for many different use cases:
- Implementing Timeout#timeout without needing to create a Thread per call which is very inefficient (especially when the timeout is not hit): https://github.com/ruby/timeout/pull/14#issuecomment-1123380880
- @jeremyevans0 (Jeremy Evans) I would love a Queue#pop :timeout argument. It would simplify the mutex/condition variable approach currently used for Sequel's connection pool.
- @byroot (Jean Boussier) Same. I wanted it so many times
- https://bugs.ruby-lang.org/issues/17363
- https://spin.atomicobject.com/2014/07/07/ruby-queue-pop-timeout/ + https://spin.atomicobject.com/2017/06/28/queue-pop-with-timeout-fixed/
- More in my email searches but this seems already plenty
I think it should be a keyword argument for clarity, and so there is no confusion with the existing optional argument non_block=false
.
Updated by byroot (Jean Boussier) over 2 years ago
In https://bugs.ruby-lang.org/issues/17363 there was some discussion on what the return value or exception should be when timeout is reached, and it seems to be part of why the discussion stalled.
@ko1 (Koichi Sasada) said:
return nil -> can't recognize returned value
Which I somewhat agree with, however pop
already returns nil when called in "non_block" mode, or when the Queue is closed, so I think returning nil
while not so great, would be the most consistent behavior.
Updated by Eregon (Benoit Daloze) over 2 years ago
Indeed, I forgot to mention that. I think returning nil
is easiest, most consistent and convenient.
I believe it is already common practice to use another element than nil
in a Queue for special markers, so it seems a rare issue in practice.
however pop already returns nil when called in "non_block" mode
Actually it's queue empty (ThreadError)
in that case. Which seems pretty expensive for such a poll case, but that's the current semantics.
When the queue is closed, then queue.pop
returns nil
indeed.
FWIW the semantics of Rubinius' Channel#receive_timeout
is to return nil
when timed out:
https://github.com/rubinius/rubinius/blob/b7a755c83f3dd3f0c1f5e546f0e58fb61851ea44/machine/class/channel.cpp#L96
Updated by ioquatix (Samuel Williams) over 2 years ago
I also think nil
can work, but shouldn't we also consider a hypothetical Queue::TimeoutError
similar to how all other timeouts work? Although I hate using exceptions for non-exceptional code paths.
Updated by Eregon (Benoit Daloze) over 2 years ago
If others feel strongly it should be an exception, I think it should be Thread::TimeoutError
(we already have Timeout::Error
, Regexp::TimeoutError
and probably IO::TimeoutError
in #18630).
I'd inherit from StandardError
as that's the common ancestor for those.
Agreed with @ioquatix (Samuel Williams) that in practice almost nobody wants an exception for something expected like here (i.e., the code would be queue.pop(timeout: 1.5)
it's clear a timeout is a possible result).
Just like everyone tends to use exception: false
for the *_nonblock
IO methods.
Updated by byroot (Jean Boussier) over 2 years ago
shouldn't we also consider a hypothetical Queue::TimeoutError
IMHO, consistency with closed queues is really more important.
similar to how all other timeouts work?
@Eregon (Benoit Daloze) just beat me to it, but whenever I can I actually use non-exception versions of methods that take a timeout, e.g. read_nonblock(exception: false)
.
For many the argument is that Exceptions should be for exceptional cases, but IMO the main argument is that these methods tend to be very low in the stack, and raising an exception that deep is really costly.
In the case of Queue#pop
, I can already imagine some code calling pop(timeout: 0.1)
in a loop to regularly check whether it should stop waiting, it would be a bit silly to raise an exception every time.
Updated by mame (Yusuke Endoh) over 2 years ago
- Related to Feature #17363: Timeouts added
Updated by Eregon (Benoit Daloze) over 2 years ago
One precision: nil
is the best option for performance (and convenience).
Using an exception on queue pop timeout would be a significant performance overhead, because an exception needs a stacktrace and that's slow on all Ruby implementations (there is the possibility to not give a stacktrace/empty one but then it's not really an exception anymore).
A timeout on Queue#pop is something expected and might happen regularly, so using an exception here is also a bad fit because it's not exceptional/rare.
Updated by Eregon (Benoit Daloze) over 2 years ago
Also, this timeout is quite similar to IO.select's timeout (i.e. expected to timeout), and IO.select returns nil
on timeout:
r, w = IO.pipe; p IO.select([r], nil, nil, 1.0) => nil
Updated by ioquatix (Samuel Williams) over 2 years ago
There are several methods which should adopt timeouts, including SizedQueue#push
and probably Queue#each
, along with essentially any other blocking operations (e.g. Kernel#system
, Process#wait
etc).
I'd like to suggest we try to agree on a general method signature for timeouts, e.g.
Queue#pop(timeout:, exception: true/false/class)
We can share this signature across all methods using a small C interface like this:
struct rb_timeout_handler {
VALUE timeout; // Qnil or timeout value.
VALUE klass; // Qnil -> return nil, Qtrue -> raise TimeoutError, or exception class -> raise that class
};
void rb_timeout_make_handler(VALUE kwargs, struct timeout_handler *);
VALUE rb_timeout_handler_invoke(struct timeout_handler *) // Used if a timeout occurs to generate a return value or raise an exception.
I also think we should introduce a top level TimeoutError
(or just Timeout
like Interrupt
). Otherwise we are going to end up with multiple "Timeout" exceptions and make it hard for users. That doesn't mean we can't have specific Queue::Timeout
exceptions, just that we should have something like Queue::Timeout < Timeout < Exception
or Queue::TimeoutError < TimeoutError < StandardError
.
Updated by matz (Yukihiro Matsumoto) over 2 years ago
I accept the original proposal Queue.pop(timeout: sec). You may consider adding timeout_value keyword argument. But it should be a different issue.
Matz.
Updated by mame (Yusuke Endoh) over 2 years ago
matz (Yukihiro Matsumoto) wrote in #note-10:
You may consider adding timeout_value keyword argument. But it should be a different issue.
A supplimental explanation. We discussed this ticket at the dev meeting, and someone suggested yet another keyword argument to specify a return value when time limit exceeded.
Queue#pop(timeout: sec, timeout_value: :TimeOut) #=> :TimeOut instead of nil at timeout
@matz (Yukihiro Matsumoto) said returning nil would be good enough in many cases, and wanted to discuss it in another ticket if anyone really wants it.
Updated by byroot (Jean Boussier) over 2 years ago
So I started to implement Queue#pop(timeout:)
and SizedQueue#pop(timeout:)
https://github.com/ruby/ruby/pull/6185
As pointed by @ioquatix (Samuel Williams), I believe that SizedQueue#push
should have a timeout as well.
Updated by Eregon (Benoit Daloze) over 2 years ago
byroot (Jean Boussier) wrote in #note-12:
So I started to implement
Queue#pop(timeout:)
andSizedQueue#pop(timeout:)
https://github.com/ruby/ruby/pull/6185
Thanks!
As pointed by @ioquatix (Samuel Williams), I believe that
SizedQueue#push
should have a timeout as well.
I think it'd be fine to add, and I don't think that needs to be discussed in a dev meeting again since it's so similar.
It seems good to add for consistency so all 3 blocking Queue/SizedQueue methods (the 2 #pop + SizedQueue#push) have a timeout.
Updated by byroot (Jean Boussier) over 2 years ago
I think it'd be fine to add, and I don't think that needs to be discussed in a dev meeting again since it's so similar.
Agreed. Also SizedQueue#push
already has non_block=false
like Queue#pop
, so it makes perfect sense. I'll implement it without first asking approval.
Updated by mame (Yusuke Endoh) over 2 years ago
According to the dev meeting log, @matz (Yukihiro Matsumoto) said "Create another ticket for SizedQueue#push if someone wants it".
Updated by byroot (Jean Boussier) over 2 years ago
@mame (Yusuke Endoh) thanks for bringing this out, I definitely missed it. I'll open another ticket tomorrow then.
Updated by byroot (Jean Boussier) over 2 years ago
I created https://bugs.ruby-lang.org/issues/18944
Updated by byroot (Jean Boussier) over 2 years ago
- Status changed from Open to Closed
Applied in changeset git|e3aabe93aae87a60ba7b8f1a0fd590534647e352.
Implement Queue#pop(timeout: sec)
[Feature #18774]
As well as SizedQueue#pop(timeout: sec)
If both non_block=true
and timeout:
are supplied, ArgumentError
is raised.