From cf30b51cd87de1deb40ae0ad994445c6d9644cfa Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Tue, 23 May 2023 19:38:03 -0700 Subject: [PATCH 1/5] Raise exception instead of throw/catch for timeouts 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 238c003c921e6e555760f8e96968562a622a99c4 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. --- lib/timeout.rb | 39 +++++++++++++++++++++------------------ test/test_timeout.rb | 27 ++++++++++++++++++++------- 2 files changed, 41 insertions(+), 25 deletions(-) diff --git a/lib/timeout.rb b/lib/timeout.rb index e110776..b692734 100644 --- a/lib/timeout.rb +++ b/lib/timeout.rb @@ -25,27 +25,31 @@ module Timeout VERSION = "0.3.2" + # Internal error raised to when a timeout is triggered. + class ExitException < Exception + # Return the receiver if it the exception is triggered in + # the same thread. + def exception(*a) + if @thread == Thread.current + self + else + super + end + end + end + # Raised by Timeout.timeout when the block times out. class Error < RuntimeError - attr_reader :thread - - def self.catch(*args) - exc = new(*args) + def self.handle_timeout(message) + exc = ExitException.new(message) 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) unless exc.equal?(e) + super end - super end end @@ -195,8 +199,7 @@ def timeout(sec, klass = nil, message = nil, &block) #:yield: +sec+ if klass perform.call(klass) else - backtrace = Error.catch(&perform) - raise Error, message, backtrace + Error.handle_timeout(message, &perform) end end module_function :timeout diff --git a/test/test_timeout.rb b/test/test_timeout.rb index c3349d0..09c963a 100644 --- a/test/test_timeout.rb +++ b/test/test_timeout.rb @@ -47,19 +47,32 @@ def (n = Object.new).zero?; false; end assert_raise(TypeError, bug3168) {Timeout.timeout(n) { sleep 0.1 }} end - def test_skip_rescue - bug8730 = '[Bug #8730]' + def test_skip_rescue_standarderror e = nil - assert_raise_with_message(Timeout::Error, /execution expired/, bug8730) do + assert_raise_with_message(Timeout::Error, /execution expired/) do Timeout.timeout 0.01 do begin sleep 3 - rescue Exception => e + rescue flunk "should not see any exception but saw #{e.inspect}" end end end - assert_nil(e, bug8730) + end + + def test_raises_exception_internally + e = nil + assert_raise_with_message(Timeout::Error, /execution expired/) do + Timeout.timeout 0.01 do + begin + sleep 3 + rescue Exception => exc + e = exc + raise + end + end + end + assert_equal Timeout::ExitException, e.class end def test_rescue_exit @@ -127,11 +140,11 @@ def test_handle_interrupt bug11344 = '[ruby-dev:49179] [Bug #11344]' ok = false assert_raise(Timeout::Error) { - Thread.handle_interrupt(Timeout::Error => :never) { + 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) { sleep 0.2 } } From 8e4a135b98a99af6c24e3a8e39f989d9c73fbcc0 Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Wed, 24 May 2023 10:31:09 -0700 Subject: [PATCH 2/5] Fix handling of nested timeouts --- lib/timeout.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/timeout.rb b/lib/timeout.rb index b692734..84fcccd 100644 --- a/lib/timeout.rb +++ b/lib/timeout.rb @@ -48,7 +48,7 @@ def self.handle_timeout(message) yield exc rescue ExitException => e raise new(message) unless exc.equal?(e) - super + raise end end end From 2343e39b0870bf65ffe8f9d4ad8f9633232fced1 Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Wed, 24 May 2023 10:32:06 -0700 Subject: [PATCH 3/5] Remove unneeded ExitException#exception --- lib/timeout.rb | 9 --------- 1 file changed, 9 deletions(-) diff --git a/lib/timeout.rb b/lib/timeout.rb index 84fcccd..9a251dd 100644 --- a/lib/timeout.rb +++ b/lib/timeout.rb @@ -27,15 +27,6 @@ module Timeout # Internal error raised to when a timeout is triggered. class ExitException < Exception - # Return the receiver if it the exception is triggered in - # the same thread. - def exception(*a) - if @thread == Thread.current - self - else - super - end - end end # Raised by Timeout.timeout when the block times out. From 55c1339545b5c599988a79c71761f5f3f0e351c4 Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Wed, 24 May 2023 10:42:59 -0700 Subject: [PATCH 4/5] Fix nested timeouts 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. --- lib/timeout.rb | 6 ++++-- test/test_timeout.rb | 13 +++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/lib/timeout.rb b/lib/timeout.rb index 9a251dd..49217d5 100644 --- a/lib/timeout.rb +++ b/lib/timeout.rb @@ -27,18 +27,20 @@ module Timeout # 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 def self.handle_timeout(message) exc = ExitException.new(message) - exc.instance_variable_set(:@thread, Thread.current) begin yield exc rescue ExitException => e - raise new(message) unless exc.equal?(e) + raise new(message) if exc.equal?(e) raise end end diff --git a/test/test_timeout.rb b/test/test_timeout.rb index 09c963a..28b4da3 100644 --- a/test/test_timeout.rb +++ b/test/test_timeout.rb @@ -41,6 +41,19 @@ def test_timeout end end + def test_nested_timeout + a = nil + assert_raise(Timeout::Error) do + Timeout.timeout(0.1) { + Timeout.timeout(1) { + nil while true + } + a = 1 + } + end + assert_nil a + end + def test_cannot_convert_into_time_interval bug3168 = '[ruby-dev:41010]' def (n = Object.new).zero?; false; end From cd050d2e0e53fc968f6846d2f734bf473176cb54 Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Fri, 26 May 2023 09:52:29 -0700 Subject: [PATCH 5/5] Update test/test_timeout.rb Co-authored-by: Nobuyoshi Nakada --- test/test_timeout.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_timeout.rb b/test/test_timeout.rb index 28b4da3..236883a 100644 --- a/test/test_timeout.rb +++ b/test/test_timeout.rb @@ -66,7 +66,7 @@ def test_skip_rescue_standarderror Timeout.timeout 0.01 do begin sleep 3 - rescue + rescue => e flunk "should not see any exception but saw #{e.inspect}" end end