Skip to content

fix(node/stream): handle 'close' event in fromReadable to prevent Windows hang - #182

Merged
taras merged 1 commit into
mainfrom
fix/windows-stream-hang
Mar 7, 2026
Merged

fix(node/stream): handle 'close' event in fromReadable to prevent Windows hang#182
taras merged 1 commit into
mainfrom
fix/windows-stream-hang

Conversation

@taras

@taras taras commented Mar 7, 2026

Copy link
Copy Markdown
Member

Motivation

On Windows, when a child process terminates, its stdio streams may emit close without first emitting end. Since fromReadable() only listened for end to close the Effection signal, the subscription would never complete. This caused the Win32 exec() implementation to hang forever — it gates process completion on stdoutDone/stderrDone resolvers that only resolve when the stream is fully drained.

This manifested as the Windows CI hanging indefinitely on the "Run peer dependency matrix tests" step, which spawns node --test subprocesses via exec().

Approach

Add a close event listener to fromReadable() that also closes the signal. Since signal.close() is idempotent, receiving both end and close (the normal case on Linux/macOS) is safe. On Windows, whichever event fires first will close the signal and unblock the stream consumer.

@coderabbitai

coderabbitai Bot commented Mar 7, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Bumps package version (0.2.2 → 0.2.3) and augments Node readable stream handling by tracking end/close events: adds an ended flag, subscribes to "end" and "close" with guarded signal closure, and ensures both handlers are detached during cleanup.

Changes

Cohort / File(s) Summary
Version Management
node/package.json
Bumps package version from 0.2.2 to 0.2.3.
Stream Event Handling
node/stream.ts
Adds ended flag and end/close handlers; closes the abort signal on end or on close only if end hasn't occurred; detaches both handlers in cleanup; retains existing data and error handling.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: handling the 'close' event in fromReadable to fix a Windows-specific hang issue.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Policy Compliance ✅ Passed The PR complies with all applicable policies. The package.json contains a non-empty description field, and the version bump is a valid semantic patch increment.
Description check ✅ Passed The PR description fully addresses both required sections with clear motivation and detailed approach, matching the repository template structure.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/windows-stream-hang

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@pkg-pr-new

pkg-pr-new Bot commented Mar 7, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@effectionx/node@182

commit: c865a6e

…dows hang

On Windows, when a child process terminates, its stdio streams may emit
'close' without first emitting 'end'. Since fromReadable only listened
for 'end' to close the signal, the Effection subscription would never
complete. This caused the Win32 exec implementation to hang forever
waiting for stdoutDone/stderrDone resolvers that gate on stream drain.

The close handler is guarded: it only closes the signal when 'end' has
not already fired. This prevents a race where 'close' arrives before
all buffered 'data' events have been consumed, which would drop chunks
and hang tests waiting for expected output.

Also bumps @effectionx/node version to 0.2.3.
@taras
taras force-pushed the fix/windows-stream-hang branch from c1f48b2 to c9fec19 Compare March 7, 2026 12:20
@taras
taras merged commit c6febee into main Mar 7, 2026
6 checks passed
@taras
taras deleted the fix/windows-stream-hang branch March 7, 2026 13:57
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.

2 participants