From fa41f5c6501d27f6cbe9a3eeba595aed7a38caba Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 8 Dec 2017 14:01:55 -0800 Subject: [PATCH 1/4] Initial fix for high sierra term kill issue --- src/unixTerminal.ts | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/src/unixTerminal.ts b/src/unixTerminal.ts index 2f365eb3..abdd825b 100644 --- a/src/unixTerminal.ts +++ b/src/unixTerminal.ts @@ -71,14 +71,35 @@ export class UnixTerminal extends Terminal { const encoding = (opt.encoding === undefined ? 'utf8' : opt.encoding); const onexit = (code: any, signal: any) => { + console.log('onexit', code, signal); // XXX Sometimes a data event is emitted after exit. Wait til socket is // destroyed. if (!this._emittedClose) { - if (this._boundClose) return; + console.log('!this._emittedClose'); + if (this._boundClose) { + return; + } this._boundClose = true; - this.once('close', () => this.emit('exit', code, signal)); + this.once('close', () => { + console.log('once close exit', code, signal); + this.emit('exit', code, signal); + }); + // From macOS High Sierra 10.13.2 sometimes the socket never gets + // closed. A timeout is applied here to avoid the terminal never being + // destroyed when this occurs. + setTimeout(() => { + if (this._boundClose) { + return; + } + // this._boundClose = true; + console.log('timeout hit!, destroy socket'); + this._socket.destroy(); + // TODO: clearTimeout when 'close' is fired? + // this.emit('exit', code, signal); + }, 1000); return; } + console.log('emit exit', code, signal); this.emit('exit', code, signal); }; @@ -92,6 +113,7 @@ export class UnixTerminal extends Terminal { // setup this._socket.on('error', (err: any) => { + console.log('error', err); // NOTE: fs.ReadStream gets EAGAIN twice at first: if (err.code) { if (~err.code.indexOf('EAGAIN')) { @@ -104,6 +126,7 @@ export class UnixTerminal extends Terminal { // EIO on exit from fs.ReadStream: if (!this._emittedClose) { this._emittedClose = true; + console.log('emit close in socket error handler'); this.emit('close'); } @@ -134,11 +157,13 @@ export class UnixTerminal extends Terminal { this._writable = true; this._socket.on('close', () => { + console.log('socket close'); if (this._emittedClose) { return; } this._emittedClose = true; this._close(); + console.log('emit close in socket close handler'); this.emit('close'); }); } From fd9bd3390c2c996472094a5636fa99452ea6ab6f Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 8 Dec 2017 15:42:48 -0800 Subject: [PATCH 2/4] Twweak timeout fix --- src/unixTerminal.ts | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/unixTerminal.ts b/src/unixTerminal.ts index abdd825b..297b2e25 100644 --- a/src/unixTerminal.ts +++ b/src/unixTerminal.ts @@ -80,23 +80,24 @@ export class UnixTerminal extends Terminal { return; } this._boundClose = true; - this.once('close', () => { - console.log('once close exit', code, signal); - this.emit('exit', code, signal); - }); // From macOS High Sierra 10.13.2 sometimes the socket never gets // closed. A timeout is applied here to avoid the terminal never being // destroyed when this occurs. - setTimeout(() => { - if (this._boundClose) { - return; - } + let timeout = setTimeout(() => { + timeout = null; // this._boundClose = true; console.log('timeout hit!, destroy socket'); this._socket.destroy(); // TODO: clearTimeout when 'close' is fired? // this.emit('exit', code, signal); }, 1000); + this.once('close', () => { + if (timeout !== null) { + window.clearTimeout(timeout); + } + console.log('once close exit', code, signal); + this.emit('exit', code, signal); + }); return; } console.log('emit exit', code, signal); From 0a6d921a2f57ab7cd179f78ae2b90d0e4bfcf5b5 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 8 Dec 2017 15:51:04 -0800 Subject: [PATCH 3/4] Clean up --- src/unixTerminal.ts | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/src/unixTerminal.ts b/src/unixTerminal.ts index 297b2e25..6020b7ca 100644 --- a/src/unixTerminal.ts +++ b/src/unixTerminal.ts @@ -71,11 +71,9 @@ export class UnixTerminal extends Terminal { const encoding = (opt.encoding === undefined ? 'utf8' : opt.encoding); const onexit = (code: any, signal: any) => { - console.log('onexit', code, signal); // XXX Sometimes a data event is emitted after exit. Wait til socket is // destroyed. if (!this._emittedClose) { - console.log('!this._emittedClose'); if (this._boundClose) { return; } @@ -85,22 +83,17 @@ export class UnixTerminal extends Terminal { // destroyed when this occurs. let timeout = setTimeout(() => { timeout = null; - // this._boundClose = true; - console.log('timeout hit!, destroy socket'); + // Destroying the socket now will cause the close event to fire this._socket.destroy(); - // TODO: clearTimeout when 'close' is fired? - // this.emit('exit', code, signal); }, 1000); this.once('close', () => { if (timeout !== null) { - window.clearTimeout(timeout); + clearTimeout(timeout); } - console.log('once close exit', code, signal); this.emit('exit', code, signal); }); return; } - console.log('emit exit', code, signal); this.emit('exit', code, signal); }; @@ -114,7 +107,6 @@ export class UnixTerminal extends Terminal { // setup this._socket.on('error', (err: any) => { - console.log('error', err); // NOTE: fs.ReadStream gets EAGAIN twice at first: if (err.code) { if (~err.code.indexOf('EAGAIN')) { @@ -127,7 +119,6 @@ export class UnixTerminal extends Terminal { // EIO on exit from fs.ReadStream: if (!this._emittedClose) { this._emittedClose = true; - console.log('emit close in socket error handler'); this.emit('close'); } @@ -158,13 +149,11 @@ export class UnixTerminal extends Terminal { this._writable = true; this._socket.on('close', () => { - console.log('socket close'); if (this._emittedClose) { return; } this._emittedClose = true; this._close(); - console.log('emit close in socket close handler'); this.emit('close'); }); } From 423cad13c77d899af814f01f8e679d13ef44902e Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 8 Dec 2017 15:57:27 -0800 Subject: [PATCH 4/4] Lower timeout to 200ms --- src/unixTerminal.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/unixTerminal.ts b/src/unixTerminal.ts index 6020b7ca..e0d67f87 100644 --- a/src/unixTerminal.ts +++ b/src/unixTerminal.ts @@ -21,6 +21,7 @@ const pty = require(path.join('..', 'build', 'Release', 'pty.node')); const DEFAULT_FILE = 'sh'; const DEFAULT_NAME = 'xterm'; +const DESTROY_SOCKET_TIMEOUT_MS = 200; export class UnixTerminal extends Terminal { protected _fd: number; @@ -85,7 +86,7 @@ export class UnixTerminal extends Terminal { timeout = null; // Destroying the socket now will cause the close event to fire this._socket.destroy(); - }, 1000); + }, DESTROY_SOCKET_TIMEOUT_MS); this.once('close', () => { if (timeout !== null) { clearTimeout(timeout);