From 0dda36de2188d3a11b26a19726f80cc31793618b Mon Sep 17 00:00:00 2001 From: Koji Takao Date: Thu, 29 Jan 2026 19:54:28 +0900 Subject: [PATCH] Fix close() method hanging with long-running Node.js processes The close() method was calling process_thread.value without first terminating the Node.js process. If the process doesn't exit when stdin is closed (e.g., processes with setTimeout or event listeners), this would hang indefinitely. This fix adds Process.kill(:KILL, pid) before waiting, consistent with the finalizer implementation from PR #19. Added tests to verify: - Finalizer does not hang with long-running processes - Multiple instances are handled correctly under GC pressure - Fork safety is maintained - close() method does not hang Co-Authored-By: Claude Opus 4.5 --- lib/schmooze/base.rb | 5 ++ test/finalize_freeze_test.rb | 154 +++++++++++++++++++++++++++++++++++ 2 files changed, 159 insertions(+) create mode 100644 test/finalize_freeze_test.rb diff --git a/lib/schmooze/base.rb b/lib/schmooze/base.rb index ef3fd03..b477965 100644 --- a/lib/schmooze/base.rb +++ b/lib/schmooze/base.rb @@ -63,6 +63,11 @@ def close @_schmooze_stdin.close @_schmooze_stdout.close @_schmooze_stderr.close + begin + Process.kill(:KILL, @_schmooze_process_thread.pid) + rescue Errno::ESRCH + # Process is already dead, so no worries. + end @_schmooze_process_thread.value end diff --git a/test/finalize_freeze_test.rb b/test/finalize_freeze_test.rb new file mode 100644 index 0000000..4df7d64 --- /dev/null +++ b/test/finalize_freeze_test.rb @@ -0,0 +1,154 @@ +require 'test_helper' +require 'timeout' + +class FinalizeTest < Minitest::Test + # Use a long-running Node.js process that doesn't exit when stdin is closed + # This is critical for testing the finalizer because the issue only manifests + # when the process keeps running after stdin is closed. + class LongRunningSchmoozer < Schmooze::Base + # This method keeps the Node.js process running with a setTimeout + # Even after stdin is closed, the process will wait for the timeout + method :echo, 'function(x) { setTimeout(() => {}, 60000); return x; }' + end + + # Test that the finalizer does not hang when the process is still running. + # + # This test reproduces an issue where the old finalizer implementation + # used `Process.kill(0, pid)` which only checks if the process exists + # instead of actually terminating it. This caused `process_thread.value` + # to block indefinitely because the Node.js process was waiting for stdin. + # + # The fix uses `Process.kill(:KILL, pid)` to actually terminate the process + # before waiting for it. + def test_finalizer_does_not_hang + finalizer = nil + pid = nil + + # Capture the finalizer without letting it run automatically + ObjectSpace.stub :define_finalizer, proc { |_s, p| finalizer = p } do + schmoozer = LongRunningSchmoozer.new(__dir__) + schmoozer.echo("test") + pid = schmoozer.pid + + # Verify the process is running + assert pid, "Process should be running" + Process.kill(0, pid) # Should not raise if process is running + end + + # Run the finalizer with a timeout to detect hanging + assert_raises_nothing_within(5) do + finalizer.call + end + + # Verify the process was killed + assert_raises Errno::ESRCH do + Process.kill(0, pid) + end + end + + # Test that finalizer properly cleans up multiple instances + # This tests the scenario where GC.stress is enabled and many instances + # are created and garbage collected. + def test_finalizer_handles_multiple_instances_under_gc_pressure + pids = [] + finalizers = [] + + ObjectSpace.stub :define_finalizer, proc { |_s, p| finalizers << p } do + 5.times do + schmoozer = LongRunningSchmoozer.new(__dir__) + schmoozer.echo("test") + pids << schmoozer.pid + end + end + + assert_equal 5, pids.length + assert_equal 5, finalizers.length + + # All finalizers should complete without hanging + assert_raises_nothing_within(15) do + finalizers.each(&:call) + end + + # All processes should be terminated + pids.each do |pid| + assert_raises Errno::ESRCH do + Process.kill(0, pid) + end + end + end + + # Test fork safety: finalizer should only kill process in the original parent + def test_finalizer_is_fork_safe + skip "Fork not available on this platform" unless Process.respond_to?(:fork) + + finalizer = nil + pid = nil + + ObjectSpace.stub :define_finalizer, proc { |_s, p| finalizer = p } do + schmoozer = LongRunningSchmoozer.new(__dir__) + schmoozer.echo("test") + pid = schmoozer.pid + end + + # Fork and try to run finalizer in child + child_pid = fork do + # In child process, finalizer should not kill the process + # because owner_pid != Process.pid + finalizer.call + # Process should still be running (not killed by child) + begin + Process.kill(0, pid) + exit 0 # Success - process still running + rescue Errno::ESRCH + exit 1 # Failure - process was killed + end + end + + _, status = Process.waitpid2(child_pid) + assert_equal 0, status.exitstatus, "Child should not kill parent's process" + + # Process should still be running + Process.kill(0, pid) # Should not raise if process is still running + + # Now run finalizer in parent - it should kill the process + assert_raises_nothing_within(5) do + finalizer.call + end + + assert_raises Errno::ESRCH do + Process.kill(0, pid) + end + end + + # Test that the close method does not hang with long-running processes. + # This is similar to the finalizer issue - close() needs to kill the + # process before waiting for it to exit. + def test_close_does_not_hang + schmoozer = LongRunningSchmoozer.new(__dir__) + schmoozer.echo("test") + pid = schmoozer.pid + + assert pid, "Process should be running" + Process.kill(0, pid) # Should not raise + + # close() should not hang + assert_raises_nothing_within(5) do + schmoozer.close + end + + # Verify the process was killed + assert_raises Errno::ESRCH do + Process.kill(0, pid) + end + end + + private + + def assert_raises_nothing_within(seconds, message = nil) + Timeout.timeout(seconds) do + yield + end + rescue Timeout::Error + flunk(message || "Block did not complete within #{seconds} seconds (likely hung)") + end +end