Skip to content

Fix close() method hanging with long-running Node.js processes#50

Open
takaokouji wants to merge 1 commit intoShopify:masterfrom
speee:fix-close-method-hanging
Open

Fix close() method hanging with long-running Node.js processes#50
takaokouji wants to merge 1 commit intoShopify:masterfrom
speee:fix-close-method-hanging

Conversation

@takaokouji
Copy link

Summary

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.

Problem

When using Schmooze with Node.js code that keeps running after stdin is closed (such as code using setTimeout, setInterval, or event listeners), calling close() would hang forever:

class LongRunningSchmoozer < Schmooze::Base
  method :echo, 'function(x) { setTimeout(() => {}, 60000); return x; }'
end

schmoozer = LongRunningSchmoozer.new(__dir__)
schmoozer.echo("test")
schmoozer.close  # Hangs forever!

Solution

Added Process.kill(:KILL, pid) to the close() method before calling process_thread.value, matching the pattern already used in the finalizer:

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

Tests Added

  • test_finalizer_does_not_hang - Verifies finalizer completes without hanging
  • test_finalizer_handles_multiple_instances_under_gc_pressure - Tests cleanup of multiple instances
  • test_finalizer_is_fork_safe - Verifies fork safety is maintained
  • test_close_does_not_hang - Verifies close() completes without hanging

All tests use a long-running Node.js process (with setTimeout) to ensure the issue is properly reproduced and fixed.

Test Plan

  • All existing tests pass
  • New tests pass
  • Verified the fix works with long-running Node.js processes

🤖 Generated with Claude Code

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 Shopify#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 <noreply@anthropic.com>
@takaokouji
Copy link
Author

takaokouji commented Jan 29, 2026

We merged this at own repositry https://github.com/speee/schmooze . Please try it with below Gemfile line.

gem 'schmooze', github: 'speee/schmooze', tag: 'v26.1.1'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant