From 8d471a9292449ef15f25c9ab2fddbde33dff9f6c Mon Sep 17 00:00:00 2001 From: AC Date: Sat, 10 Oct 2026 14:47:21 +0800 Subject: [PATCH 1/2] Avoid ScheduledTask timer lock inversion Signed-off-by: AC --- CHANGELOG.md | 3 ++ .../concurrent/scheduled_task.rb | 11 +++--- spec/concurrent/scheduled_task_spec.rb | 39 +++++++++++++++++++ 3 files changed, 48 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6461846ee..b5c41d938 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ ## Current +* Fix a lock inversion between `ScheduledTask#reschedule`/`#reset` and `TimerSet` + by using a volatile schedule time (#1099). + ## Release v1.3.8 (19 July 2026) concurrent-ruby: diff --git a/lib/concurrent-ruby/concurrent/scheduled_task.rb b/lib/concurrent-ruby/concurrent/scheduled_task.rb index efe9e193a..7d6f1ee5d 100644 --- a/lib/concurrent-ruby/concurrent/scheduled_task.rb +++ b/lib/concurrent-ruby/concurrent/scheduled_task.rb @@ -187,7 +187,7 @@ def initialize(delay, opts = {}, &task) @args = get_arguments_from(opts) @delay = delay.to_f @task = task - @time = nil + self.schedule_time = nil @executor = Options.executor_from_options(opts) || Concurrent.global_io_executor self.observers = Collection::CopyOnNotifyObserverSet.new end @@ -203,9 +203,10 @@ def initial_delay # The monotonic time at which the the task is scheduled to be executed. # # @return [Float] the schedule time or nil if `unscheduled` - def schedule_time - synchronize { @time } - end + # TimerSet reads this while holding its own lock. Do not acquire the task + # lock here: rescheduling takes the task lock before the TimerSet lock. + attr_volatile :schedule_time + private :schedule_time= # Comparator which orders by schedule time. # @@ -311,7 +312,7 @@ def process_task # @!visibility private def ns_schedule(delay) @delay = delay - @time = Concurrent.monotonic_time + @delay + self.schedule_time = Concurrent.monotonic_time + @delay @parent.send(:post_task, self) end diff --git a/spec/concurrent/scheduled_task_spec.rb b/spec/concurrent/scheduled_task_spec.rb index fc5af9143..0623b698b 100644 --- a/spec/concurrent/scheduled_task_spec.rb +++ b/spec/concurrent/scheduled_task_spec.rb @@ -174,6 +174,45 @@ def trigger_observable(observable) end end + context 'rescheduling while the timer reads the schedule time' do + [:reset, :reschedule].each do |operation| + it "does not deadlock during #{operation}" do + timer = TimerSet.new + task = ScheduledTask.execute(3600, timer_set: timer) { nil } + task_locked = CountDownLatch.new(1) + timer_locked = CountDownLatch.new(1) + threads = [] + + begin + threads << in_thread do + task.send(:synchronize) do + task_locked.count_down + raise 'timer did not acquire its lock' unless timer_locked.wait(2) + operation == :reset ? task.reset : task.reschedule(7200) + end + end + threads << in_thread do + raise 'task did not acquire its lock' unless task_locked.wait(2) + timer.send(:synchronize) do + timer_locked.count_down + # Queue comparisons read schedule_time while holding the timer lock. + task.schedule_time + end + end + + threads.each { |thread| expect(thread.join(2)).not_to be_nil } + expect(task).to be_pending + expected_delay = operation == :reset ? 3600 : 7200 + expect(task.initial_delay).to eq expected_delay + expect(task.schedule_time).to be_within(5).of(Concurrent.monotonic_time + expected_delay) + ensure + threads.each { |thread| thread.kill; thread.join } + timer.shutdown + end + end + end + end + context '#cancel' do it 'returns false if the task has already been performed' do From bb92326d0de2a5fbdf3e1621536bde5b21e71a96 Mon Sep 17 00:00:00 2001 From: Joshua Young Date: Sat, 17 Oct 2026 10:02:17 +1100 Subject: [PATCH 2/2] Move ScheduledTask deadlock spec next to TimerSet rescheduling specs Keep the schedule_time rationale out of its YARD docstring, and drop the CHANGELOG entry since release notes are generated from pull requests. --- CHANGELOG.md | 3 -- .../concurrent/scheduled_task.rb | 4 +- spec/concurrent/executor/timer_set_spec.rb | 26 +++++++++++++ spec/concurrent/scheduled_task_spec.rb | 39 ------------------- 4 files changed, 27 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b5c41d938..6461846ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,8 +1,5 @@ ## Current -* Fix a lock inversion between `ScheduledTask#reschedule`/`#reset` and `TimerSet` - by using a volatile schedule time (#1099). - ## Release v1.3.8 (19 July 2026) concurrent-ruby: diff --git a/lib/concurrent-ruby/concurrent/scheduled_task.rb b/lib/concurrent-ruby/concurrent/scheduled_task.rb index 7d6f1ee5d..e02735f62 100644 --- a/lib/concurrent-ruby/concurrent/scheduled_task.rb +++ b/lib/concurrent-ruby/concurrent/scheduled_task.rb @@ -203,9 +203,7 @@ def initial_delay # The monotonic time at which the the task is scheduled to be executed. # # @return [Float] the schedule time or nil if `unscheduled` - # TimerSet reads this while holding its own lock. Do not acquire the task - # lock here: rescheduling takes the task lock before the TimerSet lock. - attr_volatile :schedule_time + attr_volatile :schedule_time # read by TimerSet under its own lock, so it must not take the task lock private :schedule_time= # Comparator which orders by schedule time. diff --git a/spec/concurrent/executor/timer_set_spec.rb b/spec/concurrent/executor/timer_set_spec.rb index f4169473d..d9bc41a60 100644 --- a/spec/concurrent/executor/timer_set_spec.rb +++ b/spec/concurrent/executor/timer_set_spec.rb @@ -306,6 +306,32 @@ module Concurrent expect(success).to be false expect(task.schedule_time).to be_within(0.01).of(expected) end + + it 'does not deadlock when the timer reads the schedule time during rescheduling' do + task = subject.post(10){ nil } + task_locked = Concurrent::CountDownLatch.new + timer_locked = Concurrent::CountDownLatch.new + + rescheduler = in_thread do + task.send(:synchronize) do + task_locked.count_down + timer_locked.wait(2) + task.reschedule(20) + end + end + reader = in_thread do + task_locked.wait(2) + subject.send(:synchronize) do + timer_locked.count_down + task.schedule_time + end + end + + rescheduled = rescheduler.join(2) + [rescheduler, reader].each(&:kill) # cleanup, so the timer set can shut down after a deadlock + expect(rescheduled).not_to be_nil + expect(rescheduler.value).to be true + end end context 'task resetting' do diff --git a/spec/concurrent/scheduled_task_spec.rb b/spec/concurrent/scheduled_task_spec.rb index 0623b698b..fc5af9143 100644 --- a/spec/concurrent/scheduled_task_spec.rb +++ b/spec/concurrent/scheduled_task_spec.rb @@ -174,45 +174,6 @@ def trigger_observable(observable) end end - context 'rescheduling while the timer reads the schedule time' do - [:reset, :reschedule].each do |operation| - it "does not deadlock during #{operation}" do - timer = TimerSet.new - task = ScheduledTask.execute(3600, timer_set: timer) { nil } - task_locked = CountDownLatch.new(1) - timer_locked = CountDownLatch.new(1) - threads = [] - - begin - threads << in_thread do - task.send(:synchronize) do - task_locked.count_down - raise 'timer did not acquire its lock' unless timer_locked.wait(2) - operation == :reset ? task.reset : task.reschedule(7200) - end - end - threads << in_thread do - raise 'task did not acquire its lock' unless task_locked.wait(2) - timer.send(:synchronize) do - timer_locked.count_down - # Queue comparisons read schedule_time while holding the timer lock. - task.schedule_time - end - end - - threads.each { |thread| expect(thread.join(2)).not_to be_nil } - expect(task).to be_pending - expected_delay = operation == :reset ? 3600 : 7200 - expect(task.initial_delay).to eq expected_delay - expect(task.schedule_time).to be_within(5).of(Concurrent.monotonic_time + expected_delay) - ensure - threads.each { |thread| thread.kill; thread.join } - timer.shutdown - end - end - end - end - context '#cancel' do it 'returns false if the task has already been performed' do