Project

General

Profile

Actions

Feature #17363

open

Timeouts

Added by marcandre (Marc-Andre Lafortune) almost 2 years ago. Updated 7 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 3 (1 open2 closed)

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
Related to Ruby master - Feature #18774: Add Queue#pop(timeout:)ClosedActions

Updated by jeremyevans0 (Jeremy Evans) almost 2 years 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) almost 2 years 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) almost 2 years 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) almost 2 years 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) almost 2 years ago

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

Updated by marcandre (Marc-Andre Lafortune) almost 2 years 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) almost 2 years 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) almost 2 years 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) almost 2 years 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) almost 2 years 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) almost 2 years 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) almost 2 years 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) almost 2 years 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) almost 2 years ago

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

Updated by ko1 (Koichi Sasada) almost 2 years ago

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

Updated by ioquatix (Samuel Williams) almost 2 years 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) almost 2 years 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) over 1 year 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) over 1 year ago

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

Updated by Eregon (Benoit Daloze) 7 months ago

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

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

I think there is actually no use case for that besides "it looks nice/unified".
In practice if one rescues some kind of timeout I think they should use a specific class.
If they want to rescue "anything" and retry then rescue StandardError (or just rescue) covers better than just ::TimeoutError.

BTW, Regexp::TimeoutError is (currently) < RegexpError < StandardError, so it can't < ::TimeoutError unless it no longer < RegexpError.

Actions #21

Updated by mame (Yusuke Endoh) 6 months ago

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0