Project

General

Profile

Actions

Feature #17363

open

Timeouts

Added by marcandre (Marc-Andre Lafortune) 8 months ago. Updated 4 months 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`Closedioquatix (Samuel Williams)Actions
Related to Ruby master - Feature #17849: Fix Timeout.timeout so that it can be used in threaded Web serversOpenmatz (Yukihiro Matsumoto)Actions

Updated by jeremyevans0 (Jeremy Evans) 8 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.

Actions #2

Updated by Eregon (Benoit Daloze) 8 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) 8 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) 8 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) 8 months ago

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

Updated by marcandre (Marc-Andre Lafortune) 8 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) 8 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) 8 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) 8 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) 8 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) 8 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) 8 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) 8 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).

Actions #14

Updated by Eregon (Benoit Daloze) 7 months ago

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

Updated by ko1 (Koichi Sasada) 7 months ago

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

Updated by ioquatix (Samuel Williams) 6 months 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) 6 months 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)

Updated by Eregon (Benoit Daloze) 4 months ago

Timeout.wake sounds a bit like Java's Thread#interrupt(), correct?

So it would interrupt blocking calls (File.read/Queue#pop/rb_thread_call_without_gvl/sleep/Mutex#lock/etc) but wouldn't interrupt not-blocking Ruby code like loop{1+1} or while true; 1+1; end.

Also if it happens while the Thread is not doing a blocking call it should probably set a flag that's then checked before any of these blocking calls (like Java's Thread#interrupt()), otherwise it would be too easy to lose such an interrupt/timeout.
I'm not entirely sure how it would work to check the flag just before the blocking call and making sure to not lose an interrupt sent in between, but it should be possible.

Related discussion: https://twitter.com/schneems/status/1377309342819512320 and https://www.schneems.com/2017/02/21/the-oldest-bug-in-ruby-why-racktimeout-might-hose-your-server/

Actions #19

Updated by Eregon (Benoit Daloze) 3 months ago

  • Related to Feature #17849: Fix Timeout.timeout so that it can be used in threaded Web servers added
Actions

Also available in: Atom PDF