Skip to content

Commit

Permalink
Revert "Kill processes immediately on shutdown, use SIGTERM"
Browse files Browse the repository at this point in the history
This reverts commit 95c0e07.
  • Loading branch information
bpasero committed Aug 28, 2018
1 parent ba7f67a commit 1f7ce42
Show file tree
Hide file tree
Showing 10 changed files with 27 additions and 50 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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(immediate => this._proxy.$acceptProcessShutdown(request.proxy.terminalId, immediate));
request.proxy.onShutdown(() => this._proxy.$acceptProcessShutdown(request.proxy.terminalId));
}

public $sendProcessTitle(terminalId: number, title: string): void {
Expand Down
2 changes: 1 addition & 1 deletion src/vs/workbench/api/node/extHost.protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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, immediate: boolean): void;
$acceptProcessShutdown(id: number): void;
}

export interface ExtHostSCMShape {
Expand Down
4 changes: 2 additions & 2 deletions src/vs/workbench/api/node/extHostTerminalService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -415,8 +415,8 @@ export class ExtHostTerminalService implements ExtHostTerminalServiceShape {
}
}

public $acceptProcessShutdown(id: number, immediate: boolean): void {
this._terminalProcesses[id].shutdown(immediate);
public $acceptProcessShutdown(id: number): void {
this._terminalProcesses[id].shutdown();
}

private _onProcessExit(id: number, exitCode: number): void {
Expand Down
8 changes: 2 additions & 6 deletions src/vs/workbench/parts/terminal/common/terminal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -382,11 +382,8 @@ 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(isShuttingDown?: boolean): void;
dispose(): void;

/**
* Registers a link matcher, allowing custom link patterns to be matched and handled.
Expand Down Expand Up @@ -576,7 +573,6 @@ export interface ITerminalProcessManager extends IDisposable {
readonly onProcessExit: Event<number>;

addDisposable(disposable: IDisposable);
dispose(immediate?: boolean);
createProcess(shellLaunchConfig: IShellLaunchConfig, cols: number, rows: number);
write(data: string): void;
setDimensions(cols: number, rows: number): void;
Expand Down Expand Up @@ -612,7 +608,7 @@ export interface ITerminalProcessExtHostProxy extends IDisposable {

onInput: Event<string>;
onResize: Event<{ cols: number, rows: number }>;
onShutdown: Event<boolean>;
onShutdown: Event<void>;
}

export interface ITerminalProcessExtHostRequest {
Expand Down
2 changes: 1 addition & 1 deletion src/vs/workbench/parts/terminal/common/terminalService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ export abstract class TerminalService implements ITerminalService {

private _onShutdown(): void {
// Dispose of all instances
this.terminalInstances.forEach(instance => instance.dispose(true));
this.terminalInstances.forEach(instance => instance.dispose());
}

public getTabLabels(): string[] {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,7 @@ export class TerminalInstance implements ITerminalInstance {
this._terminalFocusContextKey.set(terminalFocused);
}

public dispose(isShuttingDown?: boolean): void {
public dispose(): void {
this._logService.trace(`terminalInstance#dispose (id: ${this.id})`);

this._windowsShellHelper = lifecycle.dispose(this._windowsShellHelper);
Expand All @@ -588,7 +588,7 @@ export class TerminalInstance implements ITerminalInstance {
this._xterm.dispose();
this._xterm = null;
}
this._processManager.dispose(isShuttingDown);
this._processManager = lifecycle.dispose(this._processManager);
if (!this._isDisposed) {
this._isDisposed = true;
this._onDisposed.fire(this);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,13 @@ export class TerminalProcessManager implements ITerminalProcessManager {
});
}

public dispose(immediate?: boolean): void {
public dispose(): 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(immediate);
this._process.shutdown();
this._process = null;
}
this._disposables.forEach(d => d.dispose());
Expand Down
8 changes: 1 addition & 7 deletions src/vs/workbench/parts/terminal/node/terminal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,7 @@ export interface ITerminalChildProcess {
onProcessIdReady: Event<number>;
onProcessTitleChanged: Event<string>;

/**
* 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;
shutdown(): void;
input(data: string): void;
resize(cols: number, rows: number): void;
}
Expand Down
34 changes: 11 additions & 23 deletions src/vs/workbench/parts/terminal/node/terminalProcess.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,25 +97,17 @@ export class TerminalProcess implements ITerminalChildProcess, IDisposable {
if (this._closeTimeout) {
clearTimeout(this._closeTimeout);
}
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._closeTimeout = setTimeout(() => {
// Attempt to kill the pty, it may have already been killed at this
// point but we want to make sure
try {
this._ptyProcess.kill();
} catch (ex) {
// Swallow, the pty has already been killed
}
} catch (ex) {
// Swallow, the pty has already been killed
}
this._onProcessExit.fire(this._exitCode);
this.dispose();
this._onProcessExit.fire(this._exitCode);
this.dispose();
}, 250);
}

private _sendProcessId() {
Expand All @@ -127,12 +119,8 @@ export class TerminalProcess implements ITerminalChildProcess, IDisposable {
this._onProcessTitleChanged.fire(this._currentTitle);
}

public shutdown(immediate: boolean): void {
if (immediate) {
this._kill();
} else {
this._queueProcessExit();
}
public shutdown(): void {
this._queueProcessExit();
}

public input(data: string): void {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ export class TerminalProcessExtHostProxy implements ITerminalChildProcess, ITerm
public get onInput(): Event<string> { 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<boolean> = new Emitter<boolean>();
public get onShutdown(): Event<boolean> { return this._onShutdown.event; }
private readonly _onShutdown: Emitter<void> = new Emitter<void>();
public get onShutdown(): Event<void> { return this._onShutdown.event; }

constructor(
public terminalId: number,
Expand Down Expand Up @@ -65,9 +65,8 @@ export class TerminalProcessExtHostProxy implements ITerminalChildProcess, ITerm
this._onProcessExit.fire(exitCode);
this.dispose();
}

public shutdown(immediate: boolean): void {
this._onShutdown.fire(immediate);
public shutdown(): void {
this._onShutdown.fire();
}

public input(data: string): void {
Expand Down

0 comments on commit 1f7ce42

Please sign in to comment.