Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raise exception instead of throw/catch for timeouts #30

Merged
merged 5 commits into from
Jun 22, 2023

Conversation

jeremyevans
Copy link
Contributor

throw/catch is used for non-local control flow, not for exceptional situations. For exceptional situations, raise should be used instead. A timeout is an exceptional situation, so it should use raise, not throw/catch.

Timeout's implementation that uses throw/catch internally causes serious problems. Consider the following code:

def handle_exceptions
  yield
rescue Exception => exc
  handle_error # e.g. ROLLBACK for databases
  raise
ensure
  handle_exit unless exc # e.g. COMMIT for databases
end

Timeout.timeout(1) do
  handle_exceptions do
    do_something
  end
end

This kind of design ensures that all exceptions are handled as errors, and ensures that all exits (normal exit, early return, throw/catch) are not handled as errors. With Timeout's throw/catch implementation, this type of code does not work, since a timeout triggers the normal exit path.

See rails/rails#29333 for an example of the damage Timeout's design has caused the Rails ecosystem.

This switches Timeout.timeout to use raise/rescue internally. It adds a Timeout::ExitException subclass of exception for the internal raise/rescue, which Timeout.timeout will convert to Timeout::Error for backwards compatibility. Timeout::Error remains a subclass of RuntimeError.

This is how timeout used to work in Ruby 2.0. It was changed in Ruby 2.1, after discussion in [Bug #8730] (commit
238c003 in the timeout repository). I think the change from using raise/rescue to using throw/catch has caused significant harm to the Ruby ecosystem at large, and reverting it is the most sensible choice.

