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..03ccce43 100644 --- a/lib/cli/ui/work_queue.rb +++ b/lib/cli/ui/work_queue.rb @@ -128,9 +128,9 @@ def start_worker rescue Interrupt => e future.fail(e) raise # Always re-raise interrupts to terminate the worker - rescue StandardError => e + rescue Exception => e # rubocop:disable Lint/RescueException future.fail(e) - # Don't re-raise standard errors - allow worker to continue + # The future carries the error to callers; allow the worker to continue end end rescue Interrupt diff --git a/test/cli/ui/spinner/spin_group_test.rb b/test/cli/ui/spinner/spin_group_test.rb index 4f262568..1ea9b754 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,22 @@ 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_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..609e4f88 100644 --- a/test/cli/ui/work_queue_test.rb +++ b/test/cli/ui/work_queue_test.rb @@ -65,6 +65,19 @@ 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_max_concurrent max_concurrent = 2 @work_queue = WorkQueue.new(max_concurrent)