[ruby/timeout] Raise exception instead of throw/catch for timeouts

(https://github.com/ruby/timeout/pull/30)

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 https://github.com/rails/rails/pull/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
https://github.com/ruby/timeout/commit/238c003c921e 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.

https://github.com/ruby/timeout/commit/f16545abe6

Co-authored-by: Nobuyoshi Nakada <[email protected]>

1 files changed, 15 insertions, 19 deletions

diff --git a/lib/timeout.rb b/lib/timeout.rb
index e1107765ab..49217d5b24 100644
--- a/lib/timeout.rb
+++ b/lib/timeout.rb
@@ -25,27 +25,24 @@
 module Timeout
   VERSION = "0.3.2"
 
+  # Internal error raised to when a timeout is triggered.
+  class ExitException < Exception
+    def exception(*)
+      self
+    end
+  end
+
   # Raised by Timeout.timeout when the block times out.
   class Error < RuntimeError
-    attr_reader :thread
+    def self.handle_timeout(message)
+      exc = ExitException.new(message)
 
-    def self.catch(*args)
-      exc = new(*args)
-      exc.instance_variable_set(:@thread, Thread.current)
-      exc.instance_variable_set(:@catch_value, exc)
-      ::Kernel.catch(exc) {yield exc}
-    end
-
-    def exception(*)
-      # TODO: use Fiber.current to see if self can be thrown
-      if self.thread == Thread.current
-        bt = caller
-        begin
-          throw(@catch_value, bt)
-        rescue UncaughtThrowError
-        end
+      begin
+        yield exc
+      rescue ExitException => e
+        raise new(message) if exc.equal?(e)
+        raise
       end
-      super
     end
   end
 
@@ -195,8 +192,7 @@ module Timeout
     if klass
       perform.call(klass)
     else
-      backtrace = Error.catch(&perform)
-      raise Error, message, backtrace
+      Error.handle_timeout(message, &perform)
     end
   end
   module_function :timeout