From 35c02abd4050463ef7912c38b1f7d33c92b15b1b Mon Sep 17 00:00:00 2001 From: Benoit Daloze Date: Thu, 11 Dec 2025 11:20:43 +0100 Subject: [PATCH 1/2] Reset the interrupt mask when creating the Timeout thread * Add tests related to Thread.handle_interrupt * Fixes https://github.com/ruby/timeout/issues/41 --- lib/timeout.rb | 50 ++++++++++--------- test/test_timeout.rb | 113 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 140 insertions(+), 23 deletions(-) diff --git a/lib/timeout.rb b/lib/timeout.rb index 1640542..0260fd1 100644 --- a/lib/timeout.rb +++ b/lib/timeout.rb @@ -83,36 +83,40 @@ def self.instance end def create_timeout_thread - watcher = Thread.new do - requests = [] - while true - until @queue.empty? and !requests.empty? # wait to have at least one request - req = @queue.pop - requests << req unless req.done? - end - closest_deadline = requests.min_by(&:deadline).deadline + # Threads unexpectedly inherit the interrupt mask: https://github.com/ruby/timeout/issues/41 + # So reset the interrupt mask to the default one for the timeout thread + Thread.handle_interrupt(Object => :immediate) do + watcher = Thread.new do + requests = [] + while true + until @queue.empty? and !requests.empty? # wait to have at least one request + req = @queue.pop + requests << req unless req.done? + end + closest_deadline = requests.min_by(&:deadline).deadline - now = 0.0 - @queue_mutex.synchronize do - while (now = GET_TIME.call(Process::CLOCK_MONOTONIC)) < closest_deadline and @queue.empty? - @condvar.wait(@queue_mutex, closest_deadline - now) + now = 0.0 + @queue_mutex.synchronize do + while (now = GET_TIME.call(Process::CLOCK_MONOTONIC)) < closest_deadline and @queue.empty? + @condvar.wait(@queue_mutex, closest_deadline - now) + end end - end - requests.each do |req| - req.interrupt if req.expired?(now) + requests.each do |req| + req.interrupt if req.expired?(now) + end + requests.reject!(&:done?) end - requests.reject!(&:done?) end - end - if !watcher.group.enclosed? && (!defined?(Ractor.main?) || Ractor.main?) - ThreadGroup::Default.add(watcher) - end + if !watcher.group.enclosed? && (!defined?(Ractor.main?) || Ractor.main?) + ThreadGroup::Default.add(watcher) + end - watcher.name = "Timeout stdlib thread" - watcher.thread_variable_set(:"\0__detached_thread__", true) - watcher + watcher.name = "Timeout stdlib thread" + watcher.thread_variable_set(:"\0__detached_thread__", true) + watcher + end end def ensure_timeout_thread_created diff --git a/test/test_timeout.rb b/test/test_timeout.rb index 51666b7..01beadb 100644 --- a/test/test_timeout.rb +++ b/test/test_timeout.rb @@ -4,6 +4,12 @@ class TestTimeout < Test::Unit::TestCase + private def kill_timeout_thread + thread = Timeout.const_get(:State).instance.instance_variable_get(:@timeout_thread) + thread.kill + thread.join + end + def test_public_methods assert_equal [:timeout], Timeout.private_instance_methods(false) assert_equal [], Timeout.public_instance_methods(false) @@ -221,6 +227,24 @@ def o.each end end + def test_handle_interrupt_with_exception_class + bug11344 = '[ruby-dev:49179] [Bug #11344]' + ok = false + assert_raise(Timeout::Error) { + Thread.handle_interrupt(Timeout::Error => :never) { + Timeout.timeout(0.01, Timeout::Error) { + sleep 0.2 + ok = true + Thread.handle_interrupt(Timeout::Error => :on_blocking) { + sleep 0.2 + raise "unreachable" + } + } + } + } + assert(ok, bug11344) + end + def test_handle_interrupt bug11344 = '[ruby-dev:49179] [Bug #11344]' ok = false @@ -231,6 +255,7 @@ def test_handle_interrupt ok = true Thread.handle_interrupt(Timeout::ExitException => :on_blocking) { sleep 0.2 + raise "unreachable" } } } @@ -238,6 +263,94 @@ def test_handle_interrupt assert(ok, bug11344) end + def test_handle_interrupt_with_interrupt_mask_inheritance + issue = 'https://github.com/ruby/timeout/issues/41' + + [ + -> {}, # not blocking so no opportunity to interrupt + -> { sleep 5 } + ].each_with_index do |body, idx| + # We need to create a new Timeout thread + kill_timeout_thread + + # Create the timeout thread under a handle_interrupt(:never) + # due to the interrupt mask being inherited + Thread.handle_interrupt(Object => :never) { + assert_equal :ok, Timeout.timeout(1) { :ok } + } + + # Ensure a simple timeout works and the interrupt mask was not inherited + assert_raise(Timeout::Error) { + Timeout.timeout(0.001) { sleep 1 } + } + + r = [] + # This raises Timeout::ExitException and not Timeout::Error for the non-blocking body + # because of the handle_interrupt(:never) which delays raising Timeout::ExitException + # on the main thread until getting outside of that handle_interrupt(:never) call. + # For this reason we document handle_interrupt(Timeout::ExitException) should not be used. + exc = idx == 0 ? Timeout::ExitException : Timeout::Error + assert_raise(exc) { + Thread.handle_interrupt(Timeout::ExitException => :never) { + Timeout.timeout(0.1) do + sleep 0.2 + r << :sleep_before_done + Thread.handle_interrupt(Timeout::ExitException => :on_blocking) { + r << :body + body.call + } + ensure + sleep 0.2 + r << :ensure_sleep_done + end + } + } + assert_equal([:sleep_before_done, :body, :ensure_sleep_done], r, issue) + end + end + + # Same as above but with an exception class + def test_handle_interrupt_with_interrupt_mask_inheritance_with_exception_class + issue = 'https://github.com/ruby/timeout/issues/41' + + [ + -> {}, # not blocking so no opportunity to interrupt + -> { sleep 5 } + ].each do |body| + # We need to create a new Timeout thread + kill_timeout_thread + + # Create the timeout thread under a handle_interrupt(:never) + # due to the interrupt mask being inherited + Thread.handle_interrupt(Object => :never) { + assert_equal :ok, Timeout.timeout(1) { :ok } + } + + # Ensure a simple timeout works and the interrupt mask was not inherited + assert_raise(Timeout::Error) { + Timeout.timeout(0.001) { sleep 1 } + } + + r = [] + assert_raise(Timeout::Error) { + Thread.handle_interrupt(Timeout::Error => :never) { + Timeout.timeout(0.1, Timeout::Error) do + sleep 0.2 + r << :sleep_before_done + Thread.handle_interrupt(Timeout::Error => :on_blocking) { + r << :body + body.call + } + ensure + sleep 0.2 + r << :ensure_sleep_done + end + } + } + assert_equal([:sleep_before_done, :body, :ensure_sleep_done], r, issue) + end + end + def test_fork omit 'fork not supported' unless Process.respond_to?(:fork) r, w = IO.pipe From 41308024d539446f5e78b5ca5b4baa27ec587e36 Mon Sep 17 00:00:00 2001 From: Benoit Daloze Date: Thu, 11 Dec 2025 12:29:09 +0100 Subject: [PATCH 2/2] Revise Timeout.timeout docs and add a section about `ensure` --- lib/timeout.rb | 59 ++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 52 insertions(+), 7 deletions(-) diff --git a/lib/timeout.rb b/lib/timeout.rb index 0260fd1..4281048 100644 --- a/lib/timeout.rb +++ b/lib/timeout.rb @@ -22,7 +22,7 @@ module Timeout # The version VERSION = "0.5.0" - # Internal error raised to when a timeout is triggered. + # Internal exception raised to when a timeout is triggered. class ExitException < Exception def exception(*) # :nodoc: self @@ -177,7 +177,7 @@ def finished # :startdoc: - # Perform an operation in a block, raising an error if it takes longer than + # Perform an operation in a block, raising an exception if it takes longer than # +sec+ seconds to complete. # # +sec+:: Number of seconds to wait for the block to terminate. Any non-negative number @@ -190,12 +190,18 @@ def finished # Omitting will use the default, "execution expired" # # Returns the result of the block *if* the block completed before - # +sec+ seconds, otherwise throws an exception, based on the value of +klass+. + # +sec+ seconds, otherwise raises an exception, based on the value of +klass+. # - # The exception thrown to terminate the given block cannot be rescued inside - # the block unless +klass+ is given explicitly. However, the block can use - # ensure to prevent the handling of the exception. For that reason, this - # method cannot be relied on to enforce timeouts for untrusted blocks. + # The exception raised to terminate the given block is the given +klass+, or + # Timeout::ExitException if +klass+ is not given. The reason for that behavior + # is that Timeout::Error inherits from RuntimeError and might be caught unexpectedly by `rescue`. + # Timeout::ExitException inherits from Exception so it will only be rescued by `rescue Exception`. + # Note that the Timeout::ExitException is translated to a Timeout::Error once it reaches the Timeout.timeout call, + # so outside that call it will be a Timeout::Error. + # + # In general, be aware that the code block may rescue the exception, and in such a case not respect the timeout. + # Also, the block can use +ensure+ to prevent the handling of the exception. + # For those reasons, this method cannot be relied on to enforce timeouts for untrusted blocks. # # If a scheduler is defined, it will be used to handle the timeout by invoking # Scheduler#timeout_after. @@ -203,6 +209,45 @@ def finished # Note that this is both a method of module Timeout, so you can include # Timeout into your classes so they have a #timeout method, as well as # a module method, so you can call it directly as Timeout.timeout(). + # + # ==== Ensuring the exception does not fire inside ensure blocks + # + # When using Timeout.timeout it can be desirable to ensure the timeout exception does not fire inside an +ensure+ block. + # The simplest and best way to do so it to put the Timeout.timeout call inside the body of the begin/ensure/end: + # + # begin + # Timeout.timeout(sec) { some_long_operation } + # ensure + # cleanup # safe, cannot be interrupt by timeout + # end + # + # If that is not feasible, e.g. if there are +ensure+ blocks inside +some_long_operation+, + # they need to not be interrupted by timeout, and it's not possible to move these ensure blocks outside, + # one can use Thread.handle_interrupt to delay the timeout exception like so: + # + # Thread.handle_interrupt(Timeout::Error => :never) { + # Timeout.timeout(sec, Timeout::Error) do + # setup # timeout cannot happen here, no matter how long it takes + # Thread.handle_interrupt(Timeout::Error => :immediate) { + # some_long_operation # timeout can happen here + # } + # ensure + # cleanup # timeout cannot happen here, no matter how long it takes + # end + # } + # + # An important thing to note is the need to pass an exception klass to Timeout.timeout, + # otherwise it does not work. Specifically, using +Thread.handle_interrupt(Timeout::ExitException => ...)+ + # is unsupported and causes subtle errors like raising the wrong exception outside the block, do not use that. + # + # Note that Thread.handle_interrupt is somewhat dangerous because if setup or cleanup hangs + # then the current thread will hang too and the timeout will never fire. + # Also note the block might run for longer than +sec+ seconds: + # e.g. some_long_operation executes for +sec+ seconds + whatever time cleanup takes. + # + # If you want the timeout to only happen on blocking operations one can use :on_blocking + # instead of :immediate. However, that means if the block uses no blocking operations after +sec+ seconds, + # the block will not be interrupted. def self.timeout(sec, klass = nil, message = nil, &block) #:yield: +sec+ return yield(sec) if sec == nil or sec.zero? raise ArgumentError, "Timeout sec must be a non-negative number" if 0 > sec