From the translation of [Bug #8730], it appears the issue was that someone could rescue Exception and not reraise the exception, causing timeout errors to be swallowed. However, such code is broken anyway. Using throw/catch causes far worse problems, because then it becomes impossible to differentiate between normal control flow and exceptional control flow.

Also related to this is [Bug #11344], which changed how Thread.handle_interrupt interacted with Timeout.

throw/catch is used for non-local control flow, not for exceptional situations.
For exceptional situations, raise should be used instead.  A timeout is an
exceptional situation, so it should use raise, not throw/catch.

Timeout's implementation that uses throw/catch internally causes serious problems.
Consider the following code:

```ruby
def handle_exceptions
  yield
rescue Exception => exc
  handle_error # e.g. ROLLBACK for databases
  raise
ensure
  handle_exit unless exc # e.g. COMMIT for databases
end

Timeout.timeout(1) do
  handle_exceptions do
    do_something
  end
end
```

This kind of design ensures that all exceptions are handled as errors, and
ensures that all exits (normal exit, early return, throw/catch) are not
handled as errors.  With Timeout's throw/catch implementation, this type of
code does not work, since a timeout triggers the normal exit path.

See rails/rails#29333 for an example of the damage
Timeout's design has caused the Rails ecosystem.

This switches Timeout.timeout to use raise/rescue internally.  It adds a
Timeout::ExitException subclass of exception for the internal raise/rescue,
which Timeout.timeout will convert to Timeout::Error for backwards
compatibility.  Timeout::Error remains a subclass of RuntimeError.

This is how timeout used to work in Ruby 2.0.  It was changed in Ruby 2.1,
after discussion in [Bug #8730] (commit
238c003 in the timeout repository). I
think the change from using raise/rescue to using throw/catch has caused
significant harm to the Ruby ecosystem at large, and reverting it is
the most sensible choice.

From the translation of [Bug #8730], it appears the issue was that
someone could rescue Exception and not reraise the exception, causing
timeout errors to be swallowed.  However, such code is broken anyway.
Using throw/catch causes far worse problems, because then it becomes
impossible to differentiate between normal control flow and exceptional
control flow.

Also related to this is [Bug #11344], which changed how
Thread.handle_interrupt interacted with Timeout.
@eregon
Copy link
Member

eregon commented May 24, 2023

Thank you for creating this.
From implementation simplicity and intuition I agree this is better. This is also BTW what JRuby and TruffleRuby's implementations of Timeout did for years, they never did the throw trick until they adopted this gem.
So from that POV it seems to cause little-to-none compatibility issues.

However recently I remembered rails/rails#29333 and IMO it is a good thing that people don't use non-local return and expect that to commit the transaction.
Notably discussed in https://twitter.com/eregontp/status/1658810113287933957
Let's have some examples:

def do_throw
  DB.transaction do
    throw :abort_request # feels like it should NOT commit the transaction isn't it?
  end
end

def do_next
  DB.transaction do
    next # this terminates the block but the caller cannot differentiate with natural return, so this can only commit
  end
end

def do_break
  DB.transaction do
    break # this "breaks" from the transaction, it feels like it should not commit, i.e., it should rollback
  end
end

def do_return
  DB.transaction do
    return if ... # this is probably less clear, but IMO it's really bad style to do this, it is not letting the transaction end normally but instead "escaping" from the transaction, in that POV it should rollback
  end
end

The result of these depend on the implementation of DB.transaction.
When using your snippet above:

class DB
  def self.transaction
    yield
  rescue Exception => exc
    p :ROLLBACK
  ensure
    p :COMMIT unless exc
  end
end

the result is:

:COMMIT
:COMMIT
:COMMIT
:COMMIT

When using this:

class DB
  def self.transaction
    yield
    success = true
  ensure
    if success
      p :COMMIT
    else
      p :ROLLBACK
    end
  end
end

the result is:

:ROLLBACK
:COMMIT
:ROLLBACK
:ROLLBACK

which I think is much better, it commits on next (because that just returns inside the block, not further, it is a local jump/return) but not on throw/break/return, which feels natural, those "escape" the block "abnormally" (non-local jumps/unwinds, like exceptions are too).

Does anyone expect a transaction to be committed if there was a throw in it? I would think not, also one can see throw as just an exception without a backtrace (for faster unwinding).
For example, how Sinatra famously uses throw seems a clear case of "don't commit the transaction, abort everything in this request": https://github.com/sinatra/sinatra/blob/5f4dde19719505989905782a61a19c545df7f9f9/lib/sinatra/base.rb#L1020-L1025

So the current implementation using throw, although to some degree unexpected, has the advantage to "tell" developers to use IMO more correct handling of non-local jumps.

With this PR, it is of course still possible to use the 2nd implementation of DB.transaction which IMO is more correct, so I am not strongly against it.
But it might be tempting to consider return as COMMIT for transactions, and that would then IMO be a mistake because then throw and break would also be COMMIT when they should be ROLLBACK.
IOW I consider rails/rails#29333 a fix to better respect the semantics of throw and break.

cc @ioquatix IIRC you were also discussing this matter on the Ruby bug tracker.

@eregon
Copy link
Member

eregon commented May 24, 2023

Also related to this is [Bug #11344], which changed how Thread.handle_interrupt interacted with Timeout.

Interesting. And I guess Thread.handle_interrupt(Timeout::Error => :never) { prevents the timeout by blocking https://github.com/ruby/timeout/blob/master/lib/timeout.rb#LL86C11-L86C51. With this PR, https://bugs.ruby-lang.org/issues/11344 would regress then, as the modified test shows, that seems potentially problematic for compatibility.

@ioquatix
Copy link
Member

I believe I am in favour of this PR, but I have not reviewed it in detail.

There are some nuances of whether one returns a subclass of Exception or StandardError.

IIRC, we tried both in Celluloid, and found that TimeoutError < Exception was slightly worse in practice than TimeoutError < StandardError, simply because it didn't trigger rescue => error blocks. That being said, I think the argument can be made either way, and users should probably just handle timeouts explicitly.

@headius
Copy link

headius commented May 24, 2023

Timeout has always been a problematic library. It can introduce unexpected exceptions into any code flow at any time and handle_interrupt is insufficient to make it "safe". I would prefer if the library were abolished altogether.

Since that is unlikely to happen, however, I also agree that using throw/catch is not a good choice for the library. When it was introduced into the library, JRuby chose not to follow that path. We only picked it up once we were forced to share the same code in the timeout gem. I would have no objections to returning to a normal exception, and it would not have any effect on how JRuby supports this library.

Copy link

@headius headius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but I'd like to know that we've tested this against some top-10 gems.

@eregon
Copy link
Member

eregon commented May 24, 2023

@jeremyevans @ioquatix @headius and others: I'd like to get your opinions on this part of my (long) comment above:

Does anyone expect a transaction to be committed if there was a throw in it? I would think not, also one can see throw as just an exception without a backtrace (for faster unwinding).
For example, how Sinatra famously uses throw seems a clear case of "don't commit the transaction, abort everything in this request": https://github.com/sinatra/sinatra/blob/5f4dde19719505989905782a61a19c545df7f9f9/lib/sinatra/base.rb#L1020-L1025

If we agree throw should ROLLBACK and not COMMIT, then I think there is probably no actual need to change anything here. Which doesn't mean we shouldn't change it for other reasons like simplicity or being more intuitive, but still the key reasons for throw instead of raise is IMO correct handling of non-local jumps and avoiding that it can be caught accidentally.

@headius
Copy link

headius commented May 24, 2023

I'd like to get your opinions on this part of my (long) comment

The question is not whether throw should trigger stack-unrolling cleanup operations in the same way as exceptions do. If a user designs a library using throw they are explicitly choosing to branch from a deep stack to some higher frame without traversing intermediate rescue operations, and in that case, the answer would be "no", because they are opting into such behavior.

The problem here is that the cross-thread exception raising mechanism is being subverted to introduce throw into call stacks that were not designed to handle them. It's worse than simply raising an exception in another thread; it is forcibly unrolling that thread's stack without giving the library author any say in the matter and with no way to deal with that unrolling in a safe way.

So the answer to your question is that throw is intended to unroll the stack in this way, but generally should be used within a single library and not across library boundaries, since it forces that unrolling behavior on code that may not have been designed to handle it. The current implementation of timeout breaks that rule for every library in existence since it can introduce a throw anywhere, at any time, and there's nothing you can do about it.

@eregon
Copy link
Member

eregon commented May 24, 2023

@headius I'm not sure I get your point, both raise and throw execute ensure clauses, there is no difference there.
The argument about not designed to handle them also applies to non-local return and break, yet we rarely ever see anyone complaining about those semantics. So I think code (in general) doesn't need to be designed particularly to handle non-local jumps. I think very few places need to care about non-local jumps.

My question is given a throw by the user or a gem (and not by Timeout.timeout) inside a DB transaction, should that COMMIT or ROLLBACK the transaction? What about break and return?

@jeremyevans
Copy link
Contributor Author

However recently I remembered rails/rails#29333 and IMO it is a good thing that people don't use non-local return and expect that to commit the transaction. Notably discussed in https://twitter.com/eregontp/status/1658810113287933957 Let's have some examples:

def do_throw
  DB.transaction do
    throw :abort_request # feels like it should NOT commit the transaction isn't it?
  end
end

I guess with your choice of symbol, yes. However, consider:

def do_throw
  DB.transaction do
    throw :commit_request # feels like it should commit the transaction, right?
  end
end

This is equally valid. In general, throw/catch is always used for expected control flow, and not for exceptional control flow. For one, using throw without a matching catch raises an UncaughtThrowError exception.

def do_next
DB.transaction do
next # this terminates the block but the caller cannot differentiate with natural return, so this can only commit
end
end

I would expect this to commit.

def do_break
DB.transaction do
break # this "breaks" from the transaction, it feels like it should not commit, i.e., it should rollback
end
end

I would expect this to commit, as break/next is used for expected control flow, not for exceptional control flow.

def do_return
DB.transaction do
return if ... # this is probably less clear, but IMO it's really bad style to do this, it is not letting the transaction end normally but instead "escaping" from the transaction, in that POV it should rollback
end
end

I would expect this to commit. I don't consider this bad style. It's quite common to use early returns, and again, they are used for expected control flow, not for exceptional control flow.

The result of these depend on the implementation of DB.transaction. When using your snippet above:

class DB
  def self.transaction
    yield
  rescue Exception => exc
    p :ROLLBACK
  ensure
    p :COMMIT unless exc
  end
end

the result is:

:COMMIT
:COMMIT
:COMMIT
:COMMIT

When using this:

class DB
  def self.transaction
    yield
    success = true
  ensure
    if success
      p :COMMIT
    else
      p :ROLLBACK
    end
  end
end

the result is:

:ROLLBACK
:COMMIT
:ROLLBACK
:ROLLBACK

which I think is much better, it commits on next (because that just returns inside the block, not further, it is a local jump/return) but not on throw/break/return, which feels natural, those "escape" the block "abnormally" (non-local jumps/unwinds, like exceptions are too).

I think rolling back in any of these examples is a bad idea. Only raise (exceptional control flow) should rollback. All other control flow is expected control flow and should commit by default.

Does anyone expect a transaction to be committed if there was a throw in it?

I expect transactions to be committed on throw (that is how Sequel works). throw/catch is for non-exceptional, non-local control flow, same as early return/next. When using Sinatra/Roda/Cuba/Syro and other frameworks, throw/catch is used to immediately return responses anytime during request processing. I believe all of these frameworks use throw/catch to handle responses for all requests, not just for some requests. For Sinatra, see https://github.com/sinatra/sinatra/blob/5f4dde19719505989905782a61a19c545df7f9f9/lib/sinatra/base.rb#L1058-L1086 .

I would think not, also one can see throw as just an exception without a backtrace (for faster unwinding). For example, how Sinatra famously uses throw seems a clear case of "don't commit the transaction, abort everything in this request": https://github.com/sinatra/sinatra/blob/5f4dde19719505989905782a61a19c545df7f9f9/lib/sinatra/base.rb#L1020-L1025

I think you are incorrect here. How much have you developed web applications in Sinatra? I used Sinatra as my primary web framework for over 5 years, and it's standard in Sinatra to use halt/redirect for successful conditions (also possible to use next for early returns as well). Example:

post '/' do
  DB.transaction do
    case params['foo']
    when 'bar-baz'
       Bar[1].update(...)
       Baz[2].update(...)
       redirect '/bar' # uses throw
    when 'baz-bar'
       Baz[1].update(...)
       Bar[2].update(...)
       redirect '/baz' # uses throw
    end
  end
end

So the current implementation using throw, although to some degree unexpected, has the advantage to "tell" developers to use IMO more correct handling of non-local jumps.

I disagree that your examples here are more correct, so I disagree that this is an advantage of timeout's current implementation.

With this PR, it is of course still possible to use the 2nd implementation of DB.transaction which IMO is more correct, so I am not strongly against it. But it might be tempting to consider return as COMMIT for transactions, and that would then IMO be a mistake because then throw and break would also be COMMIT when they should be ROLLBACK. IOW I consider rails/rails#29333 a fix to better respect the semantics of throw and break.

I disagree. Only exceptional control flow should rollback automatically. All expected control flow should not.

Regardless of your opinion on the implementation of a transaction method, I hope we can agree that a timeout error is exceptional control flow and not expected control flow, and should therefore use raise and not throw.

@headius
Copy link

headius commented May 24, 2023

I hope we can agree that a timeout error is exceptional control flow and not expected control flow, and should therefore use raise and not throw.

I agree. That's why we never modified JRuby's version of timeout to use throw/catch.

@eregon
Copy link
Member

eregon commented May 24, 2023

@jeremyevans Thank you for your reply. I like this expected control flow vs exceptional control flow distinction. I think I have not heard it before, do you know if there is any documentation/blog post on the subject? I suppose I'm not the only one to not already know this distinction. It would be nice to have this documented somewhere in docs for Ruby.

I used Sinatra as my primary web framework for over 5 years, and it's standard in Sinatra to use halt/redirect for successful conditions (also possible to use next for early returns as well).

Thanks for clarifying that. I have used Sinatra several times but not daily. I just thought throw :halt is meant to be only for error responses (400-599), but it seems it's not. halt does seem to mean in general error or abrupt stop, not "success, here is the page body", so I was misled by the word used. (also design and performance-wise doing a throw even on success seems pretty strange to me)

I think break is still rather ambiguous if done inside a DB transaction by a user, breaking out of the transaction block doesn't feel like that intends to commit it to me. But I can see people seeing it the other way.


I think I'm OK with the change, my main concern is the compatibility issue with #30 (comment).
Any code that does Thread.handle_interrupt(Timeout::Error will be broken.
Luckily it seems pretty rare: https://github.com/search?q=Thread.handle_interrupt%28Timeout%3A%3AError+language%3ARuby&type=code&l=Ruby&p=1
It would be good to do a gem code search too with that to make sure it's really that rare.

To be fair Thread.handle_interrupt(Timeout::Error is pretty strange, if one doesn't want unexpected interrupts they should use Exception or Object, not Timeout::Error (so e.g. it also prevents other interrupts like Thread#raise, etc).

@eregon
Copy link
Member

eregon commented May 24, 2023

Luckily it seems pretty rare: https://github.com/search?q=Thread.handle_interrupt%28Timeout%3A%3AError+language%3ARuby&type=code&l=Ruby&p=1

In non-test usages I have found:

@headius
Copy link

headius commented May 24, 2023

Any code that does Thread.handle_interrupt(Timeout::Error will be broken.

Personally I find it amusing that while we're talking about a library that violently interrupts the execution of user code there's concerns about that user code preventing us from doing so. It seems to me that what's good for the goose is good for the gander: if a user or a library author decides that intercepting timeout errors is the right thing to do, that's no more egregious a decision than us throwing unexpected exceptions at them.

To be sure, making it possible to gracefully handle (or ignore) timeout errors is the essence of Ruby; part of my issue with throw/catch was that there's no way for users to enlist in the process and handle it their own way.

Perhaps there will be poorly-behaved libraries that defeat timeout altogether. That's their prerogative, and if it affects users they will not use said library. But it's not the Ruby way for us to force this flow interruption onto code and also prevent them from overriding it.

@jeremyevans
Copy link
Contributor Author

@jeremyevans Thank you for your reply. I like this expected control flow vs exceptional control flow distinction. I think I have not heard it before, do you know if there is any documentation/blog post on the subject? I suppose I'm not the only one to not already know this distinction. It would be nice to have this documented somewhere in docs for Ruby.

I agree. I looked in doc in the ruby repository, and interestingly enough, there is no documentation on throw/catch there. Maybe because they are implemented as methods and not keywords. I'll work on a pull request to update /doc/syntax/control_expressions.rdoc to include a discussion of throw/catch.

I used Sinatra as my primary web framework for over 5 years, and it's standard in Sinatra to use halt/redirect for successful conditions (also possible to use next for early returns as well).

Thanks for clarifying that. I have used Sinatra several times but not daily. I just thought throw :halt is meant to be only for error responses (400-599), but it seems it's not. halt does seem to mean in general error or abrupt stop, not "success, here is the page body", so I was misled by the word used. (also design and performance-wise doing a throw even on success seems pretty strange to me)

Using throw for all responses seems odd until you get used to it, similar to how dynamic typing seems odd if you are only used to static typing. However, once you get used to it, I think you'll find that using throw for all responses allows for simpler code in the long run, and is one of the reasons that Sinatra/Roda are simpler to use than frameworks that don't throw/catch for responses.

I think I'm OK with the change, my main concern is the compatibility issue with #30 (comment). Any code that does Thread.handle_interrupt(Timeout::Error will be broken. Luckily it seems pretty rare: https://github.com/search?q=Thread.handle_interrupt%28Timeout%3A%3AError+language%3ARuby&type=code&l=Ruby&p=1 It would be good to do a gem code search too with that to make sure it's really that rare.

Since it looks like there isn't that many libraries affected by this change, I can notify both projects if this pull request is accepted, and work with them to get their code updated before Ruby 3.3 is released.

@eregon
Copy link
Member

eregon commented May 24, 2023

Personally I find it amusing that while we're talking about a library that violently interrupts the execution of user code there's concerns about that user code preventing us from doing so.

Thread.handle_interrupt(Timeout::Error => :never) { does work (even though it is indeed surprising) with the current version of timeout using throw, in that it will prevent the error and the throw to happen in that block.
It will not have any effect anymore with this PR.

Also this is officially documented in https://github.com/ruby/ruby/blob/31b28b31fa5a0452cb9d5f7eee88eebfebe5b4d1/thread.c#L2067-L2089, these docs will need to be adapted as well.

lib/timeout.rb Outdated
yield exc
rescue ExitException => e
raise new(message) unless exc.equal?(e)
super
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does super do here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be fixed. It should raise instead. I'll push a fix. This is necessary for some nested timeouts, so only convert from Timeout::ExitException to Timeout::Error for the matching Timeout.timeout call.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, could you add a test for this? I assume there isn't one since it would have failed with the super.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I added a nested timeouts test which handles this case.

lib/timeout.rb Outdated
Comment on lines 30 to 31
# Return the receiver if it the exception is triggered in
# the same thread.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this or could we just assume the ExitException we rescue is the one we raised? Maybe it's necessary to handle the case of nested timeouts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs say:

=== Implementation from Exception
------------------------------------------------------------------------
  exc.exception([string])  ->  an_exception or exc

------------------------------------------------------------------------

With no argument, or if the argument is the same as the receiver, return
the receiver. Otherwise, create a new exception object of the same class
as the receiver, but with a message equal to string.to_str.

so I think we don't need to do anything special here (it should already return the receiver).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out the #exception define is needed, because the interrupt method does @thread.raise @exception_class, @message. Without the @message, it wouldn't be needed. However, the @message is needed in the cases where you are providing a klass argument to timeout.

@eregon
Copy link
Member

eregon commented May 24, 2023

Here is gem-codesearch:

$ gem-codesearch 'Thread\.handle_interrupt\(Timeout::Error'
2022-04-14 /srv/gems/libhoney-2.2.0/lib/libhoney/transmission.rb:          Thread.handle_interrupt(Timeout::Error => :on_blocking) do
2013-07-24 /srv/gems/pollter_geist-0.0.2/lib/pollter_geist/imap_idler.rb:      Thread.handle_interrupt(Timeout::Error => :never, Interrupt => :never) do
2013-07-24 /srv/gems/pollter_geist-0.0.2/lib/pollter_geist/imap_idler.rb:              Thread.handle_interrupt(Timeout::Error => :immediate, Interrupt => :on_blocking) {
2023-04-26 /srv/gems/rbs-3.1.0/core/thread.rbs:  #     Thread.handle_interrupt(Timeout::Error => :never) {
2023-04-26 /srv/gems/rbs-3.1.0/core/thread.rbs:  #         Thread.handle_interrupt(Timeout::Error => :on_blocking) {
2022-07-20 /srv/gems/rhodes-7.5.1/platform/shared/ruby/thread.c: *   Thread.handle_interrupt(Timeout::Error => :never) {
2022-07-20 /srv/gems/rhodes-7.5.1/platform/shared/ruby/thread.c: *       Thread.handle_interrupt(Timeout::Error => :on_blocking) {
2016-12-29 /srv/gems/ruby-compiler-0.1.1/vendor/ruby/test/test_timeout.rb:      Thread.handle_interrupt(Timeout::Error => :never) {
2016-12-29 /srv/gems/ruby-compiler-0.1.1/vendor/ruby/test/test_timeout.rb:          Thread.handle_interrupt(Timeout::Error => :on_blocking) {
2016-12-29 /srv/gems/ruby-compiler-0.1.1/vendor/ruby/test/webrick/test_utils.rb:    Thread.handle_interrupt(Timeout::Error => :never, EX => :never) do
2016-12-29 /srv/gems/ruby-compiler-0.1.1/vendor/ruby/test/webrick/test_utils.rb:    Thread.handle_interrupt(Timeout::Error => :never, EX => :never) do
2016-12-29 /srv/gems/ruby-compiler-0.1.1/vendor/ruby/thread.c: *   Thread.handle_interrupt(Timeout::Error => :never) {
2016-12-29 /srv/gems/ruby-compiler-0.1.1/vendor/ruby/thread.c: *       Thread.handle_interrupt(Timeout::Error => :on_blocking) {

So libhoney and pollter_geist seem potentially affected, the rest is just vendored CRuby code/docs.

ExitException#exception needs to always return self, so the same
exception object will be used.  The previous code never used the
same exception object, but the conditional was inverted from what
it should have been, so all existing tests passed.  There wasn't
a test for nested timeouts, so I added one.

Remove the @thread instance variable setting.  When using
Thread#raise, it is expected that the Thread that raises the
exception will be different than the thread that created the
exception.
@ioquatix
Copy link
Member

ioquatix commented May 25, 2023

I discussed this PR with Jeremy.

If we accept this PR, I will follow up with a 2nd PR to align the fiber scheduler implementation logic more closely with this PR (so please don't do an immediate release after merging this PR). As it stands:

  • The current behaviour (throw/catch) is completely different to scheduler.timeout_after.
  • This PR aligns the behaviour very closely but not exactly the same as scheduler.timeout_after.
  • I accept this PR is a good compromise and if accepted, I'll align the scheduler.timeout_after path with the behaviour of this PR.

Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with this but I would like @nobu's review as well.

Comment on lines +156 to +160
Thread.handle_interrupt(Timeout::ExitException => :never) {
Timeout.timeout(0.01) {
sleep 0.2
ok = true
Thread.handle_interrupt(Timeout::Error => :on_blocking) {
Thread.handle_interrupt(Timeout::ExitException => :on_blocking) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to add a new public interface Timeout::ExitException.
Does it need to be public?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does if we want to allow users to use Thread.handle_interrupt with it. From @eregon's research, there are only 2 gems actually doing that, and whether it is actually needed is not known. If we decide we don't want to support that, we could make ExitException a private constant.

@jjb
Copy link
Contributor

jjb commented Jun 30, 2023

I have a test suite I'm getting ready to submit as a PR. It's not ready yet, but I wanted to use it to flag a behavior change from this PR, just in case it's interesting / wasn't intended.

For the scenario when an exception to raise is not specified and the inner code does catch Exception, I've marked in comments here what changed between 0.2 and 0.4 (the two i tested)

assert s.inner_rescue # true in 1.9, false in gem 0.2.0, true in 0.4.0
# BAD?
assert !s.outer_rescue # false in 1.9 stdlib, true in gem 0.2.0, false in 0.4.0

You can see inner_rescue and outer_rescue referred here

@inner_rescue = true
else
@inner_else = true
ensure
@inner_ensure = true
t = Time.now; nil while Time.now < t+1
@inner_ensure_has_time_to_finish = true
end
}
rescue Exception
@outer_rescue = true

I believe the change to the inner behavior is the purpose of this PR (30).

I'm not sure if the outer behavior change was intended.

@jeremyevans
Copy link
Contributor Author

I'm not sure if the outer behavior change was intended.

If you rescue Exception, catch the internal timeout exception, and don't reraise the exception, it is expected that the change in this PR will result in the exception being swallowed and the external timeout exception not being raised. There's no way to avoid that, any such code that uses rescue Exception and doesn't reraise should be considered broken.

@jjb
Copy link
Contributor

jjb commented Jul 1, 2023

gotcha, thanks for the explanation!

@jjb
Copy link
Contributor

jjb commented Jul 1, 2023

couple questions if you don't mind:

There's no way to avoid that

Does this mean, there's no way to implement this behavior in timeout, but it is theoretically desired? Or, given other semantic constraints, this behavior is an implication.

any such code that uses rescue Exception and doesn't reraise should be considered broken.

When you say "any such code", are you referring to client code using timeout (it's that code's responsibility to know it's in a timeout block) or are you referring to a potential timeout modification which would change the behavior?

@jeremyevans
Copy link
Contributor Author

There's no way to avoid that

Does this mean, there's no way to implement this behavior in timeout, but it is theoretically desired? Or, given other semantic constraints, this behavior is an implication.

It's not desired. Timeout errors must result in related rescue Exception blocks being called. If those blocks do not reraise, that is a bug in that code.

The rescue Exception issue is the reason timeout was originally switched to use throw, but since the use of throw causes far more problems than it solves, this PR reverts the behavior.

any such code that uses rescue Exception and doesn't reraise should be considered broken.

When you say "any such code", are you referring to client code using timeout (it's that code's responsibility to know it's in a timeout block) or are you referring to a potential timeout modification which would change the behavior?

I'm referring to any code that uses rescue Exception and doesn't reraise. The only reasonable exception would be when all code calling into a block using rescue Exception is controlled by you. For example, a top level exception handler in a server-process such as puma to prevent the entire process from crashing. In any case where your code could be called by code you don't control, you should not be rescuing Exception without reraising.

@jjb
Copy link
Contributor

jjb commented Jul 1, 2023

It's not desired. Timeout errors must result in related rescue Exception blocks being called. If those blocks do not reraise, that is a bug in that code.

So I think you are saying that it's not desired because the only way to do it is to defeat inner code behavior, and doing so is not acceptable, so therefore attempting this behavior (outer code always gets an exception when time has lapsed) is not desired. the prerequisite is not desired.

I think I did achieve both things in a older implementation of timeout here: jjb/better_timeout@a5282a2

And the current implementation of timeout here: https://github.com/jjb/timeout/pull/4/files

Maybe now is not the time to dive into the pros/cons of that approach, but I just wanted to call it out while fresh on our minds. I want to get feedback on that approach eventually, but first waiting on possible merge of my tests-only PR #33

@jeremyevans
Copy link
Contributor Author

It's not desired. Timeout errors must result in related rescue Exception blocks being called. If those blocks do not reraise, that is a bug in that code.

So I think you are saying that it's not desired because the only way to do it is to defeat inner code behavior, and doing so is not acceptable, so therefore attempting this behavior (outer code always gets an exception when time has lapsed) is not desired. the prerequisite is not desired.

I think I did achieve both things in a older implementation of timeout here: jjb/better_timeout@a5282a2

Assuming I'm reading this correctly (jjb/better_timeout@a5282a2#diff-5fa5bc762e29d9e8a89e396dff49339c77d3ed55d4ac349e1066f324b24dce71R59), your code has #timeout yield in a separate thread (unless a nil/0 timeout is passed, in which case it yields in the current thread), which would not be acceptable as it would break code that depends on the current thread (such as thread-based connection pools by common database libraries).

@jjb
Copy link
Contributor

jjb commented Jul 1, 2023

Thanks for the explanation of that requirement!

Do you know if that requirement is currently represented in a test in the current test suite? I would love to help writing one if not.

"depends on the current thread" - depends on what about the current thread? it continuing?

@jeremyevans
Copy link
Contributor Author

Do you know if that requirement is currently represented in a test in the current test suite? I would love to help writing one if not.

Not sure, but a test would be:

  assert_equal Thread.current, Timeout.timeout(10){Thread.current}

"depends on the current thread" - depends on what about the current thread? it continuing?

Depends on the current thread inside the timeout block being the same as the current thread outside the timeout block.

jjb added a commit to jjb/timeout that referenced this pull request Jul 2, 2023
@jjb
Copy link
Contributor

jjb commented Jul 2, 2023

Great - made a PR if interested #34

eregon pushed a commit that referenced this pull request Jul 3, 2023
matzbot pushed a commit to ruby/ruby that referenced this pull request Jul 3, 2023
@eregon eregon mentioned this pull request Jul 3, 2023
casperisfine pushed a commit to Shopify/rails that referenced this pull request Jul 10, 2023
Fix: rails#45017
Ref: rails#29333
Ref: ruby/timeout#30

Historically only raised errors would trigger a rollback, but in Ruby `2.3`, the `timeout` library
started using `throw` to interupt execution which had the adverse effect of committing open transactions.

To solve this, in Active Record 6.1 the behavior was changed to instead rollback the transaction as it was safer
than to potentially commit an incomplete transaction.

Using `return`, `break` or `throw` inside a `transaction` block was essentially deprecated from Rails 6.1 onwards.

However with the release of `timeout 0.4.0`, `Timeout.timeout` now raises an error again, and Active Record is able
to return to its original, less surprising, behavior.
jpcamara added a commit to jpcamara/ruby that referenced this pull request Aug 27, 2024
* This PR from the timeout gem (ruby/timeout#30) made it so you have to handle_interrupt on Timeout::ExitException instead of Timeout::Error

* Top-level #timeout was removed from the timeout gem 5 years ago - it needs to be Timeout.timeout
jpcamara added a commit to jpcamara/ruby that referenced this pull request Aug 29, 2024
* This PR from the timeout gem (ruby/timeout#30) made it so you have to handle_interrupt on Timeout::ExitException instead of Timeout::Error

* Efficiency changes to the gem (one shared thread) mean you can't consistently handle timeout errors using handle_timeout: ruby/timeout#41
jpcamara added a commit to jpcamara/ruby that referenced this pull request Aug 29, 2024
* This PR from the timeout gem (ruby/timeout#30) made it so you have to handle_interrupt on Timeout::ExitException instead of Timeout::Error

* Efficiency changes to the gem (one shared thread) mean you can't consistently handle timeout errors using handle_timeout: ruby/timeout#41
jpcamara added a commit to jpcamara/ruby that referenced this pull request Aug 29, 2024
* This PR from the timeout gem (ruby/timeout#30) made it so you have to handle_interrupt on Timeout::ExitException instead of Timeout::Error

* Efficiency changes to the gem (one shared thread) mean you can't consistently handle timeout errors using handle_timeout: ruby/timeout#41
hsbt pushed a commit to ruby/ruby that referenced this pull request Sep 9, 2024
* This PR from the timeout gem (ruby/timeout#30) made it so you have to handle_interrupt on Timeout::ExitException instead of Timeout::Error

* Efficiency changes to the gem (one shared thread) mean you can't consistently handle timeout errors using handle_timeout: ruby/timeout#41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants