Feature #18774: Add Queue#pop(timeout:)

• Status: Closed
• Assignee: -
• Target version: -

[Eregon (Benoit Daloze)]

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.

[byroot (Jean Boussier)]

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.

[Eregon (Benoit Daloze)]

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

[ioquatix (Samuel Williams)]

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.

[Eregon (Benoit Daloze)]

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.

[byroot (Jean Boussier)]

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.

[mame (Yusuke Endoh)]

• Related to Feature #17363: Timeouts added

[Eregon (Benoit Daloze)]

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.

[Eregon (Benoit Daloze)]

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

[ioquatix (Samuel Williams)]

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.

[matz (Yukihiro Matsumoto)]

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.

[mame (Yusuke Endoh)]

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.

[byroot (Jean Boussier)]

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.

[Eregon (Benoit Daloze)]

byroot (Jean Boussier) wrote in #note-12:

So I started to implement Queue#pop(timeout:) and SizedQueue#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.

[byroot (Jean Boussier)]

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.

[mame (Yusuke Endoh)]

According to the dev meeting log, @matz (Yukihiro Matsumoto) said "Create another ticket for SizedQueue#push if someone wants it".

https://github.com/ruby/dev-meeting-log/blob/master/DevMeeting-2022-05-19.md#feature-18774-add-queuepoptimeout-eregon

[byroot (Jean Boussier)]

@mame (Yusuke Endoh) thanks for bringing this out, I definitely missed it. I'll open another ticket tomorrow then.

[byroot (Jean Boussier)]

I created https://bugs.ruby-lang.org/issues/18944

[byroot (Jean Boussier)]

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