Skip to content

Handle writes to non-blocking pty instead of wrapping via Node's tty api #831

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Tyriar merged 10 commits into from
Dec 13, 2025
Merged

Handle writes to non-blocking pty instead of wrapping via Node's tty api #831

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3bc1409
Speculative fix for stream backpressure
Tyriar Dec 12, 2025
bb8a1af
Add additional change needed to fix it
Tyriar Dec 12, 2025
b8ecd5e
Use raw write stream, handle partial writes/kernel backpressure
Tyriar Dec 13, 2025
8b2b77b
Remove unneeded write stream
Tyriar Dec 13, 2025
3538d16
Remove unwanted logs
Tyriar Dec 13, 2025
80b2af7
Tidy up comments
Tyriar Dec 13, 2025
872ff8e
Add setting of encoding on the read socket back
Tyriar Dec 13, 2025
1674fdf
Move props to top
Tyriar Dec 13, 2025
6bb38cc
Use os.constants.errno
Tyriar Dec 13, 2025
c6b06ed
Revert "Use os.constants.errno"
Tyriar Dec 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions src/unix/pty.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,10 +448,10 @@ Napi::Value PtyFork(const Napi::CallbackInfo& info) {
}
#endif

Napi::Object obj = Napi::Object::New(napiEnv);
obj.Set("fd", Napi::Number::New(napiEnv, master));
obj.Set("pid", Napi::Number::New(napiEnv, pid));
obj.Set("pty", Napi::String::New(napiEnv, ptsname(master)));
Napi::Object obj = Napi::Object::New(napiEnv);
obj.Set("fd", Napi::Number::New(napiEnv, master));
obj.Set("pid", Napi::Number::New(napiEnv, pid));
obj.Set("pty", Napi::String::New(napiEnv, ptsname(master)));

// Set up process exit callback.
Napi::Function cb = info[10].As<Napi::Function>();
Expand Down Expand Up @@ -492,10 +492,10 @@ Napi::Value PtyOpen(const Napi::CallbackInfo& info) {
throw Napi::Error::New(env, "Could not set slave fd to nonblocking.");
}

Napi::Object obj = Napi::Object::New(env);
obj.Set("master", Napi::Number::New(env, master));
obj.Set("slave", Napi::Number::New(env, slave));
obj.Set("pty", Napi::String::New(env, ptsname(master)));
Napi::Object obj = Napi::Object::New(env);
obj.Set("master", Napi::Number::New(env, master));
obj.Set("slave", Napi::Number::New(env, slave));
obj.Set("pty", Napi::String::New(env, ptsname(master)));

return obj;
}
Expand Down
19 changes: 18 additions & 1 deletion src/unixTerminal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* Copyright (c) 2016, Daniel Imms (MIT License).
* Copyright (c) 2018, Microsoft Corporation (MIT License).
*/
import * as fs from 'fs';
import * as net from 'net';
import * as path from 'path';
import * as tty from 'tty';
Expand All @@ -25,6 +26,7 @@ const DESTROY_SOCKET_TIMEOUT_MS = 200;
export class UnixTerminal extends Terminal {
protected _fd: number;
protected _pty: string;
private _writeSocket!: net.Socket; // HACK: This is unsafe

protected _file: string;
protected _name: string;
Expand Down Expand Up @@ -102,6 +104,8 @@ export class UnixTerminal extends Terminal {
const term = pty.fork(file, args, parsedEnv, cwd, this._cols, this._rows, uid, gid, (encoding === 'utf8'), helperPath, onexit);

this._socket = new tty.ReadStream(term.fd);
// HACK: This needs to be created or all some data may get lost
this._writeSocket = new tty.WriteStream(term.fd);
if (encoding !== null) {
this._socket.setEncoding(encoding);
}
Expand Down Expand Up @@ -162,7 +166,20 @@ export class UnixTerminal extends Terminal {
}

protected _write(data: string | Buffer): void {
this._socket.write(data);
// Use the underlying file descriptor to avoid issues with backpressure
// handling in net.Socket/tty.*Stream which can result in data loss with
// ~1-4kb of data.
// Context: https://github.com/microsoft/vscode/issues/283056
fs.write(this.fd, data, (err, written) => {
Copy link

@Jarred-Sumner Jarred-Sumner Dec 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest the original approach of one stream because:

  • fs.write may not write all the data passed to it because the underlying system call doesn't guarantee all bytes are written. To use fs.write here, it would need to do something like data = data.slice(Buffer.byteLength(data) - written) and write again until data.length is empty
  • fs.write enqueues it to a threadpool (or io_uring) which means multiple write() calls could happen at the same time and in an interleaving order. To use fs.write here, this function would need to enqueue each write call and drain one after another

I think the question is why did it stop writing the final chunk? If I was debugging this, I would try to reproduce it and then run either strace node <file> or perf trace node <file> and compare the logs in the success scenario versus the failure scenario. Is it missing a write at the end? Did the write happen after the process already exited?

Since this native module is handling process spawning, I would also closely compare the libuv implementation of process spawning with this one.

My suspicion is that this is an edgecase in node:stream where when one end of the socket is shutdown, it caused the other end to stop sending data (sort of like allowHalfOpen)

Copy link
Member Author

@Tyriar Tyriar Dec 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't actually see this until just now after pushing. It's hinted to in the code, but note also there's a hang issue when writing on macOS specifically that can occur that is closely related to all this stuff (microsoft/vscode#246204).

I have a better implementation now that's pretty close to what you were suggesting. It's using fs.write directly and handles partial writes and EAGAIN. Here are some details I found which made it particularly confusing to work through:

  1. fs.WriteStream returns ERR_SYSTEM_ERROR which obfuscated the actual underlying EAGAIN so I couldn't handle it, fs.write was needed for this.
  2. The drain event never seems to happen for tty.WriteStream, it's implemented in Writable but not tty.WriteStream. I couldn't find why though, this is what an LLM said:
    • TTYs don’t apply meaningful backpressure
    • Writes are synchronous or minimally buffered
    • The stream buffer never fills
    • Therefore, drain is never needed
  3. tty.WriteStream has a habit of locking up the process completely when calling write with too much data. @lhecker investigated the hang earlier today and found it would always get stuck in uv__write Remove ugly throttling mechanism when pasting to the terminal vscode#283056 (comment). Decided not to investigate further and use fs.write as it appears simpler and seems to accomplish the goals.

if (err) {
console.log('pty write error', err);
}
console.log('written', written);
});
// TODO: Understand why this hangs the process
// this._writeSocket.write(data, (err) => {
// console.log('err', err);
// });
}

/* Accessors */
Expand Down