Project

General

Profile

Feature #17363

Timeouts

Added by marcandre (Marc-Andre Lafortune) 3 months ago. Updated 18 days ago.

Status:
Assigned
Priority:
Normal
Target version:
-
[ruby-core:101217]

Description

Builtin methods like Queue.pop and Ractor.receive have no timeout parameter.

We should either:

  • provide such a parameter
  • and/or provide a Timeout::wake that raises an timeout error only if the block is currently sleeping.

Details:

q = Queue.new
# ...
elem = Timeout::timeout(42) { q.pop } # => It is possible that an element is retreived from the queue but never stored in `elem`

elem = Timeout::wake(42) { q.pop } # => Guaranteed that either element is retrieved from the queue or an exception is raised, never both
Timeout::wake(42) { loop {} } # => infinite loop
# and/or
elem = q.pop(timeout: 42)

Currently, the only reliable way to have a Queue that accepts a timeout is to re-implement it from scratch. This post describe how involved that can be: https://spin.atomicobject.com/2017/06/28/queue-pop-with-timeout-fixed/


Related issues

Related to Ruby master - Feature #17470: Introduce non-blocking `Timeout.timeout`OpenActions

Updated by jeremyevans0 (Jeremy Evans) 3 months ago

I've wanted a timed version of Queue#pop for a long time, to use as the backed for Sequel's connection pool. I was thinking of a separate method (Queue#timed_pop), but a keyword argument works fine too. I think either is better than Timeout.wake.

#2

Updated by Eregon (Benoit Daloze) 3 months ago

+1 for Queue#pop(timeout: 42).

FWIW TruffleRuby already has Queue#receive_timeout as a private method,
and this is used to implement Timeout.timeout without creating a new Thread every time.


It sounds like the proposed Timeout.wake{} would be similar to Thread#wakeup.
I'm not sure how it could work, because reading another thread state is always racy (without a GIL), and the thread checking timeouts must be a separate thread than the one doing the blocking call.
Also it could interrupt a blocking call in ensure (e.g., cleaning up a connection), which would be unwanted.

Updated by nobu (Nobuyoshi Nakada) 3 months ago

I'm positive about that option too.
But I wonder how Timeout.wake works and if it is possible.

Updated by ko1 (Koichi Sasada) 3 months ago

I also positive to introduce timeout but not sure what happens on timeout.

  • raise an exception -> which exception?
  • return nil -> can't recognize returned value

I think timeout: nil is same as no timeout: given. Is it same as other methods?

Updated by ko1 (Koichi Sasada) 3 months ago

Timeout::wake you can implement it with Thread#handle_interrupt(RuntimeError => :never){ ... }.

Updated by marcandre (Marc-Andre Lafortune) 3 months ago

ko1 (Koichi Sasada) wrote in #note-4:

I also positive to introduce timeout but not sure what happens on timeout.

  • raise an exception -> which exception?

How about subclassing Timeout::Error to create Queue::Timeout and Ractor::Timeout?

  • return nil -> can't recognize returned value

Agreed, it is not a good solution.

I think timeout: nil is same as no timeout: given. Is it same as other methods?

Agree.

I think queue.pop(timeout: 0) should be same as queue.pop(true) but raise Queue:Timeout.

Same idea with Ractor, timeout: 0 is non-blocking version of Ractor.receive/receive_if/select.

Updated by Eregon (Benoit Daloze) 3 months ago

marcandre (Marc-Andre Lafortune) wrote in #note-6:

How about subclassing Timeout::Error to create Queue::Timeout and Ractor::Timeout?

Timeout is stdlib, unlike the other 2 which are in core, so that's an issue.

Updated by marcandre (Marc-Andre Lafortune) 3 months ago

Eregon (Benoit Daloze) wrote in #note-7:

marcandre (Marc-Andre Lafortune) wrote in #note-6:

How about subclassing Timeout::Error to create Queue::Timeout and Ractor::Timeout?

Timeout is stdlib, unlike the other 2 which are in core, so that's an issue.

Good point. We could create Thread::Timeout as a common base class for all 3?

Updated by Eregon (Benoit Daloze) 3 months ago

marcandre (Marc-Andre Lafortune) wrote in #note-8:

Good point. We could create Thread::Timeout as a common base class for all 3?

Thread::TimeoutError then maybe?
Sounds OK, but not sure timeouts are always related to threads (e.g., an IO.select timeout).
Might not matter much, so Thread::TimeoutError is fine for me.

Updated by marcandre (Marc-Andre Lafortune) 3 months ago

We could also define ::TimeoutError as base class, and modify timeout lib so that Timeout::Error < ::TimeoutError instead of == as it is currently.

Updated by nobu (Nobuyoshi Nakada) 3 months ago

It is just one line to built-in Timeout::Error.

rb_define_class_under(rb_define_module("Timeout"), "Error", rb_eRuntimeError);

Updated by Eregon (Benoit Daloze) 3 months ago

nobu (Nobuyoshi Nakada) wrote in #note-11:

It is just one line to built-in Timeout::Error.

rb_define_class_under(rb_define_module("Timeout"), "Error", rb_eRuntimeError);

I think that would be confusing, if Timeout::Error is in core, and so a Timeout module is always defined, and yet Timeout.timeout is not defined.

Updated by Eregon (Benoit Daloze) 3 months ago

So another option would be to move the timeout stdlib to core, which could be interesting (can be better optimized, avoid an extra Ruby thread, etc).

#14

Updated by Eregon (Benoit Daloze) 2 months ago

  • Related to Feature #17470: Introduce non-blocking `Timeout.timeout` added

Updated by ko1 (Koichi Sasada) about 2 months ago

  • Assignee set to ko1 (Koichi Sasada)
  • Status changed from Open to Assigned

Updated by ioquatix (Samuel Williams) 23 days ago

This seems like a good idea. Thank you everyone for the great discussion here.

I agree with the following things:

  • Move Timeout to core.
  • Add Timeout::Error as base class in core.
  • Add new method for predictable timeout during sleeping operations (e.g. Timeout.wake or something similar).

In terms of queue and ractor, I'm less inclined to support:

  • timeout: keyword argument.
  • Custom exception classes for Ractor, Queue and so on.

I'm not against it, I'm just not sure if it's useful in practice. I think the latter feature should be separate issue/PR if possible.

Finally, I'd also like to suggest that we deprecate Timeout.timeout once this is merged.

Updated by naruse (Yui NARUSE) 18 days ago

Through my experience on implementing write_timeout for net/http, there are 2 layers for this type of APIs.
Low-level layer has SELECT (or wait_readable/wait_writable) and nonblock write/read APIs. (IO and Socket)
High-level layer has timeout APIs which is implemented with above. (Net::BufferedIO and Net::HTTP)

Also available in: Atom PDF