behavior changes

lib/timeout.rb

@@ -58,6 +58,7 @@ class Request
     attr_reader :deadline
 
     def initialize(thread, timeout, exception_class, message)
+      @timeout = timeout
       @thread = thread
       @deadline = GET_TIME.call(Process::CLOCK_MONOTONIC) + timeout
       @exception_class = exception_class
@@ -67,6 +68,16 @@ def initialize(thread, timeout, exception_class, message)
       @done = false # protected by @mutex
     end
 
+    def in_runner_thread
+
+      # do we want to be able to pass down handle_interrupt values from parent thread?
+      # or does that already happen by default? test_handle_interrupt does pass.
+      @runner_thread = Thread.new do
+        yield
+      end
+      @runner_thread.value
+    end
+
     def done?
       @mutex.synchronize do
         @done
@@ -80,8 +91,21 @@ def expired?(now)
     def interrupt
       @mutex.synchronize do
         unless @done
-          @thread.raise @exception_class, @message
+          return unless @runner_thread
+          @runner_thread.raise @exception_class, @message
           @done = true
+
+          # value given to join is time allowed for inner ensure block.
+          # implication is total code runtime within Timeout.timeout block is (@timeout + whatever is given here)
+          # with timeout gem 0.4.0, it is infinite
+          #
+          # a sensible value to give here would be @timeout, capping total code execution time at 2x@timeout.
+          # because of how test_handle_interrupt is currently done, doing so makes it fail
+          # i don't know if the big delta between the timeout numbers in that test are crucial to its behavior check
+          # hardcoding 5 in here for now
+          @runner_thread.join(5) # replace with @timeout?
+          next unless @runner_thread.alive?
+          @thread.raise @exception_class, @message
         end
       end
     end
@@ -183,7 +207,9 @@ def timeout(sec, klass = nil, message = nil, &block)   #:yield: +sec+
         CONDVAR.signal
       end
       begin
-        return yield(sec)
+        request.in_runner_thread do
+          yield(sec)
+        end
       ensure
         request.finished
       end

test/error_lifecycle.rb

@@ -0,0 +1,49 @@
+class MyStandardError < StandardError; end
+class MyException< Exception; end
+
+class ErrorLifeCycleTester
+  attr_reader :inner_attempted, :inner_else, :inner_rescue, :inner_ensure, :inner_ensure_has_time_to_finish,
+              :outer_rescue, :outer_else, :outer_ensure, :outer_ensure_has_time_to_finish
+
+  def subject(error_to_raise, error_to_rescue)
+    @inner_attempted = false
+    @inner_else = false
+    @inner_rescue = false
+    @inner_ensure = false
+    @inner_ensure_has_time_to_finish = false
+
+    @outer_rescue = false
+    @outer_else = false
+    @outer_ensure = false
+    @outer_ensure_has_time_to_finish = false
+
+    begin
+      Timeout.timeout(0.001, error_to_raise) do
+        @inner_attempted = true
+        nil while true
+      rescue error_to_rescue
+        @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
+    else
+      @outer_else = true
+    ensure
+      @outer_ensure = true
+      t = Time.now; nil while Time.now < t+1
+      @outer_ensure_has_time_to_finish = true
+    end
+
+    # this is here to avoid cluttering the "UNDESIRED?" section of each test,
+    # can be flatted into the main tests
+    unless @outer_else ^ @outer_rescue
+      raise "something strange happened with the outer_rescue variables"
+    end
+  end
+end

test/test_error_lifecycle.rb

@@ -0,0 +1,85 @@
+require 'test/unit'
+require 'timeout'
+require 'thread'
+
+class TestTimeout < Test::Unit::TestCase
+
+  # Behavior marked "UNDESIRED?" is done so as John's opinion, these can/should be removed before the PR is merged
+
+  require_relative 'error_lifecycle.rb'
+
+  def core_assertions(s)
+    assert s.inner_attempted
+    assert !s.inner_else
+    assert s.inner_ensure
+    assert s.outer_ensure
+
+    # This can result in user's expectation of total possible time
+    # being very wrong
+    # t = Time.now; Timeout.timeout(0.1){begin; sleep 1; ensure; sleep 2; end} rescue puts Time.now-t
+    # => 2.106306
+    assert s.inner_ensure_has_time_to_finish
+    assert s.outer_ensure_has_time_to_finish
+  end
+
+  # when an exception to raise is not specified and the inner code does not catch Exception
+  def test_1
+    s = ErrorLifeCycleTester.new
+    s.subject(nil, StandardError)
+    core_assertions(s)
+
+    assert !s.inner_rescue
+    assert s.outer_rescue
+  end
+
+  # when an exception to raise is not specified and the inner code does catch Exception
+  def test_2
+    s = ErrorLifeCycleTester.new
+    s.subject(nil, Exception)
+    core_assertions(s)
+
+    assert s.inner_rescue # true in 1.9, false in gem 0.2.0, true in gem 0.4.0
+    assert s.outer_rescue # false in 1.9 stdlib, true in gem 0.2.0, false in gem 0.4.0
+  end
+
+  # when an exception to raise is StandardError and the inner code does not catch Exception
+  def test_3
+    s = ErrorLifeCycleTester.new
+    s.subject(MyStandardError, StandardError)
+    core_assertions(s)
+
+    assert s.inner_rescue
+    assert s.outer_rescue
+  end
+
+  # when an exception to raise is StandardError and the inner code does catch Exception
+  def test_4
+    s = ErrorLifeCycleTester.new
+    s.subject(MyStandardError, Exception)
+    core_assertions(s)
+
+    assert s.inner_rescue
+    assert s.outer_rescue
+  end
+
+  # when an exception to raise is Exception and the inner code does not catch Exception
+  def test_5
+    s = ErrorLifeCycleTester.new
+    s.subject(MyException, StandardError)
+    core_assertions(s)
+
+    assert !s.inner_rescue
+    assert s.outer_rescue
+  end
+
+  # when an exception to raise is Exception and the inner code does catch Exception
+  def test_6
+    s = ErrorLifeCycleTester.new
+    s.subject(MyException, Exception)
+    core_assertions(s)
+
+    assert s.inner_rescue
+    assert s.outer_rescue
+  end
+
+end

test/test_timeout.rb

@@ -91,15 +91,16 @@ def test_raises_exception_internally
   def test_rescue_exit
     exc = Class.new(RuntimeError)
     e = nil
-    assert_nothing_raised(exc) do
+    assert_raise_with_message(exc, 'execution expired') do
       Timeout.timeout 0.01, exc do
         begin
           sleep 3
         rescue exc => e
         end
       end
     end
-    assert_raise_with_message(exc, 'execution expired') {raise e if e}
+    # pretty sure irrelevant now that above behavior is reversed
+    # assert_raise_with_message(exc, 'execution expired') {raise e if e}
   end
 
   def test_custom_exception