Defer conptyNative.connect() to prevent event loop blocking #885
+225
−22
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes microsoft#763 The issue was that conptyNative.connect() called ConnectNamedPipe() synchronously for both input and output pipes. While the input pipe was connected via fs.openSync() before the call, the output pipe connection happened asynchronously in a worker thread. If the worker thread hadn't connected yet when ConnectNamedPipe was called, it would block the Node.js event loop waiting for the connection. This caused a deadlock when a debugger was attached (which slows down the event loop and worker thread startup). The fix defers the conptyNative.connect() call until the worker thread signals it has connected to the output pipe (via the onReady callback). This ensures both pipes have clients connected before ConnectNamedPipe is called, so it returns immediately without blocking.
If the worker fails to signal ready within 5 seconds, complete the connection anyway to avoid leaving the PTY in a zombie state.
Clear _pendingPtyInfo in kill() to prevent the timeout or onReady callback from calling connect() on an already-killed PTY handle.
The public pid property was only set once at construction, before the deferred connection. Update it in the ready_datapipe handler so users get the correct pid value.
Add test to ensure WindowsTerminal.pid is correctly updated after the deferred connection completes. This closes the test coverage gap for the public pid property.
Tyriar
approved these changes
Feb 3, 2026
aeschli
approved these changes
Feb 3, 2026
Member
Author
|
Thanks for the quick reviews! 🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(Note: this is my first PR here so apologies in advance if I missed any important steps!)
ConnectNamedPipeblocks the Node.js event loop when called before the worker thread has connected to the output pipe. This causes spawning PTYs to freeze, particularly noticeable when using a debugger (as in the original issue report) and when running parallel processes with limited CPU cores (e.g. in CI environments).This change defers the call to
conptyNative.connect()until the worker signals it's ready via the onReady callback. At that point,ConnectNamedPipereturns immediately because the client is already connected.The new tests added in this PR fail without the fix (innerPid is immediately set to a real PID) and pass with it.
Fix #763