From 2e7e774f93a914e7338789b59fc81a63cc716492 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Fri, 24 May 2019 02:36:13 +0200 Subject: [PATCH 01/14] custom flow control and discard limit: - adding watermark for write buffers - low and high watermark to toggle flow control - sanity watermark to discard data --- demo/server.js | 5 ++- package.json | 2 +- src/Terminal.ts | 111 ++++++++++++++++++++++++++++-------------------- yarn.lock | 17 +++++--- 4 files changed, 81 insertions(+), 54 deletions(-) diff --git a/demo/server.js b/demo/server.js index c7c9fc9b26..bb410a9d7d 100644 --- a/demo/server.js +++ b/demo/server.js @@ -45,7 +45,10 @@ function startServer() { rows: rows || 24, cwd: process.env.PWD, env: process.env, - encoding: USE_BINARY_UTF8 ? null : 'utf8' + encoding: USE_BINARY_UTF8 ? null : 'utf8', + handleFlowControl: true, + flowControlPause: '\x1b^p\x1b\\', + flowControlResume: '\x1b^r\x1b\\' }); console.log('Created terminal with PID: ' + term.pid); diff --git a/package.json b/package.json index 043b76cba5..60b2df006b 100644 --- a/package.json +++ b/package.json @@ -31,7 +31,7 @@ "gulp-util": "3.0.8", "jsdom": "^11.11.0", "merge-stream": "^1.0.1", - "node-pty": "0.7.6", + "node-pty": "0.9.0-beta10", "nodemon": "1.10.2", "nyc": "^11.8.0", "puppeteer": "^1.15.0", diff --git a/src/Terminal.ts b/src/Terminal.ts index 8dd9f461f2..bb9de85864 100644 --- a/src/Terminal.ts +++ b/src/Terminal.ts @@ -59,11 +59,29 @@ import { RenderCoordinator } from './renderer/RenderCoordinator'; const document = (typeof window !== 'undefined') ? window.document : null; /** - * The amount of write requests to queue before sending an XOFF signal to the - * pty process. This number must be small in order for ^C and similar sequences - * to be responsive. + * Safety watermark to avoid memory exhaustion. + * The actual watermark is calculated as sum of + * unhandled chunk sizes in both write buffers. */ -const WRITE_BUFFER_PAUSE_THRESHOLD = 5; +const DISCARD_WATERMARK = 10000000; // FIXME: should this be bigger? + +/** + * Flow control watermarks for the write buffer. + * low: send resume to pty + * high: send pause to pty + * + * TODO: make this configurable + */ +const LOW_WATERMARK = 100000; +const HIGH_WATERMARK = 300000; + +/** + * Flow control PAUSE/RESUME messages. + * + * TODO: make this configurable + */ +const FLOW_CONTROL_PAUSE = '\x1b^p\x1b\\'; // PM p ST +const FLOW_CONTROL_RESUME = '\x1b^r\x1b\\'; // PM r ST /** * The max number of ms to spend on writes before allowing the renderer to @@ -187,6 +205,7 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II public writeBuffer: string[]; public writeBufferUtf8: Uint8Array[]; private _writeInProgress: boolean; + private _watermark: number = 0; /** * Whether _xterm.js_ sent XOFF in order to catch up with the pty process. @@ -1371,18 +1390,23 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II return; } - this.writeBufferUtf8.push(data); + // safety measure: dont allow the backend to crash + // the terminal by writing to much data to fast. + if (this._watermark > DISCARD_WATERMARK) { + // FIXME: do something more useful + console.error('write data discarded, use flow control to avoid losing data'); + return; + } - // Send XOFF to pause the pty process if the write buffer becomes too large so - // xterm.js can catch up before more data is sent. This is necessary in order - // to keep signals such as ^C responsive. - if (this.options.useFlowControl && !this._xoffSentToCatchUp && this.writeBufferUtf8.length >= WRITE_BUFFER_PAUSE_THRESHOLD) { - // XOFF - stop pty pipe - // XON will be triggered by emulator before processing data chunk - this.handler(C0.DC3); + // flow control: pause pty (like XOFF) + this._watermark += data.length; + if (this.options.useFlowControl && this._watermark > HIGH_WATERMARK) { + this.handler(FLOW_CONTROL_PAUSE); this._xoffSentToCatchUp = true; } + this.writeBufferUtf8.push(data); + if (!this._writeInProgress && this.writeBufferUtf8.length > 0) { // Kick off a write which will write all data in sequence recursively this._writeInProgress = true; @@ -1404,23 +1428,11 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II const data = this.writeBufferUtf8[bufferOffset]; bufferOffset++; - // If XOFF was sent in order to catch up with the pty process, resume it if - // we reached the end of the writeBuffer to allow more data to come in. - if (this._xoffSentToCatchUp && this.writeBufferUtf8.length === bufferOffset) { - this.handler(C0.DC1); - this._xoffSentToCatchUp = false; - } - this._refreshStart = this.buffer.y; this._refreshEnd = this.buffer.y; - // HACK: Set the parser state based on it's state at the time of return. - // This works around the bug #662 which saw the parser state reset in the - // middle of parsing escape sequence in two chunks. For some reason the - // state of the parser resets to 0 after exiting parser.parse. This change - // just sets the state back based on the correct return statement. - this._inputHandler.parseUtf8(data); + this._watermark -= data.length; this.updateRange(this.buffer.y); this.refresh(this._refreshStart, this._refreshEnd); @@ -1429,6 +1441,13 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II break; } } + + // flow control: resume pty (like XON) + if (this._xoffSentToCatchUp && this._watermark < LOW_WATERMARK) { + this.handler(FLOW_CONTROL_RESUME); + this._xoffSentToCatchUp = false; + } + if (this.writeBufferUtf8.length > bufferOffset) { // Allow renderer to catch up before processing the next batch // trim already processed chunks if we are above threshold @@ -1458,18 +1477,23 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II return; } - this.writeBuffer.push(data); + // safety measure: dont allow the backend to crash + // the terminal by writing to much data to fast. + if (this._watermark > DISCARD_WATERMARK) { + // FIXME: do something more useful + console.error('write data discarded, use flow control to avoid losing data'); + return; + } - // Send XOFF to pause the pty process if the write buffer becomes too large so - // xterm.js can catch up before more data is sent. This is necessary in order - // to keep signals such as ^C responsive. - if (this.options.useFlowControl && !this._xoffSentToCatchUp && this.writeBuffer.length >= WRITE_BUFFER_PAUSE_THRESHOLD) { - // XOFF - stop pty pipe - // XON will be triggered by emulator before processing data chunk - this.handler(C0.DC3); + // flow control: pause pty (like XOFF) + this._watermark += data.length; + if (this.options.useFlowControl && this._watermark > HIGH_WATERMARK) { + this.handler(FLOW_CONTROL_PAUSE); this._xoffSentToCatchUp = true; } + this.writeBuffer.push(data); + if (!this._writeInProgress && this.writeBuffer.length > 0) { // Kick off a write which will write all data in sequence recursively this._writeInProgress = true; @@ -1491,23 +1515,11 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II const data = this.writeBuffer[bufferOffset]; bufferOffset++; - // If XOFF was sent in order to catch up with the pty process, resume it if - // we reached the end of the writeBuffer to allow more data to come in. - if (this._xoffSentToCatchUp && this.writeBuffer.length === bufferOffset) { - this.handler(C0.DC1); - this._xoffSentToCatchUp = false; - } - this._refreshStart = this.buffer.y; this._refreshEnd = this.buffer.y; - // HACK: Set the parser state based on it's state at the time of return. - // This works around the bug #662 which saw the parser state reset in the - // middle of parsing escape sequence in two chunks. For some reason the - // state of the parser resets to 0 after exiting parser.parse. This change - // just sets the state back based on the correct return statement. - this._inputHandler.parse(data); + this._watermark -= data.length; this.updateRange(this.buffer.y); this.refresh(this._refreshStart, this._refreshEnd); @@ -1516,6 +1528,13 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II break; } } + + // flow control: resume pty (like XON) + if (this._xoffSentToCatchUp && this._watermark < LOW_WATERMARK) { + this.handler(FLOW_CONTROL_RESUME); + this._xoffSentToCatchUp = false; + } + if (this.writeBuffer.length > bufferOffset) { // Allow renderer to catch up before processing the next batch // trim already processed chunks if we are above threshold diff --git a/yarn.lock b/yarn.lock index 321fe6f390..0b1a2b6668 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4484,7 +4484,12 @@ mute-stream@0.0.7: resolved "https://registry.yarnpkg.com/mute-stream/-/mute-stream-0.0.7.tgz#3075ce93bc21b8fab43e1bc4da7e8115ed1e7bab" integrity sha1-MHXOk7whuPq0PhvE2n6BFe0ee6s= -nan@2.10.0, nan@^2.9.2: +nan@^2.13.2: + version "2.14.0" + resolved "https://registry.yarnpkg.com/nan/-/nan-2.14.0.tgz#7818f722027b2459a86f0295d434d1fc2336c52c" + integrity sha512-INOFj37C7k3AfaNTtX8RhsTw7qRy7eLET14cROi9+5HAVbbHuIWUHEauBv5qT4Av2tWasiTY1Jw6puUNqRJXQg== + +nan@^2.9.2: version "2.10.0" resolved "https://registry.yarnpkg.com/nan/-/nan-2.10.0.tgz#96d0cd610ebd58d4b4de9cc0c6828cda99c7548f" integrity sha512-bAdJv7fBLhWC+/Bls0Oza+mvTaNQtP+1RyhhhvD95pgUJz6XM5IzgmxOkItJ9tkoCiplvAnXI1tNmmUD/eScyA== @@ -4592,12 +4597,12 @@ node-pre-gyp@^0.10.0: semver "^5.3.0" tar "^4" -node-pty@0.7.6: - version "0.7.6" - resolved "https://registry.yarnpkg.com/node-pty/-/node-pty-0.7.6.tgz#bff6148c9c5836ca7e73c7aaaec067dcbdac2f7b" - integrity sha512-ECzKUB7KkAFZ0cjyjMXp5WLJ+7YIZ1xnNmiiegOI6WdDaKABUNV5NbB1Dw9MXD4KrZipWII0wQ7RGZ6StU/7jA== +node-pty@0.9.0-beta10: + version "0.9.0-beta10" + resolved "https://registry.yarnpkg.com/node-pty/-/node-pty-0.9.0-beta10.tgz#058850d6971b04fefaa5ffe00a8816cd892f1419" + integrity sha512-I+wvK1FCiaAkIhlW7zA7V2FkJSX2JjOto5R9DXLGQGWMIXo+n2f0vXu7YLMbGaR5eR6NIm6KP0UhsFKKCn/bwg== dependencies: - nan "2.10.0" + nan "^2.13.2" nodemon@1.10.2: version "1.10.2" From d7ccd9f2a8923d1b0e5c549dbabde34d90df3971 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Fri, 24 May 2019 03:26:56 +0200 Subject: [PATCH 02/14] make linter happy --- src/Terminal.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Terminal.ts b/src/Terminal.ts index bb9de85864..74bd8d2fb5 100644 --- a/src/Terminal.ts +++ b/src/Terminal.ts @@ -69,7 +69,7 @@ const DISCARD_WATERMARK = 10000000; // FIXME: should this be bigger? * Flow control watermarks for the write buffer. * low: send resume to pty * high: send pause to pty - * + * * TODO: make this configurable */ const LOW_WATERMARK = 100000; @@ -77,7 +77,7 @@ const HIGH_WATERMARK = 300000; /** * Flow control PAUSE/RESUME messages. - * + * * TODO: make this configurable */ const FLOW_CONTROL_PAUSE = '\x1b^p\x1b\\'; // PM p ST From 97767f199fbae40c167fe7abf1c40246b00abf74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Sun, 26 May 2019 03:33:37 +0200 Subject: [PATCH 03/14] fining tuning watermarks --- demo/server.js | 30 +++++++++++++++++++++--------- fast_producer.c | 8 ++++++++ src/Terminal.ts | 5 +++-- 3 files changed, 32 insertions(+), 11 deletions(-) create mode 100644 fast_producer.c diff --git a/demo/server.js b/demo/server.js index bb410a9d7d..892c83b84c 100644 --- a/demo/server.js +++ b/demo/server.js @@ -54,9 +54,9 @@ function startServer() { console.log('Created terminal with PID: ' + term.pid); terminals[term.pid] = term; logs[term.pid] = ''; - term.on('data', function(data) { - logs[term.pid] += data; - }); + //term.on('data', function(data) { + // logs[term.pid] += data; + //}); res.send(term.pid.toString()); res.end(); }); @@ -75,15 +75,20 @@ function startServer() { app.ws('/terminals/:pid', function (ws, req) { var term = terminals[parseInt(req.params.pid)]; console.log('Connected to terminal ' + term.pid); - ws.send(logs[term.pid]); + //ws.send(logs[term.pid]); // string message buffering - function buffer(socket, timeout) { + function buffer(socket, timeout, limit) { let s = ''; let sender = null; return (data) => { s += data; - if (!sender) { + if (s.length > limit) { + clearTimeout(sender); + socket.send(s); + s = ''; + sender = null; + } else if (!sender) { sender = setTimeout(() => { socket.send(s); s = ''; @@ -93,14 +98,20 @@ function startServer() { }; } // binary message buffering - function bufferUtf8(socket, timeout) { + function bufferUtf8(socket, timeout, limit) { let buffer = []; let sender = null; let length = 0; return (data) => { buffer.push(data); length += data.length; - if (!sender) { + if (length > limit) { + clearTimeout(sender); + socket.send(Buffer.concat(buffer, length)); + buffer = []; + sender = null; + length = 0; + } else if (!sender) { sender = setTimeout(() => { socket.send(Buffer.concat(buffer, length)); buffer = []; @@ -110,10 +121,11 @@ function startServer() { } }; } - const send = USE_BINARY_UTF8 ? bufferUtf8(ws, 5) : buffer(ws, 5); + const send = USE_BINARY_UTF8 ? bufferUtf8(ws, 5, 16384) : buffer(ws, 5, 16384); term.on('data', function(data) { try { + console.log(data.length); send(data); } catch (ex) { // The WebSocket is not open, ignore diff --git a/fast_producer.c b/fast_producer.c new file mode 100644 index 0000000000..50f3760742 --- /dev/null +++ b/fast_producer.c @@ -0,0 +1,8 @@ +#include +#include + +int main(int argc, char **argv) { + while (1) { + putchar('#'); + } +} diff --git a/src/Terminal.ts b/src/Terminal.ts index 74bd8d2fb5..f01331e529 100644 --- a/src/Terminal.ts +++ b/src/Terminal.ts @@ -72,8 +72,8 @@ const DISCARD_WATERMARK = 10000000; // FIXME: should this be bigger? * * TODO: make this configurable */ -const LOW_WATERMARK = 100000; -const HIGH_WATERMARK = 300000; +const LOW_WATERMARK = 32768; +const HIGH_WATERMARK = 131072; /** * Flow control PAUSE/RESUME messages. @@ -1467,6 +1467,7 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II * @param data The text to write to the terminal. */ public write(data: string): void { + console.log((this._watermark/1000).toFixed(2), data.length); // Ensure the terminal isn't disposed if (this._isDisposed) { return; From a4ec9b3eec0e45f99673946b4a69c2d8c6607d2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Sun, 26 May 2019 03:37:51 +0200 Subject: [PATCH 04/14] make linter happy --- src/Terminal.ts | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/Terminal.ts b/src/Terminal.ts index 439c5f3d69..a893576f6d 100644 --- a/src/Terminal.ts +++ b/src/Terminal.ts @@ -63,25 +63,21 @@ const document = (typeof window !== 'undefined') ? window.document : null; * The actual watermark is calculated as sum of * unhandled chunk sizes in both write buffers. */ -const DISCARD_WATERMARK = 10000000; // FIXME: should this be bigger? +const DISCARD_WATERMARK = 10000000; /** * Flow control watermarks for the write buffer. * low: send resume to pty * high: send pause to pty - * - * TODO: make this configurable */ const LOW_WATERMARK = 32768; const HIGH_WATERMARK = 131072; /** * Flow control PAUSE/RESUME messages. - * - * TODO: make this configurable */ -const FLOW_CONTROL_PAUSE = '\x1b^p\x1b\\'; // PM p ST -const FLOW_CONTROL_RESUME = '\x1b^r\x1b\\'; // PM r ST +const FLOW_CONTROL_PAUSE = '\x1b^pause\x1b\\'; // PM pause ST +const FLOW_CONTROL_RESUME = '\x1b^resume\x1b\\'; // PM resume ST /** * The max number of ms to spend on writes before allowing the renderer to @@ -1467,7 +1463,7 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II * @param data The text to write to the terminal. */ public write(data: string): void { - console.log((this._watermark/1000).toFixed(2), data.length); + console.log((this._watermark / 1000).toFixed(2), data.length); // Ensure the terminal isn't disposed if (this._isDisposed) { return; From ca3e47da975165f1134a35c5c41cf6da0b2b80b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Sun, 26 May 2019 03:43:53 +0200 Subject: [PATCH 05/14] fix PAUSE/RESUME in server.js --- demo/server.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/demo/server.js b/demo/server.js index 892c83b84c..6408ee55b1 100644 --- a/demo/server.js +++ b/demo/server.js @@ -47,8 +47,8 @@ function startServer() { env: process.env, encoding: USE_BINARY_UTF8 ? null : 'utf8', handleFlowControl: true, - flowControlPause: '\x1b^p\x1b\\', - flowControlResume: '\x1b^r\x1b\\' + flowControlPause: '\x1b^pause\x1b\\', + flowControlResume: '\x1b^resume\x1b\\' }); console.log('Created terminal with PID: ' + term.pid); From 34c3a777e99899c6f81a60de0faed2e74739377b Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Sun, 26 May 2019 20:48:54 -0700 Subject: [PATCH 06/14] Add useFlowControl as an option in demo --- demo/client.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/demo/client.ts b/demo/client.ts index 8bcdc9718d..e07d68d0b4 100644 --- a/demo/client.ts +++ b/demo/client.ts @@ -203,7 +203,6 @@ function initOptions(term: TerminalType): void { 'handler', 'screenKeys', 'termName', - 'useFlowControl', // Complex option 'theme' ]; From 43e2fb33cdb499cc123c784a403eb7959b86b548 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Thu, 30 May 2019 15:50:50 +0200 Subject: [PATCH 07/14] comment discard watermark --- src/Terminal.ts | 42 ++++++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/src/Terminal.ts b/src/Terminal.ts index a893576f6d..4bfa64c7f9 100644 --- a/src/Terminal.ts +++ b/src/Terminal.ts @@ -59,11 +59,15 @@ import { RenderCoordinator } from './renderer/RenderCoordinator'; const document = (typeof window !== 'undefined') ? window.document : null; /** - * Safety watermark to avoid memory exhaustion. - * The actual watermark is calculated as sum of - * unhandled chunk sizes in both write buffers. + * Safety watermark to avoid memory exhaustion and browser engine crash on fast data input. + * Once hit the terminal will stop working. Enable flow control to avoid this limit + * and make sure that your backend correctly propagates this to the underlying pty. + * (see docs for further instructions) + * Since this limit is meant as a safety parachute to prevent browser crashs, + * it is set to a very high number. Typically xterm.js gets unresponsive with + * a much lower number (>500 kB). */ -const DISCARD_WATERMARK = 10000000; +const DISCARD_WATERMARK = 50000000; // ~50 MB /** * Flow control watermarks for the write buffer. @@ -201,7 +205,13 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II public writeBuffer: string[]; public writeBufferUtf8: Uint8Array[]; private _writeInProgress: boolean; - private _watermark: number = 0; + + /** + * Sum of length of pending chunks in all write buffers. + * Note: For the string chunks the actual memory usage is + * doubled (JSString char takes 2 bytes). + */ + private _writeBuffersPendingSize: number = 0; /** * Whether _xterm.js_ sent XOFF in order to catch up with the pty process. @@ -1388,15 +1398,15 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II // safety measure: dont allow the backend to crash // the terminal by writing to much data to fast. - if (this._watermark > DISCARD_WATERMARK) { + if (this._writeBuffersPendingSize > DISCARD_WATERMARK) { // FIXME: do something more useful console.error('write data discarded, use flow control to avoid losing data'); return; } // flow control: pause pty (like XOFF) - this._watermark += data.length; - if (this.options.useFlowControl && this._watermark > HIGH_WATERMARK) { + this._writeBuffersPendingSize += data.length; + if (this.options.useFlowControl && this._writeBuffersPendingSize > HIGH_WATERMARK) { this.handler(FLOW_CONTROL_PAUSE); this._xoffSentToCatchUp = true; } @@ -1428,7 +1438,7 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II this._refreshEnd = this.buffer.y; this._inputHandler.parseUtf8(data); - this._watermark -= data.length; + this._writeBuffersPendingSize -= data.length; this.updateRange(this.buffer.y); this.refresh(this._refreshStart, this._refreshEnd); @@ -1439,7 +1449,7 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II } // flow control: resume pty (like XON) - if (this._xoffSentToCatchUp && this._watermark < LOW_WATERMARK) { + if (this._xoffSentToCatchUp && this._writeBuffersPendingSize < LOW_WATERMARK) { this.handler(FLOW_CONTROL_RESUME); this._xoffSentToCatchUp = false; } @@ -1463,7 +1473,7 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II * @param data The text to write to the terminal. */ public write(data: string): void { - console.log((this._watermark / 1000).toFixed(2), data.length); + console.log((this._writeBuffersPendingSize / 1000).toFixed(2), data.length); // Ensure the terminal isn't disposed if (this._isDisposed) { return; @@ -1476,15 +1486,15 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II // safety measure: dont allow the backend to crash // the terminal by writing to much data to fast. - if (this._watermark > DISCARD_WATERMARK) { + if (this._writeBuffersPendingSize > DISCARD_WATERMARK) { // FIXME: do something more useful console.error('write data discarded, use flow control to avoid losing data'); return; } // flow control: pause pty (like XOFF) - this._watermark += data.length; - if (this.options.useFlowControl && this._watermark > HIGH_WATERMARK) { + this._writeBuffersPendingSize += data.length; + if (this.options.useFlowControl && this._writeBuffersPendingSize > HIGH_WATERMARK) { this.handler(FLOW_CONTROL_PAUSE); this._xoffSentToCatchUp = true; } @@ -1516,7 +1526,7 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II this._refreshEnd = this.buffer.y; this._inputHandler.parse(data); - this._watermark -= data.length; + this._writeBuffersPendingSize -= data.length; this.updateRange(this.buffer.y); this.refresh(this._refreshStart, this._refreshEnd); @@ -1527,7 +1537,7 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II } // flow control: resume pty (like XON) - if (this._xoffSentToCatchUp && this._watermark < LOW_WATERMARK) { + if (this._xoffSentToCatchUp && this._writeBuffersPendingSize < LOW_WATERMARK) { this.handler(FLOW_CONTROL_RESUME); this._xoffSentToCatchUp = false; } From e366e1b25340f70c442a61b2f5a7cebee02261f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Thu, 30 May 2019 15:56:23 +0200 Subject: [PATCH 08/14] remove logs from demo --- demo/server.js | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/demo/server.js b/demo/server.js index 6408ee55b1..74410c5c9e 100644 --- a/demo/server.js +++ b/demo/server.js @@ -14,8 +14,7 @@ function startServer() { var app = express(); expressWs(app); - var terminals = {}, - logs = {}; + var terminals = {}; app.use('/src', express.static(__dirname + '/../src')); app.get('/logo.png', (req, res) => res.sendFile(__dirname + '/logo.png')); @@ -53,10 +52,6 @@ function startServer() { console.log('Created terminal with PID: ' + term.pid); terminals[term.pid] = term; - logs[term.pid] = ''; - //term.on('data', function(data) { - // logs[term.pid] += data; - //}); res.send(term.pid.toString()); res.end(); }); @@ -75,7 +70,6 @@ function startServer() { app.ws('/terminals/:pid', function (ws, req) { var term = terminals[parseInt(req.params.pid)]; console.log('Connected to terminal ' + term.pid); - //ws.send(logs[term.pid]); // string message buffering function buffer(socket, timeout, limit) { @@ -125,7 +119,6 @@ function startServer() { term.on('data', function(data) { try { - console.log(data.length); send(data); } catch (ex) { // The WebSocket is not open, ignore @@ -139,7 +132,6 @@ function startServer() { console.log('Closed terminal ' + term.pid); // Clean things up delete terminals[term.pid]; - delete logs[term.pid]; }); }); From adf04885c62454db723697bc5aab75e854a8b830 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Thu, 30 May 2019 15:59:22 +0200 Subject: [PATCH 09/14] remove magic numbers --- demo/server.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/demo/server.js b/demo/server.js index 74410c5c9e..3e9bb81567 100644 --- a/demo/server.js +++ b/demo/server.js @@ -9,6 +9,10 @@ var pty = require('node-pty'); */ const USE_BINARY_UTF8 = false; +// buffering +const MAX_SEND_INTERVAL = 5; +const MAX_CHUNK_SIZE = 16384; + function startServer() { var app = express(); @@ -115,7 +119,9 @@ function startServer() { } }; } - const send = USE_BINARY_UTF8 ? bufferUtf8(ws, 5, 16384) : buffer(ws, 5, 16384); + const send = USE_BINARY_UTF8 + ? bufferUtf8(ws, MAX_SEND_INTERVAL, MAX_CHUNK_SIZE) + : buffer(ws, MAX_SEND_INTERVAL, MAX_CHUNK_SIZE); term.on('data', function(data) { try { From cdf9753b217b4625a1506805f7483a065bf641f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Thu, 30 May 2019 16:10:46 +0200 Subject: [PATCH 10/14] throw error in discard limit --- src/Terminal.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Terminal.ts b/src/Terminal.ts index 4bfa64c7f9..ae0e3d693d 100644 --- a/src/Terminal.ts +++ b/src/Terminal.ts @@ -1398,10 +1398,10 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II // safety measure: dont allow the backend to crash // the terminal by writing to much data to fast. + // If we hit this, the terminal cant keep up with data written + // and will start to degenerate. if (this._writeBuffersPendingSize > DISCARD_WATERMARK) { - // FIXME: do something more useful - console.error('write data discarded, use flow control to avoid losing data'); - return; + throw new Error('write data discarded, use flow control to avoid losing data'); } // flow control: pause pty (like XOFF) @@ -1486,10 +1486,10 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II // safety measure: dont allow the backend to crash // the terminal by writing to much data to fast. + // If we hit this, the terminal cant keep up with data written + // and will start to degenerate. if (this._writeBuffersPendingSize > DISCARD_WATERMARK) { - // FIXME: do something more useful - console.error('write data discarded, use flow control to avoid losing data'); - return; + throw new Error('write data discarded, use flow control to avoid losing data'); } // flow control: pause pty (like XOFF) From 4460c6a70787fc5ebc6f820d0ab7c7728f4970fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Thu, 30 May 2019 17:58:23 +0200 Subject: [PATCH 11/14] simplify demo --- demo/server.js | 35 +++++++++++++++++------------------ src/Terminal.ts | 2 +- 2 files changed, 18 insertions(+), 19 deletions(-) diff --git a/demo/server.js b/demo/server.js index 3e9bb81567..da90e5d270 100644 --- a/demo/server.js +++ b/demo/server.js @@ -9,7 +9,7 @@ var pty = require('node-pty'); */ const USE_BINARY_UTF8 = false; -// buffering +// pty --> websocket buffering const MAX_SEND_INTERVAL = 5; const MAX_CHUNK_SIZE = 16384; @@ -75,20 +75,27 @@ function startServer() { var term = terminals[parseInt(req.params.pid)]; console.log('Connected to terminal ' + term.pid); + const _send = data => { + // handle only 'open' websocket state + if (ws.readyState === 1) { + ws.send(data); + } + } + // string message buffering - function buffer(socket, timeout, limit) { + function buffer(timeout, limit) { let s = ''; let sender = null; return (data) => { s += data; if (s.length > limit) { clearTimeout(sender); - socket.send(s); + _send(s); s = ''; sender = null; } else if (!sender) { sender = setTimeout(() => { - socket.send(s); + _send(s); s = ''; sender = null; }, timeout); @@ -96,7 +103,7 @@ function startServer() { }; } // binary message buffering - function bufferUtf8(socket, timeout, limit) { + function bufferUtf8(timeout, limit) { let buffer = []; let sender = null; let length = 0; @@ -105,13 +112,13 @@ function startServer() { length += data.length; if (length > limit) { clearTimeout(sender); - socket.send(Buffer.concat(buffer, length)); + _send(Buffer.concat(buffer, length)); buffer = []; sender = null; length = 0; } else if (!sender) { sender = setTimeout(() => { - socket.send(Buffer.concat(buffer, length)); + _send(Buffer.concat(buffer, length)); buffer = []; sender = null; length = 0; @@ -119,17 +126,9 @@ function startServer() { } }; } - const send = USE_BINARY_UTF8 - ? bufferUtf8(ws, MAX_SEND_INTERVAL, MAX_CHUNK_SIZE) - : buffer(ws, MAX_SEND_INTERVAL, MAX_CHUNK_SIZE); - - term.on('data', function(data) { - try { - send(data); - } catch (ex) { - // The WebSocket is not open, ignore - } - }); + + term.on('data', (USE_BINARY_UTF8 ? bufferUtf8 : buffer)(MAX_SEND_INTERVAL, MAX_CHUNK_SIZE)); + ws.on('message', function(msg) { term.write(msg); }); diff --git a/src/Terminal.ts b/src/Terminal.ts index ae0e3d693d..8ffe9920f5 100644 --- a/src/Terminal.ts +++ b/src/Terminal.ts @@ -1473,7 +1473,7 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II * @param data The text to write to the terminal. */ public write(data: string): void { - console.log((this._writeBuffersPendingSize / 1000).toFixed(2), data.length); + console.log((this._writeBuffersPendingSize / 1000000).toFixed(2), data.length); // Ensure the terminal isn't disposed if (this._isDisposed) { return; From 1bcf2c5411da62e199b132b09486817fe501b275 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Fri, 31 May 2019 20:41:55 +0200 Subject: [PATCH 12/14] better fast_producer --- fast_producer.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/fast_producer.c b/fast_producer.c index 50f3760742..386a70ae47 100644 --- a/fast_producer.c +++ b/fast_producer.c @@ -1,8 +1,29 @@ #include #include +#include + +static char MSG[10][10] = { + { [0 ... 8] = '0', '\n' }, + { [0 ... 8] = '1', '\n' }, + { [0 ... 8] = '2', '\n' }, + { [0 ... 8] = '3', '\n' }, + { [0 ... 8] = '4', '\n' }, + { [0 ... 8] = '5', '\n' }, + { [0 ... 8] = '6', '\n' }, + { [0 ... 8] = '7', '\n' }, + { [0 ... 8] = '8', '\n' }, + { [0 ... 8] = '9', '\n' }, +}; +static char ALL[60000]; int main(int argc, char **argv) { + int i, offset = 0; + // fill 10kB buffer + for (i=0; i<600; ++i) { + memcpy(ALL+i*100, MSG, 100); + } + // copy buffer data as fast as possible (~6GB/s on my machine) while (1) { - putchar('#'); + write(1, ALL, 60000); } } From 4faedd63e143d0e385132ba2a9653f660d5310c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Fri, 31 May 2019 22:21:07 +0200 Subject: [PATCH 13/14] ACK counting approach --- demo/server.js | 31 ++++++++++++++++++++++++++++--- fast_producer.c | 4 ++-- src/Terminal.ts | 14 +++++++++++++- 3 files changed, 43 insertions(+), 6 deletions(-) diff --git a/demo/server.js b/demo/server.js index da90e5d270..505e404f28 100644 --- a/demo/server.js +++ b/demo/server.js @@ -78,10 +78,16 @@ function startServer() { const _send = data => { // handle only 'open' websocket state if (ws.readyState === 1) { - ws.send(data); + setTimeout(() => ws.send(data), 200); } } + const ACK_WATERMARK = 131072;//524288; + const FLOW_CONTROL_ACK = '\x1b^ack\x1b\\'; // PM ack ST + const MAX_ACK_DIFF = 7; + let ack_expected = 0; + let sent = 0; + // string message buffering function buffer(timeout, limit) { let s = ''; @@ -126,10 +132,29 @@ function startServer() { } }; } - - term.on('data', (USE_BINARY_UTF8 ? bufferUtf8 : buffer)(MAX_SEND_INTERVAL, MAX_CHUNK_SIZE)); + const send = (USE_BINARY_UTF8 ? bufferUtf8 : buffer)(MAX_SEND_INTERVAL, MAX_CHUNK_SIZE); + + term.on('data', data => { + send(data); + sent += data.length; + if (sent > ACK_WATERMARK) { + ack_expected++; + sent -= ACK_WATERMARK; + if (ack_expected > MAX_ACK_DIFF) { + term.pause(); + } + } + }); ws.on('message', function(msg) { + //console.log([msg, sent]); + if (msg === FLOW_CONTROL_ACK) { + ack_expected--; + if (ack_expected <= MAX_ACK_DIFF) { + term.resume(); + } + return; + } term.write(msg); }); ws.on('close', function () { diff --git a/fast_producer.c b/fast_producer.c index 386a70ae47..f90a15af55 100644 --- a/fast_producer.c +++ b/fast_producer.c @@ -17,8 +17,8 @@ static char MSG[10][10] = { static char ALL[60000]; int main(int argc, char **argv) { - int i, offset = 0; - // fill 10kB buffer + int i; + // fill 60kB buffer for (i=0; i<600; ++i) { memcpy(ALL+i*100, MSG, 100); } diff --git a/src/Terminal.ts b/src/Terminal.ts index 8ffe9920f5..a3c8e47231 100644 --- a/src/Terminal.ts +++ b/src/Terminal.ts @@ -68,6 +68,10 @@ const document = (typeof window !== 'undefined') ? window.document : null; * a much lower number (>500 kB). */ const DISCARD_WATERMARK = 50000000; // ~50 MB +/** + * send ACK every ACK_WATERMARK-th byte + */ +const ACK_WATERMARK = 131072;//524288; // 2^19 /** * Flow control watermarks for the write buffer. @@ -78,10 +82,11 @@ const LOW_WATERMARK = 32768; const HIGH_WATERMARK = 131072; /** - * Flow control PAUSE/RESUME messages. + * Flow control PAUSE/RESUME/ACK messages. */ const FLOW_CONTROL_PAUSE = '\x1b^pause\x1b\\'; // PM pause ST const FLOW_CONTROL_RESUME = '\x1b^resume\x1b\\'; // PM resume ST +const FLOW_CONTROL_ACK = '\x1b^ack\x1b\\'; // PM ack ST /** * The max number of ms to spend on writes before allowing the renderer to @@ -212,6 +217,7 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II * doubled (JSString char takes 2 bytes). */ private _writeBuffersPendingSize: number = 0; + private _ackWatermark: number = 0; /** * Whether _xterm.js_ sent XOFF in order to catch up with the pty process. @@ -1386,6 +1392,7 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II * @param data UintArray with UTF8 bytes to write to the terminal. */ public writeUtf8(data: Uint8Array): void { + console.log((this._writeBuffersPendingSize / 1000000).toFixed(2), data.length); // Ensure the terminal isn't disposed if (this._isDisposed) { return; @@ -1527,6 +1534,7 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II this._inputHandler.parse(data); this._writeBuffersPendingSize -= data.length; + this._ackWatermark += data.length; this.updateRange(this.buffer.y); this.refresh(this._refreshStart, this._refreshEnd); @@ -1541,6 +1549,10 @@ export class Terminal extends EventEmitter implements ITerminal, IDisposable, II this.handler(FLOW_CONTROL_RESUME); this._xoffSentToCatchUp = false; } + if (this._ackWatermark > ACK_WATERMARK) { + setTimeout(() => this.handler(FLOW_CONTROL_ACK), 200); + this._ackWatermark -= ACK_WATERMARK; + } if (this.writeBuffer.length > bufferOffset) { // Allow renderer to catch up before processing the next batch From cb2b7452ebc579abe6db7844737099368460c9e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Mon, 10 Jun 2019 16:35:16 +0200 Subject: [PATCH 14/14] make linter happy --- src/Terminal.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Terminal.ts b/src/Terminal.ts index c415b61c46..75618d3e14 100644 --- a/src/Terminal.ts +++ b/src/Terminal.ts @@ -72,7 +72,7 @@ const DISCARD_WATERMARK = 50000000; // ~50 MB /** * send ACK every ACK_WATERMARK-th byte */ -const ACK_WATERMARK = 131072;//524288; // 2^19 +const ACK_WATERMARK = 131072; // 524288; // 2^19 /** * Flow control watermarks for the write buffer.