Deprecate timeout

Author: pitr-ch

lib/concurrent/agent.rb

@@ -2,7 +2,6 @@
 
 require 'concurrent/dereferenceable'
 require 'concurrent/observable'
-require 'concurrent/utility/timeout'
 require 'concurrent/logging'
 
 module Concurrent
@@ -64,6 +63,7 @@ def rescue(clazz = StandardError, &block)
       end
       self
     end
+
     alias_method :catch, :rescue
     alias_method :on_error, :rescue
 
@@ -87,6 +87,7 @@ def validate(&block)
       end
       self
     end
+
     alias_method :validates, :validate
     alias_method :validate_with, :validate
     alias_method :validates_with, :validate
@@ -106,20 +107,30 @@ def post(&block)
     # Update the current value with the result of the given block fast,
     # block can do blocking calls
     #
-    # @param [Fixnum, nil] timeout maximum number of seconds before an update is cancelled
+    # @param [Fixnum, nil] timeout [DEPRECATED] maximum number of seconds before an update is cancelled
     #
     # @yield the fast to be performed with the current value in order to calculate
     #   the new value
     # @yieldparam [Object] value the current value
     # @yieldreturn [Object] the new value
     # @return [true, nil] nil when no block is given
     def post_off(timeout = nil, &block)
-      block = if timeout
-                lambda { |value| Concurrent::timeout(timeout) { block.call(value) } }
+      warn '[DEPRECATED] post_off with timeout options is deprecated and will be removed'
+      task = if timeout
+                lambda do |value|
+                  future = Future.execute do
+                    block.call(value)
+                  end
+                  if future.wait(timeout)
+                    future.value!
+                  else
+                    raise Concurrent::TimeoutError
+                  end
+                end
               else
                 block
               end
-      post_on(@io_executor, &block)
+      post_on(@io_executor, &task)
     end
 
     # Update the current value with the result of the given block fast,

lib/concurrent/utility/timeout.rb

@@ -5,12 +5,13 @@
 
 module Concurrent
 
-  # Wait the given number of seconds for the block operation to complete.
+  # [DEPRECATED] Wait the given number of seconds for the block operation to complete.
   # Intended to be a simpler and more reliable replacement to the Ruby
-  # standard library `Timeout::timeout` method.
+  # standard library `Timeout::timeout` method. It does not kill the task
+  # so it finishes anyway. Advantage is that it cannot cause any ugly errors by
+  # killing threads.
   #
   # @param [Integer] seconds The number of seconds to wait
-  #
   # @return [Object] The result of the block operation
   #
   # @raise [Concurrent::TimeoutError] when the block operation does not complete
@@ -19,20 +20,16 @@ module Concurrent
   # @see http://ruby-doc.org/stdlib-2.2.0/libdoc/timeout/rdoc/Timeout.html Ruby Timeout::timeout
   #
   # @!macro monotonic_clock_warning
-  def timeout(seconds)
-
-    thread = Thread.new do
-      Thread.current[:result] = yield
-    end
-    success = thread.join(seconds)
+  def timeout(seconds, &block)
+    warn '[DEPRECATED] timeout is deprecated and will be removed'
 
-    if success
-      return thread[:result]
+    future = Future.execute(&block)
+    future.wait(seconds)
+    if future.complete?
+      future.value!
     else
       raise TimeoutError
     end
-  ensure
-    Thread.kill(thread) unless thread.nil?
   end
   module_function :timeout
 end

spec/concurrent/agent_spec.rb

@@ -33,11 +33,19 @@ module Concurrent
       end
 
       it 'times out' do
-        ex = nil
-        subject.post_off(0.1) { |v| sleep(0.2); ex = true }
+        ex      = nil
+        timeout = false
+        subject.rescue(Concurrent::TimeoutError) { timeout = true }
+        subject.post_off(0.1) do |v|
+          sleep(0.2)
+          ex = true
+        end
+        sleep 0.1
         subject.await
+        expect(timeout).to eq false
         sleep 0.3
-        expect(ex).to eq nil
+        expect(timeout).to eq false
+        expect(ex).to eq true
       end
     end
 
@@ -410,8 +418,10 @@ def trigger_observable(observable)
         agent    = Agent.new(0, executor: executor)
         agent.post { |old| old + continue.value }
         sleep 0.1
-        Concurrent.timeout(0.2) { expect(agent.value).to eq 0 }
+        expect(agent.value).to eq 0
         continue.set 1
+        agent.await
+        expect(agent.value).to eq 1
         sleep 0.1
       end
 

spec/concurrent/utility/timeout_spec.rb

@@ -3,46 +3,25 @@ module Concurrent
   describe '#timeout' do
 
     it 'raises an exception if no block is given' do
-      expect {
-        Concurrent::timeout(1)
-      }.to raise_error
+      expect { Concurrent::timeout(0.1) }.to raise_error
     end
 
     it 'returns the value of the block on success' do
-      result = Concurrent::timeout(1) { 42 }
+      result = Concurrent::timeout(0.1) { 42 }
       expect(result).to eq 42
     end
 
     it 'raises an exception if the timeout value is reached' do
-      expect {
-        Concurrent::timeout(1){ sleep }
-      }.to raise_error(Concurrent::TimeoutError)
+      expect { Concurrent::timeout(0.1) { sleep 2 } }.to raise_error(Concurrent::TimeoutError)
     end
 
     it 'bubbles thread exceptions' do
-      expect {
-        Concurrent::timeout(1){ raise NotImplementedError }
-      }.to raise_error
+      expect { Concurrent::timeout(0.1) { raise NotImplementedError } }.to raise_error
     end
 
-    it 'kills the thread on success' do
-      result = Concurrent::timeout(1) { 42 }
-      expect(Thread).to receive(:kill).with(any_args())
-      Concurrent::timeout(1){ 42 }
-    end
-
-    it 'kills the thread on timeout' do
-      expect(Thread).to receive(:kill).with(any_args())
-      expect {
-        Concurrent::timeout(1){ sleep }
-      }.to raise_error
-    end
-
-    it 'kills the thread on exception' do
-      expect(Thread).to receive(:kill).with(any_args())
-      expect {
-        Concurrent::timeout(1){ raise NotImplementedError }
-      }.to raise_error
+    it 'does not kill the thread on timeout' do
+      expect(Thread).not_to receive(:kill).with(any_args())
+      expect { Concurrent::timeout(0.1) { sleep 2 } }.to raise_error
     end
   end
 end