diff --git a/lib/cli/ui/spinner/spin_group.rb b/lib/cli/ui/spinner/spin_group.rb index ab459921..4b1ef5f3 100644 --- a/lib/cli/ui/spinner/spin_group.rb +++ b/lib/cli/ui/spinner/spin_group.rb @@ -439,6 +439,12 @@ def wait(to: $stdout) @work_queue.interrupt debrief(to: to) if @interrupt_debrief stopped? ? false : raise + rescue Exception # rubocop:disable Lint/RescueException + # A task failure outside StandardError is not ours to debrief, but it + # leaves wait mid-render. Stop the group and its workers before the + # exception escapes so sibling tasks do not continue unattended. + stop + raise end #: (String message) -> void diff --git a/lib/cli/ui/stdout_router.rb b/lib/cli/ui/stdout_router.rb index 20f8144b..ae429e83 100644 --- a/lib/cli/ui/stdout_router.rb +++ b/lib/cli/ui/stdout_router.rb @@ -209,36 +209,35 @@ def run StdoutRouter.assert_enabled! - Thread.current[:cliui_current_capture] = self - - prev_frame_inset = Thread.current[:no_cliui_frame_inset] - prev_hook = Thread.current[:cliui_output_hook] - - if Thread.current.respond_to?(:report_on_exception) - Thread.current.report_on_exception = false - end - - self.class.with_stdin_masked do - Thread.current[:no_cliui_frame_inset] = !@with_frame_inset - Thread.current[:cliui_output_hook] = ->(data, stream) do - stream = :stdout if @merged_output - case stream - when :stdout - @out.write(data) - @duplicate_output_to.write(data) - when :stderr - @err.write(data) - else raise + previous_capture = Thread.current[:cliui_current_capture] + begin + Thread.current[:cliui_current_capture] = self + self.class.with_stdin_masked do + previous_frame_inset = Thread.current[:no_cliui_frame_inset] + previous_hook = Thread.current[:cliui_output_hook] + begin + Thread.current[:no_cliui_frame_inset] = !@with_frame_inset + Thread.current[:cliui_output_hook] = ->(data, stream) do + stream = :stdout if @merged_output + case stream + when :stdout + @out.write(data) + @duplicate_output_to.write(data) + when :stderr + @err.write(data) + else raise + end + print_captured_output # suppress writing to terminal by default + end + @block.call + ensure + Thread.current[:cliui_output_hook] = previous_hook + Thread.current[:no_cliui_frame_inset] = previous_frame_inset end - print_captured_output # suppress writing to terminal by default end - - @block.call + ensure + Thread.current[:cliui_current_capture] = previous_capture end - ensure - Thread.current[:cliui_output_hook] = prev_hook - Thread.current[:no_cliui_frame_inset] = prev_frame_inset - Thread.current[:cliui_current_capture] = nil end #: -> String @@ -318,15 +317,17 @@ class << self def with_id(on_streams:, &block) require 'securerandom' id = format('%05d', rand(10**5)) - Thread.current[:cliui_output_id] = { - id: id, - streams: on_streams.map do |stream| - stream #: as io_like - end, - } - yield(id) - ensure - Thread.current[:cliui_output_id] = nil + streams = on_streams.map do |stream| + stream #: as io_like + end + + previous_id = Thread.current[:cliui_output_id] + begin + Thread.current[:cliui_output_id] = { id: id, streams: streams } + yield(id) + ensure + Thread.current[:cliui_output_id] = previous_id + end end #: -> Hash[Symbol, (String | io_like)]? diff --git a/lib/cli/ui/work_queue.rb b/lib/cli/ui/work_queue.rb index 47650e17..b1d75e8d 100644 --- a/lib/cli/ui/work_queue.rb +++ b/lib/cli/ui/work_queue.rb @@ -4,6 +4,10 @@ module CLI module UI class WorkQueue + # Settled into a future whose worker left without settling it itself. + class WorkerDied < StandardError + end + class Future #: -> void def initialize @@ -69,17 +73,19 @@ def initialize(max_concurrent) @max_concurrent = max_concurrent @queue = Queue.new #: Queue @mutex = Mutex.new #: Mutex + @interrupt_mutex = Mutex.new #: Mutex @condition = ConditionVariable.new #: ConditionVariable @workers = [] #: Array[Thread] + @stopping = false #: bool end #: { -> untyped } -> Future def enqueue(&block) future = Future.new @mutex.synchronize do + @queue.push([future, block]) start_worker if @workers.size < @max_concurrent end - @queue.push([future, block]) future end @@ -91,22 +97,49 @@ def close #: -> void def wait @queue.close - @workers.each(&:join) + loop do + workers = @mutex.synchronize { @workers.dup } + break if workers.empty? + + workers.each(&:join) + end end #: -> void def interrupt - @mutex.synchronize do - @queue.close - # Fail any remaining tasks in the queue - until @queue.empty? - future, _block = @queue.pop(true) - future&.fail(Interrupt.new) + @interrupt_mutex.synchronize do + workers = @mutex.synchronize do + @stopping = true + @queue.close + + # Fail any remaining tasks in the queue. Workers can consume from + # the queue concurrently, so an empty? check followed by pop is racy. + loop do + future, _block = @queue.pop(true) + future&.fail(Interrupt.new) + rescue ThreadError + break + end + + @workers.dup end - # Interrupt all worker threads - @workers.each { |worker| worker.raise(Interrupt) if worker.alive? } - @workers.each(&:join) - @workers.clear + + # These are WorkQueue-owned threads being deliberately torn down, so + # neither their thread-death report nor their Interrupt belongs to the + # caller performing the teardown. + workers.each do |worker| + next unless worker.alive? + + worker.report_on_exception = false + worker.raise(Interrupt) + end + workers.each do |worker| + worker.join + rescue Interrupt + nil + end + ensure + @mutex.synchronize { @workers.clear } end end @@ -115,26 +148,59 @@ def interrupt #: -> void def start_worker @workers << Thread.new do - loop do - work = @queue.pop - break if work.nil? - - future, block = work + run_worker + rescue Interrupt + # Clean exit on interrupt + ensure + worker_finished(Thread.current) + end + end - begin + #: -> void + def run_worker + loop do + future = nil #: Future? + begin + # Do not let an asynchronous exception land after Queue#pop has + # removed work but before its future is assigned. Interrupts remain + # enabled while pop is blocked and while the task itself is running. + Thread.handle_interrupt(Exception => :never) do + work = Thread.handle_interrupt(Exception => :on_blocking) { @queue.pop } + return if work.nil? + + future, block = work future.start - result = block.call + result = Thread.handle_interrupt(Exception => :immediate) { block.call } future.complete(result) - rescue Interrupt => e - future.fail(e) - raise # Always re-raise interrupts to terminate the worker - rescue StandardError => e - future.fail(e) - # Don't re-raise standard errors - allow worker to continue + end + rescue Interrupt => e + future&.fail(e) + raise # Always re-raise interrupts to terminate the worker + rescue Exception => e # rubocop:disable Lint/RescueException + # The future carries the error to callers. Keep the worker: tasks + # already queued may have no later enqueue to replace it. + future&.fail(e) + ensure + if future + Thread.handle_interrupt(Exception => :never) do + unless future.completed? + future.fail(WorkerDied.new('worker died before its task completed')) + end + end + end + end + end + end + + #: (Thread worker) -> void + def worker_finished(worker) + Thread.handle_interrupt(Exception => :never) do + @mutex.synchronize do + @workers.delete(worker) + if !@stopping && !@queue.empty? && @workers.size < @max_concurrent + start_worker end end - rescue Interrupt - # Clean exit on interrupt end end end diff --git a/test/cli/ui/spinner/spin_group_test.rb b/test/cli/ui/spinner/spin_group_test.rb index 4f262568..5d830fd3 100644 --- a/test/cli/ui/spinner/spin_group_test.rb +++ b/test/cli/ui/spinner/spin_group_test.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'test_helper' +require 'timeout' module CLI module UI @@ -36,6 +37,45 @@ def test_spin_group_auto_debrief_false assert_equal('', err) end + def test_spin_group_non_standard_error_does_not_hang_or_report_thread_death + _out, err = capture_io do + CLI::UI::StdoutRouter.ensure_activated + + sg = SpinGroup.new(auto_debrief: false) + sg.add('s') { raise NotImplementedError, 'not implemented' } + + error = Timeout.timeout(10) do + assert_raises(NotImplementedError) { sg.wait } + end + assert_equal('not implemented', error.message) + end + + assert_equal('', err) + end + + def test_spin_group_non_standard_error_stops_the_group_and_its_siblings + _out, err = capture_io do + CLI::UI::StdoutRouter.ensure_activated + + sg = SpinGroup.new(auto_debrief: false) + sibling_finished = false + sg.add('boom') { raise NotImplementedError, 'not implemented' } + sg.add('sibling') do + sleep(30) + sibling_finished = true + end + + Timeout.timeout(10) do + assert_raises(NotImplementedError) { sg.wait } + end + + assert(sg.stopped?) + refute(sibling_finished) + end + + assert_equal('', err) + end + def test_spin_group_success_debrief capture_io do CLI::UI::StdoutRouter.ensure_activated diff --git a/test/cli/ui/stdout_router_test.rb b/test/cli/ui/stdout_router_test.rb index 23be5aa5..748ece52 100644 --- a/test/cli/ui/stdout_router_test.rb +++ b/test/cli/ui/stdout_router_test.rb @@ -24,6 +24,139 @@ def test_current_id end end + def test_nested_with_id_restores_outer_id + StdoutRouter.with_id(on_streams: [$stdout]) do |outer_id| + StdoutRouter.with_id(on_streams: [$stdout]) do |inner_id| + assert_equal(inner_id, StdoutRouter.current_id&.fetch(:id)) + end + assert_equal(outer_id, StdoutRouter.current_id&.fetch(:id)) + end + assert_nil(StdoutRouter.current_id) + end + + def test_nested_with_id_restores_outer_id_when_inner_raises + StdoutRouter.with_id(on_streams: [$stdout]) do |outer_id| + assert_raises(RuntimeError) do + StdoutRouter.with_id(on_streams: [$stdout]) { raise('inner') } + end + assert_equal(outer_id, StdoutRouter.current_id&.fetch(:id)) + end + assert_nil(StdoutRouter.current_id) + end + + def test_capture_leaves_report_on_exception_untouched + capture_io do + StdoutRouter.with_enabled do + prev = Thread.current.report_on_exception + begin + [true, false].each do |value| + Thread.current.report_on_exception = value + during = nil + StdoutRouter::Capture.new { during = Thread.current.report_on_exception }.run + assert_equal(value, during) + assert_equal(value, Thread.current.report_on_exception) + end + ensure + Thread.current.report_on_exception = prev + end + end + end + end + + def test_capture_failure_in_a_thread_still_reports_thread_death + script = <<~RUBY + require 'cli/ui' + CLI::UI::StdoutRouter.enable + thread = Thread.new { CLI::UI::StdoutRouter::Capture.new { raise('boom') }.run } + begin + thread.join + rescue RuntimeError + nil + end + RUBY + + lib = File.expand_path('../../../lib', __dir__) + stdout, stderr, _ = Open3.capture3(RbConfig.ruby, '-I', lib, '-e', script) + + assert_match(/terminated with exception/, stderr, "stdout:\n#{stdout}\nstderr:\n#{stderr}") + end + + def test_nested_capture_restores_outer_capture + capture_io do + StdoutRouter.with_enabled do + inner_current = nil + restored_current = nil + inner = StdoutRouter::Capture.new do + inner_current = StdoutRouter::Capture.current_capture + end + outer = StdoutRouter::Capture.new do + inner.run + restored_current = StdoutRouter::Capture.current_capture + end + outer.run + assert_same(inner, inner_current) + assert_same(outer, restored_current) + assert_nil(StdoutRouter::Capture.current_capture) + end + end + end + + def test_nested_capture_restores_outer_capture_and_hook_when_inner_raises + capture_io do + StdoutRouter.with_enabled do + restored_current = nil + inner = StdoutRouter::Capture.new { raise('inner') } + outer = StdoutRouter::Capture.new do + assert_raises(RuntimeError) { inner.run } + restored_current = StdoutRouter::Capture.current_capture + puts('after inner') + end + outer.run + assert_same(outer, restored_current) + assert_includes(outer.stdout, 'after inner') + assert_nil(StdoutRouter::Capture.current_capture) + end + end + end + + def test_nested_capture_can_enter_alternate_screen + capture_io do + StdoutRouter.with_enabled do + entered_alternate_screen = false + outer = StdoutRouter::Capture.new do + StdoutRouter::Capture.new {}.run + StdoutRouter::Capture.in_alternate_screen do + entered_alternate_screen = true + end + end + + outer.run + + assert(entered_alternate_screen) + end + end + end + + def test_capture_restores_frame_inset_when_nested + capture_io do + StdoutRouter.with_enabled do + inset_during_inner = nil + inset_after_inner = nil + inner = StdoutRouter::Capture.new(with_frame_inset: true) do + inset_during_inner = Thread.current[:no_cliui_frame_inset] + end + outer = StdoutRouter::Capture.new(with_frame_inset: false) do + inner.run + inset_after_inner = Thread.current[:no_cliui_frame_inset] + end + outer.run + assert_equal(false, inset_during_inner) + assert_equal(true, inset_after_inner) + assert_nil(Thread.current[:no_cliui_frame_inset]) + end + end + end + def test_frame_can_autoload_after_router_is_enabled script = <<~RUBY require 'stringio' diff --git a/test/cli/ui/work_queue_test.rb b/test/cli/ui/work_queue_test.rb index 686fcdb0..11c2d162 100644 --- a/test/cli/ui/work_queue_test.rb +++ b/test/cli/ui/work_queue_test.rb @@ -1,6 +1,7 @@ # typed: false require 'test_helper' +require 'timeout' require 'cli/ui/work_queue' module CLI @@ -65,6 +66,56 @@ def test_future_error assert_raises(StandardError, 'Test error') { future.value } end + def test_future_non_standard_error_completes_and_worker_continues + @work_queue = WorkQueue.new(1) + failed = @work_queue.enqueue { raise NotImplementedError, 'not implemented' } + replacement = @work_queue.enqueue { :ran } + + @work_queue.wait + + assert(failed.completed?) + error = assert_raises(NotImplementedError) { failed.value } + assert_equal('not implemented', error.message) + assert_equal(:ran, replacement.value) + end + + def test_killed_worker_fails_its_future_and_runs_already_queued_work + @work_queue = WorkQueue.new(1) + started = Queue.new + abandoned = @work_queue.enqueue do + started.push(:started) + sleep(30) + end + replacement = @work_queue.enqueue { :ran } + + started.pop + @work_queue.instance_variable_get(:@workers).first.kill + Timeout.timeout(5) { @work_queue.wait } + + error = assert_raises(WorkQueue::WorkerDied) { abandoned.value } + assert_match(/died before its task completed/, error.message) + assert_equal(:ran, replacement.value) + end + + def test_task_interrupt_replaces_worker_for_already_queued_work + @work_queue = WorkQueue.new(1) + started = Queue.new + release = Queue.new + interrupted = @work_queue.enqueue do + started.push(:started) + release.pop + raise Interrupt + end + + started.pop + replacement = @work_queue.enqueue { :ran } + release.push(:continue) + Timeout.timeout(5) { @work_queue.wait } + + assert_raises(Interrupt) { interrupted.value } + assert_equal(:ran, replacement.value) + end + def test_max_concurrent max_concurrent = 2 @work_queue = WorkQueue.new(max_concurrent)