Project

General

Profile

Actions

Feature #18774

closed

Add Queue#pop(timeout:)

Added by Eregon (Benoit Daloze) almost 2 years ago. Updated over 1 year ago.

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

Description

This has been mentioned many times but somehow was never added.
It is useful for many different use cases:

I think it should be a keyword argument for clarity, and so there is no confusion with the existing optional argument non_block=false.


Related issues 1 (1 open0 closed)

Related to Ruby master - Feature #17363: TimeoutsAssignedko1 (Koichi Sasada)Actions
Actions #1

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

Actions #6

Updated by mame (Yusuke Endoh) almost 2 years ago

Updated by Eregon (Benoit Daloze) almost 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) almost 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) almost 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) almost 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) almost 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 1 year 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 1 year ago

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.

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

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

Actions #18

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

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0