Merge pull request #118437 from microsoft/tyriar/1_54_117956

Don't force kill ptys on Windows on window exit

Author: Tyriar

src/vs/platform/terminal/node/terminalProcess.ts

@@ -24,6 +24,25 @@ import { localize } from 'vs/nls';
 const WRITE_MAX_CHUNK_SIZE = 50;
 const WRITE_INTERVAL_MS = 5;
 
+const enum ShutdownConstants {
+	/**
+	 * The amount of time that must pass between data events after exit is queued before the actual
+	 * kill call is triggered. This data flush mechanism works around an [issue in node-pty][1]
+	 * where not all data is flushed which causes problems for task problem matchers. Additionally
+	 * on Windows under conpty, killing a process while data is being output will cause the [conhost
+	 * flush to hang the pty host][2] because [conhost should be hosted on another thread][3].
+	 *
+	 * [1]: https://github.com/Tyriar/node-pty/issues/72
+	 * [2]: https://github.com/microsoft/vscode/issues/71966
+	 * [3]: https://github.com/microsoft/node-pty/pull/415
+	 */
+	DataFlushTimeout = 250,
+	/**
+	 * The maximum time to allow after dispose is called because forcefully killing the process.
+	 */
+	MaximumShutdownTime = 5000
+}
+
 export class TerminalProcess extends Disposable implements ITerminalChildProcess {
 	readonly id = 0;
 	readonly shouldPersist = false;
@@ -223,7 +242,10 @@ export class TerminalProcess extends Disposable implements ITerminalChildProcess
 		if (this._closeTimeout) {
 			clearTimeout(this._closeTimeout);
 		}
-		this._closeTimeout = setTimeout(() => this._kill(), 250);
+		this._closeTimeout = setTimeout(() => {
+			this._closeTimeout = undefined;
+			this._kill();
+		}, ShutdownConstants.DataFlushTimeout);
 	}
 
 	private async _kill(): Promise<void> {
@@ -263,7 +285,16 @@ export class TerminalProcess extends Disposable implements ITerminalChildProcess
 		if (immediate) {
 			this._kill();
 		} else {
-			this._queueProcessExit();
+			if (!this._closeTimeout && !this._isDisposed) {
+				this._queueProcessExit();
+				// Allow a maximum amount of time for the process to exit, otherwise force kill it
+				setTimeout(() => {
+					if (this._closeTimeout && !this._isDisposed) {
+						this._closeTimeout = undefined;
+						this._kill();
+					}
+				}, ShutdownConstants.MaximumShutdownTime);
+			}
 		}
 	}
 

src/vs/workbench/contrib/terminal/browser/terminalService.ts

@@ -334,8 +334,12 @@ export class TerminalService implements ITerminalService {
 			return;
 		}
 
-		// Force dispose of all terminal instances
-		this.terminalInstances.forEach(instance => instance.dispose(true));
+		// Force dispose of all terminal instances, don't force immediate disposal of the terminal
+		// processes on Windows as an additional mitigation for https://github.com/microsoft/vscode/issues/71966
+		// which causes the pty host to become unresponsive, disconnecting all terminals across all
+		// windows
+		this.terminalInstances.forEach(instance => instance.dispose(!isWindows));
+
 		this._terminalInstanceService.setTerminalLayoutInfo(undefined);
 	}