Reset the interrupt mask when creating the Timeout thread

* Add tests related to Thread.handle_interrupt
* Fixes #41

Author: eregon

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

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,13 +255,102 @@ def test_handle_interrupt
           ok = true
           Thread.handle_interrupt(Timeout::ExitException => :on_blocking) {
             sleep 0.2
+            raise "unreachable"
           }
         }
       }
     }
     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