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..ce2e89ff 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'; @@ -34,6 +35,11 @@ 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; @@ -162,7 +168,69 @@ export class UnixTerminal extends Terminal { } protected _write(data: string | Buffer): void { - this._socket.write(data); + // 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 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) => { + if (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: // 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; + } + } + + // Requeue any partial writes + if (written < data.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); + }); } /* Accessors */ @@ -238,6 +306,8 @@ export class UnixTerminal extends Terminal { }); this._socket.destroy(); + + clearTimeout(this._writeTimeout); } public kill(signal?: string): void {