From 90d06ab05bfe0db69009d06e502b847a9d2a9166 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Tue, 28 Aug 2018 07:30:49 -0700 Subject: [PATCH] Revert "Revert "Kill processes immediately on shutdown, use SIGTERM"" This reverts commit 1f7ce4212250529b192657c8294c52d15469b4ec. --- .../mainThreadTerminalService.ts | 2 +- src/vs/workbench/api/node/extHost.protocol.ts | 2 +- .../api/node/extHostTerminalService.ts | 4 +-- .../parts/terminal/common/terminal.ts | 8 +++-- .../parts/terminal/common/terminalService.ts | 2 +- .../electron-browser/terminalInstance.ts | 4 +-- .../terminalProcessManager.ts | 4 +-- .../workbench/parts/terminal/node/terminal.ts | 8 ++++- .../parts/terminal/node/terminalProcess.ts | 34 +++++++++++++------ .../node/terminalProcessExtHostProxy.ts | 9 ++--- 10 files changed, 50 insertions(+), 27 deletions(-) diff --git a/src/vs/workbench/api/electron-browser/mainThreadTerminalService.ts b/src/vs/workbench/api/electron-browser/mainThreadTerminalService.ts index c25e31ae085..89c4f5b559f 100644 --- a/src/vs/workbench/api/electron-browser/mainThreadTerminalService.ts +++ b/src/vs/workbench/api/electron-browser/mainThreadTerminalService.ts @@ -199,7 +199,7 @@ export class MainThreadTerminalService implements MainThreadTerminalServiceShape this._proxy.$createProcess(request.proxy.terminalId, shellLaunchConfigDto, request.cols, request.rows); request.proxy.onInput(data => this._proxy.$acceptProcessInput(request.proxy.terminalId, data)); request.proxy.onResize(dimensions => this._proxy.$acceptProcessResize(request.proxy.terminalId, dimensions.cols, dimensions.rows)); - request.proxy.onShutdown(() => this._proxy.$acceptProcessShutdown(request.proxy.terminalId)); + request.proxy.onShutdown(immediate => this._proxy.$acceptProcessShutdown(request.proxy.terminalId, immediate)); } public $sendProcessTitle(terminalId: number, title: string): void { diff --git a/src/vs/workbench/api/node/extHost.protocol.ts b/src/vs/workbench/api/node/extHost.protocol.ts index da27bbb931d..5ad7b8cc82d 100644 --- a/src/vs/workbench/api/node/extHost.protocol.ts +++ b/src/vs/workbench/api/node/extHost.protocol.ts @@ -878,7 +878,7 @@ export interface ExtHostTerminalServiceShape { $createProcess(id: number, shellLaunchConfig: ShellLaunchConfigDto, cols: number, rows: number): void; $acceptProcessInput(id: number, data: string): void; $acceptProcessResize(id: number, cols: number, rows: number): void; - $acceptProcessShutdown(id: number): void; + $acceptProcessShutdown(id: number, immediate: boolean): void; } export interface ExtHostSCMShape { diff --git a/src/vs/workbench/api/node/extHostTerminalService.ts b/src/vs/workbench/api/node/extHostTerminalService.ts index 55ecfbfe88d..65ef37f3cf4 100644 --- a/src/vs/workbench/api/node/extHostTerminalService.ts +++ b/src/vs/workbench/api/node/extHostTerminalService.ts @@ -415,8 +415,8 @@ export class ExtHostTerminalService implements ExtHostTerminalServiceShape { } } - public $acceptProcessShutdown(id: number): void { - this._terminalProcesses[id].shutdown(); + public $acceptProcessShutdown(id: number, immediate: boolean): void { + this._terminalProcesses[id].shutdown(immediate); } private _onProcessExit(id: number, exitCode: number): void { diff --git a/src/vs/workbench/parts/terminal/common/terminal.ts b/src/vs/workbench/parts/terminal/common/terminal.ts index e31b9701cc3..11b7e37ec42 100644 --- a/src/vs/workbench/parts/terminal/common/terminal.ts +++ b/src/vs/workbench/parts/terminal/common/terminal.ts @@ -382,8 +382,11 @@ export interface ITerminalInstance { /** * Dispose the terminal instance, removing it from the panel/service and freeing up resources. + * + * @param isShuttingDown Whether VS Code is shutting down, if so kill any terminal processes + * immediately. */ - dispose(): void; + dispose(isShuttingDown?: boolean): void; /** * Registers a link matcher, allowing custom link patterns to be matched and handled. @@ -573,6 +576,7 @@ export interface ITerminalProcessManager extends IDisposable { readonly onProcessExit: Event; addDisposable(disposable: IDisposable); + dispose(immediate?: boolean); createProcess(shellLaunchConfig: IShellLaunchConfig, cols: number, rows: number); write(data: string): void; setDimensions(cols: number, rows: number): void; @@ -608,7 +612,7 @@ export interface ITerminalProcessExtHostProxy extends IDisposable { onInput: Event; onResize: Event<{ cols: number, rows: number }>; - onShutdown: Event; + onShutdown: Event; } export interface ITerminalProcessExtHostRequest { diff --git a/src/vs/workbench/parts/terminal/common/terminalService.ts b/src/vs/workbench/parts/terminal/common/terminalService.ts index f4dbe8e61d7..dfa0ed4f2d9 100644 --- a/src/vs/workbench/parts/terminal/common/terminalService.ts +++ b/src/vs/workbench/parts/terminal/common/terminalService.ts @@ -113,7 +113,7 @@ export abstract class TerminalService implements ITerminalService { private _onShutdown(): void { // Dispose of all instances - this.terminalInstances.forEach(instance => instance.dispose()); + this.terminalInstances.forEach(instance => instance.dispose(true)); } public getTabLabels(): string[] { diff --git a/src/vs/workbench/parts/terminal/electron-browser/terminalInstance.ts b/src/vs/workbench/parts/terminal/electron-browser/terminalInstance.ts index ee4185d3d88..e621ec9cac6 100644 --- a/src/vs/workbench/parts/terminal/electron-browser/terminalInstance.ts +++ b/src/vs/workbench/parts/terminal/electron-browser/terminalInstance.ts @@ -563,7 +563,7 @@ export class TerminalInstance implements ITerminalInstance { this._terminalFocusContextKey.set(terminalFocused); } - public dispose(): void { + public dispose(isShuttingDown?: boolean): void { this._logService.trace(`terminalInstance#dispose (id: ${this.id})`); this._windowsShellHelper = lifecycle.dispose(this._windowsShellHelper); @@ -588,7 +588,7 @@ export class TerminalInstance implements ITerminalInstance { this._xterm.dispose(); this._xterm = null; } - this._processManager = lifecycle.dispose(this._processManager); + this._processManager.dispose(isShuttingDown); if (!this._isDisposed) { this._isDisposed = true; this._onDisposed.fire(this); diff --git a/src/vs/workbench/parts/terminal/electron-browser/terminalProcessManager.ts b/src/vs/workbench/parts/terminal/electron-browser/terminalProcessManager.ts index c4d62a87ead..0b0e480fb66 100644 --- a/src/vs/workbench/parts/terminal/electron-browser/terminalProcessManager.ts +++ b/src/vs/workbench/parts/terminal/electron-browser/terminalProcessManager.ts @@ -65,13 +65,13 @@ export class TerminalProcessManager implements ITerminalProcessManager { }); } - public dispose(): void { + public dispose(immediate?: boolean): void { if (this._process) { // If the process was still connected this dispose came from // within VS Code, not the process, so mark the process as // killed by the user. this.processState = ProcessState.KILLED_BY_USER; - this._process.shutdown(); + this._process.shutdown(immediate); this._process = null; } this._disposables.forEach(d => d.dispose()); diff --git a/src/vs/workbench/parts/terminal/node/terminal.ts b/src/vs/workbench/parts/terminal/node/terminal.ts index 8d787d74e93..7eaadc133aa 100644 --- a/src/vs/workbench/parts/terminal/node/terminal.ts +++ b/src/vs/workbench/parts/terminal/node/terminal.ts @@ -19,7 +19,13 @@ export interface ITerminalChildProcess { onProcessIdReady: Event; onProcessTitleChanged: Event; - shutdown(): void; + /** + * Shutdown the terminal process. + * + * @param immediate When true the process will be killed immediately, otherwise the process will + * be given some time to make sure no additional data comes through. + */ + shutdown(immediate: boolean): void; input(data: string): void; resize(cols: number, rows: number): void; } diff --git a/src/vs/workbench/parts/terminal/node/terminalProcess.ts b/src/vs/workbench/parts/terminal/node/terminalProcess.ts index 1eabf0d7c0d..ff218c11c80 100644 --- a/src/vs/workbench/parts/terminal/node/terminalProcess.ts +++ b/src/vs/workbench/parts/terminal/node/terminalProcess.ts @@ -97,17 +97,25 @@ export class TerminalProcess implements ITerminalChildProcess, IDisposable { if (this._closeTimeout) { clearTimeout(this._closeTimeout); } - this._closeTimeout = setTimeout(() => { - // Attempt to kill the pty, it may have already been killed at this - // point but we want to make sure - try { + this._closeTimeout = setTimeout(() => this._kill(), 250); + } + + private _kill(): void { + // Attempt to kill the pty, it may have already been killed at this + // point but we want to make sure + try { + if (!platform.isWindows) { + // Send SIGTERM, SIGHUP does not seem to work when the parent process dies + // immediately after. + process.kill(this._ptyProcess.pid, 'SIGTERM'); + } else { this._ptyProcess.kill(); - } catch (ex) { - // Swallow, the pty has already been killed } - this._onProcessExit.fire(this._exitCode); - this.dispose(); - }, 250); + } catch (ex) { + // Swallow, the pty has already been killed + } + this._onProcessExit.fire(this._exitCode); + this.dispose(); } private _sendProcessId() { @@ -119,8 +127,12 @@ export class TerminalProcess implements ITerminalChildProcess, IDisposable { this._onProcessTitleChanged.fire(this._currentTitle); } - public shutdown(): void { - this._queueProcessExit(); + public shutdown(immediate: boolean): void { + if (immediate) { + this._kill(); + } else { + this._queueProcessExit(); + } } public input(data: string): void { diff --git a/src/vs/workbench/parts/terminal/node/terminalProcessExtHostProxy.ts b/src/vs/workbench/parts/terminal/node/terminalProcessExtHostProxy.ts index cac16c5ff22..08e3144893c 100644 --- a/src/vs/workbench/parts/terminal/node/terminalProcessExtHostProxy.ts +++ b/src/vs/workbench/parts/terminal/node/terminalProcessExtHostProxy.ts @@ -25,8 +25,8 @@ export class TerminalProcessExtHostProxy implements ITerminalChildProcess, ITerm public get onInput(): Event { return this._onInput.event; } private readonly _onResize: Emitter<{ cols: number, rows: number }> = new Emitter<{ cols: number, rows: number }>(); public get onResize(): Event<{ cols: number, rows: number }> { return this._onResize.event; } - private readonly _onShutdown: Emitter = new Emitter(); - public get onShutdown(): Event { return this._onShutdown.event; } + private readonly _onShutdown: Emitter = new Emitter(); + public get onShutdown(): Event { return this._onShutdown.event; } constructor( public terminalId: number, @@ -65,8 +65,9 @@ export class TerminalProcessExtHostProxy implements ITerminalChildProcess, ITerm this._onProcessExit.fire(exitCode); this.dispose(); } - public shutdown(): void { - this._onShutdown.fire(); + + public shutdown(immediate: boolean): void { + this._onShutdown.fire(immediate); } public input(data: string): void { -- GitLab