From 2bafc458f1bbc7c344475a2c97a6f05161868773 Mon Sep 17 00:00:00 2001 From: Benoit Daloze Date: Thu, 12 May 2022 16:20:56 +0200 Subject: [PATCH 1/5] Reimplement Timeout.timeout with a single thread and a Queue --- lib/timeout.rb | 125 ++++++++++++++++++++++++++++++------------- test/test_timeout.rb | 13 +++++ 2 files changed, 101 insertions(+), 37 deletions(-) diff --git a/lib/timeout.rb b/lib/timeout.rb index f50e706..3c3cb8e 100644 --- a/lib/timeout.rb +++ b/lib/timeout.rb @@ -1,4 +1,4 @@ -# frozen_string_literal: false +# frozen_string_literal: true # Timeout long-running blocks # # == Synopsis @@ -23,7 +23,7 @@ # Copyright:: (C) 2000 Information-technology Promotion Agency, Japan module Timeout - VERSION = "0.2.0".freeze + VERSION = "0.2.0" # Raised by Timeout.timeout when the block times out. class Error < RuntimeError @@ -50,9 +50,79 @@ def exception(*) end # :stopdoc: - THIS_FILE = /\A#{Regexp.quote(__FILE__)}:/o - CALLER_OFFSET = ((c = caller[0]) && THIS_FILE =~ c) ? 1 : 0 - private_constant :THIS_FILE, :CALLER_OFFSET + CONDVAR = ConditionVariable.new + QUEUE = Queue.new + QUEUE_MUTEX = Mutex.new + TIMEOUT_THREAD_MUTEX = Mutex.new + @timeout_thread = nil + private_constant :CONDVAR, :QUEUE, :QUEUE_MUTEX, :TIMEOUT_THREAD_MUTEX + + class Request + attr_reader :deadline + + def initialize(thread, timeout, exception_class, message) + @thread = thread + @deadline = Process.clock_gettime(Process::CLOCK_MONOTONIC) + timeout + @exception_class = exception_class + @message = message + + @mutex = Mutex.new + @done = false + end + + def done? + @done + end + + def expired?(now) + now >= @deadline and !done? + end + + def interrupt + @mutex.synchronize do + unless @done + @thread.raise @exception_class, @message + @done = true + end + end + end + + def finished + @mutex.synchronize do + @done = true + end + end + end + private_constant :Request + + def self.ensure_timeout_thread_created + unless @timeout_thread + TIMEOUT_THREAD_MUTEX.synchronize do + @timeout_thread ||= 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 = Process.clock_gettime(Process::CLOCK_MONOTONIC)) < closest_deadline and QUEUE.empty? + CONDVAR.wait(QUEUE_MUTEX, closest_deadline - now) + end + end + + requests.each do |req| + req.interrupt if req.expired?(now) + end + requests.reject!(&:done?) + end + end + end + end + end # :startdoc: # Perform an operation in a block, raising an error if it takes longer than @@ -83,51 +153,32 @@ def exception(*) def timeout(sec, klass = nil, message = nil, &block) #:yield: +sec+ return yield(sec) if sec == nil or sec.zero? - message ||= "execution expired".freeze + message ||= "execution expired" if Fiber.respond_to?(:current_scheduler) && (scheduler = Fiber.current_scheduler)&.respond_to?(:timeout_after) return scheduler.timeout_after(sec, klass || Error, message, &block) end - from = "from #{caller_locations(1, 1)[0]}" if $DEBUG - e = Error - bl = proc do |exception| + Timeout.ensure_timeout_thread_created + perform = Proc.new do |exc| + request = Request.new(Thread.current, sec, exc, message) + QUEUE_MUTEX.synchronize do + QUEUE << request + CONDVAR.signal + end begin - x = Thread.current - y = Thread.start { - Thread.current.name = from - begin - sleep sec - rescue => e - x.raise e - else - x.raise exception, message - end - } return yield(sec) ensure - if y - y.kill - y.join # make sure y is dead. - end + request.finished end end + if klass - begin - bl.call(klass) - rescue klass => e - message = e.message - bt = e.backtrace - end + perform.call(klass) else - bt = Error.catch(message, &bl) + backtrace = Error.catch(&perform) + raise Error, message, backtrace end - level = -caller(CALLER_OFFSET).size-2 - while THIS_FILE =~ bt[level] - bt.delete_at(level) - end - raise(e, message, bt) end - module_function :timeout end diff --git a/test/test_timeout.rb b/test/test_timeout.rb index 74b65f1..58eb842 100644 --- a/test/test_timeout.rb +++ b/test/test_timeout.rb @@ -10,6 +10,18 @@ def test_non_timing_out_code_is_successful end end + def test_included + c = Class.new do + include Timeout + def test + timeout(1) { :ok } + end + end + assert_nothing_raised do + assert_equal :ok, c.new.test + end + end + def test_yield_param assert_equal [5, :ok], Timeout.timeout(5){|s| [s, :ok] } end @@ -43,6 +55,7 @@ def test_skip_rescue begin sleep 3 rescue Exception => e + flunk "should not see any exception but saw #{e.inspect}" end end end From 74add5edd03c1843d5d8e29a847361fab71a88ae Mon Sep 17 00:00:00 2001 From: Benoit Daloze Date: Fri, 13 May 2022 12:33:54 +0200 Subject: [PATCH 2/5] Add jruby and truffleruby in CI --- .github/workflows/test.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index cc4d46e..f942cfb 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -1,4 +1,4 @@ -name: ubuntu +name: test on: [push, pull_request] @@ -6,8 +6,9 @@ jobs: build: name: build (${{ matrix.ruby }} / ${{ matrix.os }}) strategy: + fail-fast: false matrix: - ruby: [ '3.0', 2.7, 2.6, 2.5, 2.4, head ] + ruby: [ '3.0', 2.7, 2.6, 2.5, 2.4, head, jruby, truffleruby-head ] os: [ ubuntu-latest, macos-latest ] runs-on: ${{ matrix.os }} steps: From 4baee63b9b0d1691e02b97722c7b7bee33e9e0c6 Mon Sep 17 00:00:00 2001 From: Benoit Daloze Date: Sun, 15 May 2022 13:43:29 +0200 Subject: [PATCH 3/5] Handle Timeout + fork and add test for it --- lib/timeout.rb | 51 +++++++++++++++++++++++++------------------- test/test_timeout.rb | 20 +++++++++++++++++ 2 files changed, 49 insertions(+), 22 deletions(-) diff --git a/lib/timeout.rb b/lib/timeout.rb index 3c3cb8e..4149830 100644 --- a/lib/timeout.rb +++ b/lib/timeout.rb @@ -95,30 +95,37 @@ def finished end private_constant :Request + def self.create_timeout_thread + 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 = Process.clock_gettime(Process::CLOCK_MONOTONIC)) < closest_deadline and QUEUE.empty? + CONDVAR.wait(QUEUE_MUTEX, closest_deadline - now) + end + end + + requests.each do |req| + req.interrupt if req.expired?(now) + end + requests.reject!(&:done?) + end + end + end + private_class_method :create_timeout_thread + def self.ensure_timeout_thread_created - unless @timeout_thread + unless @timeout_thread and @timeout_thread.alive? TIMEOUT_THREAD_MUTEX.synchronize do - @timeout_thread ||= 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 = Process.clock_gettime(Process::CLOCK_MONOTONIC)) < closest_deadline and QUEUE.empty? - CONDVAR.wait(QUEUE_MUTEX, closest_deadline - now) - end - end - - requests.each do |req| - req.interrupt if req.expired?(now) - end - requests.reject!(&:done?) - end + unless @timeout_thread and @timeout_thread.alive? + @timeout_thread = create_timeout_thread end end end diff --git a/test/test_timeout.rb b/test/test_timeout.rb index 58eb842..76de389 100644 --- a/test/test_timeout.rb +++ b/test/test_timeout.rb @@ -139,4 +139,24 @@ def test_handle_interrupt } assert(ok, bug11344) end + + def test_fork + omit 'fork not supported' unless Process.respond_to?(:fork) + r, w = IO.pipe + pid = fork do + r.close + begin + r = Timeout.timeout(0.01) { sleep 5 } + w.write r.inspect + rescue Timeout::Error + w.write 'timeout' + ensure + w.close + end + end + w.close + Process.wait pid + assert_equal 'timeout', r.read + r.close + end end From 5e0d8e1637afed1a2e912ad588c89127512b1c94 Mon Sep 17 00:00:00 2001 From: Benoit Daloze Date: Sun, 15 May 2022 13:49:31 +0200 Subject: [PATCH 4/5] Synchronize all accesses to @done * So it is trivially correct. * Performance seems the same overall. --- lib/timeout.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/timeout.rb b/lib/timeout.rb index 4149830..4659ec3 100644 --- a/lib/timeout.rb +++ b/lib/timeout.rb @@ -67,11 +67,13 @@ def initialize(thread, timeout, exception_class, message) @message = message @mutex = Mutex.new - @done = false + @done = false # protected by @mutex end def done? - @done + @mutex.synchronize do + @done + end end def expired?(now) From 5f43254f81c69d0f3d7673c1a671f6e7288e2c35 Mon Sep 17 00:00:00 2001 From: Benoit Daloze Date: Sun, 15 May 2022 13:51:57 +0200 Subject: [PATCH 5/5] Remove redundant done? check * It's already checked inside #interrupt. --- lib/timeout.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/timeout.rb b/lib/timeout.rb index 4659ec3..6e7adbd 100644 --- a/lib/timeout.rb +++ b/lib/timeout.rb @@ -77,7 +77,7 @@ def done? end def expired?(now) - now >= @deadline and !done? + now >= @deadline end def interrupt