From 3bc1409befecf0f387ed85b9a031371265f100ed Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Fri, 12 Dec 2025 10:47:20 -0800 Subject: [PATCH 01/10] Speculative fix for stream backpressure Part of microsoft/vscode#283056 Part of microsoft/vscode#246204 --- src/unix/pty.cc | 16 ++++++++-------- src/unixTerminal.ts | 11 ++++++++++- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/src/unix/pty.cc b/src/unix/pty.cc index ec5524f5..7b4b9e1f 100644 --- a/src/unix/pty.cc +++ b/src/unix/pty.cc @@ -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(); @@ -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; } diff --git a/src/unixTerminal.ts b/src/unixTerminal.ts index 211aadcb..9c65986a 100644 --- a/src/unixTerminal.ts +++ b/src/unixTerminal.ts @@ -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'; @@ -162,7 +163,15 @@ 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) => { + if (err) { + console.log('pty write error', err); + } + }); } /* Accessors */ From bb8a1af7f0b7c65f3bc745640202dca14b94efcb Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Fri, 12 Dec 2025 10:55:42 -0800 Subject: [PATCH 02/10] Add additional change needed to fix it --- src/unixTerminal.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/unixTerminal.ts b/src/unixTerminal.ts index 9c65986a..763e92f8 100644 --- a/src/unixTerminal.ts +++ b/src/unixTerminal.ts @@ -26,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; @@ -103,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); } @@ -171,7 +174,12 @@ export class UnixTerminal extends Terminal { 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 */ From b8ecd5eefb7c7848f6fa22ae25f19a4c2820bacf Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Fri, 12 Dec 2025 19:13:02 -0800 Subject: [PATCH 03/10] Use raw write stream, handle partial writes/kernel backpressure --- src/unixTerminal.ts | 94 +++++++++++++++++++++++++++++++++++++-------- 1 file changed, 77 insertions(+), 17 deletions(-) diff --git a/src/unixTerminal.ts b/src/unixTerminal.ts index 763e92f8..5d871acc 100644 --- a/src/unixTerminal.ts +++ b/src/unixTerminal.ts @@ -26,7 +26,6 @@ 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; @@ -36,6 +35,8 @@ export class UnixTerminal extends Terminal { private _boundClose: boolean = false; private _emittedClose: boolean = false; + + private readonly _writeStream: fs.WriteStream; private _master: net.Socket | undefined; private _slave: net.Socket | undefined; @@ -104,11 +105,11 @@ 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); - } + this._writeStream = fs.createWriteStream('', { + fd: term.fd, + encoding: encoding ?? undefined, + autoClose: false + }); // setup this._socket.on('error', (err: any) => { @@ -165,21 +166,77 @@ export class UnixTerminal extends Terminal { this._forwardEvents(); } + private _writeQueue: (string | Buffer)[] = []; + protected _write(data: string | Buffer): void { - // 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) => { + // Writes are put in a queue and processed asynchronously in order to handle + // backpressure from the kernel buffer. + this._writeQueue.push(data); + if (this._writeInProgress) { + return; + } + this._writeInProgress = true; + this._processWriteQueue(); + } + + private _writeInProgress: boolean = false; + private _writeTimeout: NodeJS.Timeout | undefined; + + private async _processWriteQueue(): Promise { + const data = this._writeQueue.shift(); + if (!data) { + this._writeInProgress = false; + return; + } + + // Write to the underlying file descriptor and handle it directly, rather + // than using the `net.Socket`/`tty.WriteStream` wrappers which swallow the + // errors and cause the thread to block indefinitely. + fs.write(this._fd, data, (err, written) => { + console.log('written', written); if (err) { - console.log('pty write error', err); + const errno = (err as any).errno; + switch (errno) { + case -35: // EAGAIN (macOS) + case -11: // EAGAIN (Linux( + // This error appears to get swallowed and translated into + // `ERR_SYSTEM_ERROR` when using tty.WriteStream and not fs.write + // directly. + // This can happen during a regular partial write, but the most + // reliable way to test this is to run `sleep 10` in the shell and + // paste enough data to fill the kernel-level buffer. Once the sleep + // ends, the pty should accept the data again. Re-process after a + // short break. + this._writeTimeout = setTimeout(() => this._processWriteQueue(), 5); + return; + case -5: // macOS+Linux EIO + case -32: // EPIPE + // Stop processing writes immediately as the pty is closed. + this._writeInProgress = false; + return; + default: + console.error('Unhandled pty write error', errno, err); + // Fall through as it's important to finish processing the queue + break; + } } - console.log('written', written); + + // Requeue any partial writes + if (written < data.length) { + console.log('requeueing unwritten', data.slice(written).length); + this._writeQueue.unshift(data.slice(written)); + } + + // Using `setImmediate` here appears to corrupt the data, this may be what + // the interleaving/dropped data comment is about in Node.js' tty module: + // https://github.com/nodejs/node/blob/4cac2b94bed4bf02810be054e8f63c0048c66564/lib/tty.js#L106C1-L111C34 + // + // Yielding via `setImmediate` also doesn't seem to drain the buffer much + // anyway, so use a short timeout when this happens instead. Note that the `drain` + // event does not appear to happen on `net.Socket`/`tty.WriteStream` when + // writing to ptys. + this._writeTimeout = setTimeout(() => this._processWriteQueue(), 5); }); - // TODO: Understand why this hangs the process - // this._writeSocket.write(data, (err) => { - // console.log('err', err); - // }); } /* Accessors */ @@ -255,6 +312,9 @@ export class UnixTerminal extends Terminal { }); this._socket.destroy(); + + this._writeStream?.destroy(); + clearTimeout(this._writeTimeout); } public kill(signal?: string): void { From 8b2b77bd7959c3eefcee82f6878ffd4f5e40c356 Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Fri, 12 Dec 2025 19:36:00 -0800 Subject: [PATCH 04/10] Remove unneeded write stream --- src/unixTerminal.ts | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/unixTerminal.ts b/src/unixTerminal.ts index 5d871acc..f771ceb1 100644 --- a/src/unixTerminal.ts +++ b/src/unixTerminal.ts @@ -36,7 +36,6 @@ export class UnixTerminal extends Terminal { private _boundClose: boolean = false; private _emittedClose: boolean = false; - private readonly _writeStream: fs.WriteStream; private _master: net.Socket | undefined; private _slave: net.Socket | undefined; @@ -105,11 +104,6 @@ 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); - this._writeStream = fs.createWriteStream('', { - fd: term.fd, - encoding: encoding ?? undefined, - autoClose: false - }); // setup this._socket.on('error', (err: any) => { @@ -313,7 +307,6 @@ export class UnixTerminal extends Terminal { this._socket.destroy(); - this._writeStream?.destroy(); clearTimeout(this._writeTimeout); } From 3538d165fc32c2e4e8f23d383606ee50914fec2a Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Fri, 12 Dec 2025 19:45:47 -0800 Subject: [PATCH 05/10] Remove unwanted logs --- src/unixTerminal.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/unixTerminal.ts b/src/unixTerminal.ts index f771ceb1..3a516b26 100644 --- a/src/unixTerminal.ts +++ b/src/unixTerminal.ts @@ -187,7 +187,6 @@ export class UnixTerminal extends Terminal { // than using the `net.Socket`/`tty.WriteStream` wrappers which swallow the // errors and cause the thread to block indefinitely. fs.write(this._fd, data, (err, written) => { - console.log('written', written); if (err) { const errno = (err as any).errno; switch (errno) { @@ -217,7 +216,6 @@ export class UnixTerminal extends Terminal { // Requeue any partial writes if (written < data.length) { - console.log('requeueing unwritten', data.slice(written).length); this._writeQueue.unshift(data.slice(written)); } From 80b2af767675caa849afd80deabd3c2eb809578d Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Fri, 12 Dec 2025 19:47:12 -0800 Subject: [PATCH 06/10] Tidy up comments --- src/unixTerminal.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/unixTerminal.ts b/src/unixTerminal.ts index 3a516b26..ed9d8a8e 100644 --- a/src/unixTerminal.ts +++ b/src/unixTerminal.ts @@ -191,7 +191,7 @@ export class UnixTerminal extends Terminal { const errno = (err as any).errno; switch (errno) { case -35: // EAGAIN (macOS) - case -11: // EAGAIN (Linux( + case -11: // EAGAIN (Linux) // This error appears to get swallowed and translated into // `ERR_SYSTEM_ERROR` when using tty.WriteStream and not fs.write // directly. @@ -202,7 +202,7 @@ export class UnixTerminal extends Terminal { // short break. this._writeTimeout = setTimeout(() => this._processWriteQueue(), 5); return; - case -5: // macOS+Linux EIO + case -5: // EIO case -32: // EPIPE // Stop processing writes immediately as the pty is closed. this._writeInProgress = false; From 872ff8ec4a7628554cd58c1085797e6aedaaa595 Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Sat, 13 Dec 2025 06:27:48 -0800 Subject: [PATCH 07/10] Add setting of encoding on the read socket back --- src/unixTerminal.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/unixTerminal.ts b/src/unixTerminal.ts index ed9d8a8e..da3ff72a 100644 --- a/src/unixTerminal.ts +++ b/src/unixTerminal.ts @@ -104,6 +104,9 @@ 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); + if (encoding !== null) { + this._socket.setEncoding(encoding); + } // setup this._socket.on('error', (err: any) => { From 1674fdfe28bce51eacac1d6f7e60efb0b248ab1c Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Sat, 13 Dec 2025 06:28:51 -0800 Subject: [PATCH 08/10] Move props to top --- src/unixTerminal.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/unixTerminal.ts b/src/unixTerminal.ts index da3ff72a..ce2e89ff 100644 --- a/src/unixTerminal.ts +++ b/src/unixTerminal.ts @@ -36,6 +36,10 @@ export class UnixTerminal extends Terminal { private _boundClose: boolean = false; private _emittedClose: boolean = false; + private readonly _writeQueue: (string | Buffer)[] = []; + private _writeInProgress: boolean = false; + private _writeTimeout: NodeJS.Timeout | undefined; + private _master: net.Socket | undefined; private _slave: net.Socket | undefined; @@ -163,8 +167,6 @@ export class UnixTerminal extends Terminal { this._forwardEvents(); } - private _writeQueue: (string | Buffer)[] = []; - protected _write(data: string | Buffer): void { // Writes are put in a queue and processed asynchronously in order to handle // backpressure from the kernel buffer. @@ -176,9 +178,6 @@ export class UnixTerminal extends Terminal { this._processWriteQueue(); } - private _writeInProgress: boolean = false; - private _writeTimeout: NodeJS.Timeout | undefined; - private async _processWriteQueue(): Promise { const data = this._writeQueue.shift(); if (!data) { From 6bb38ccb06802a7f034e7071f9ae506206274372 Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Sat, 13 Dec 2025 07:47:01 -0800 Subject: [PATCH 09/10] Use os.constants.errno --- src/unixTerminal.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/unixTerminal.ts b/src/unixTerminal.ts index ce2e89ff..c8d20dd6 100644 --- a/src/unixTerminal.ts +++ b/src/unixTerminal.ts @@ -4,6 +4,7 @@ * Copyright (c) 2018, Microsoft Corporation (MIT License). */ import * as fs from 'fs'; +import * as os from 'os'; import * as net from 'net'; import * as path from 'path'; import * as tty from 'tty'; @@ -192,8 +193,7 @@ export class UnixTerminal extends Terminal { if (err) { const errno = (err as any).errno; switch (errno) { - case -35: // EAGAIN (macOS) - case -11: // EAGAIN (Linux) + case os.constants.errno.EAGAIN: // This error appears to get swallowed and translated into // `ERR_SYSTEM_ERROR` when using tty.WriteStream and not fs.write // directly. @@ -204,8 +204,8 @@ export class UnixTerminal extends Terminal { // short break. this._writeTimeout = setTimeout(() => this._processWriteQueue(), 5); return; - case -5: // EIO - case -32: // EPIPE + case os.constants.errno.EIO: + case os.constants.errno.EPIPE: // Stop processing writes immediately as the pty is closed. this._writeInProgress = false; return; From c6b06ed2c21865e468ebce569018076b57f45e2a Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Sat, 13 Dec 2025 08:29:08 -0800 Subject: [PATCH 10/10] Revert "Use os.constants.errno" This reverts commit 6bb38ccb06802a7f034e7071f9ae506206274372. errno.EAGAIN is coming through as 35 (positive) on macOS, not -35. --- src/unixTerminal.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/unixTerminal.ts b/src/unixTerminal.ts index c8d20dd6..ce2e89ff 100644 --- a/src/unixTerminal.ts +++ b/src/unixTerminal.ts @@ -4,7 +4,6 @@ * Copyright (c) 2018, Microsoft Corporation (MIT License). */ import * as fs from 'fs'; -import * as os from 'os'; import * as net from 'net'; import * as path from 'path'; import * as tty from 'tty'; @@ -193,7 +192,8 @@ export class UnixTerminal extends Terminal { if (err) { const errno = (err as any).errno; switch (errno) { - case os.constants.errno.EAGAIN: + case -35: // EAGAIN (macOS) + case -11: // EAGAIN (Linux) // This error appears to get swallowed and translated into // `ERR_SYSTEM_ERROR` when using tty.WriteStream and not fs.write // directly. @@ -204,8 +204,8 @@ export class UnixTerminal extends Terminal { // short break. this._writeTimeout = setTimeout(() => this._processWriteQueue(), 5); return; - case os.constants.errno.EIO: - case os.constants.errno.EPIPE: + case -5: // EIO + case -32: // EPIPE // Stop processing writes immediately as the pty is closed. this._writeInProgress = false; return;