From c60c6c1c57e746bc46c28b335340ae8007e9f475 Mon Sep 17 00:00:00 2001 From: Konstantin Solomatov Date: Thu, 7 Nov 2019 17:41:31 -0800 Subject: [PATCH 1/7] Fix bug with hanged pseudoterminal --- src/vs/workbench/api/common/extHostTerminalService.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/vs/workbench/api/common/extHostTerminalService.ts b/src/vs/workbench/api/common/extHostTerminalService.ts index 40ecebf643b21..3c7effe75cd7d 100644 --- a/src/vs/workbench/api/common/extHostTerminalService.ts +++ b/src/vs/workbench/api/common/extHostTerminalService.ts @@ -456,14 +456,18 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ // Processes should be initialized here for normal virtual process terminals, however for // tasks they are responsible for attaching the virtual process to a terminal so this // function may be called before tasks is able to attach to the terminal. - let retries = 5; + let retries = 8; + let currentTimeout = 50; while (retries-- > 0) { if (this._terminalProcesses[id]) { (this._terminalProcesses[id] as ExtHostPseudoterminal).startSendingEvents(initialDimensions); return; } - await timeout(50); + await timeout(currentTimeout); + currentTimeout *= 2; } + + throw new Error('Wasn\'t able to start extension terminal'); } protected _setupExtHostProcessListeners(id: number, p: ITerminalChildProcess): void { From bde2d42430da6ce46ac2b5cefb4aeca728df2abb Mon Sep 17 00:00:00 2001 From: Konstantin Solomatov Date: Fri, 8 Nov 2019 10:12:41 -0800 Subject: [PATCH 2/7] Code review comments --- .../api/common/extHostTerminalService.ts | 23 ++++++++----------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/src/vs/workbench/api/common/extHostTerminalService.ts b/src/vs/workbench/api/common/extHostTerminalService.ts index 3c7effe75cd7d..60e3badcb1038 100644 --- a/src/vs/workbench/api/common/extHostTerminalService.ts +++ b/src/vs/workbench/api/common/extHostTerminalService.ts @@ -271,6 +271,7 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ protected _activeTerminal: ExtHostTerminal | undefined; protected _terminals: ExtHostTerminal[] = []; protected _terminalProcesses: { [id: number]: ITerminalChildProcess } = {}; + protected _initialDimensions: { [id: number]: ITerminalDimensionsDto } = {}; protected _getTerminalPromises: { [id: number]: Promise } = {}; public get activeTerminal(): ExtHostTerminal | undefined { return this._activeTerminal; } @@ -453,21 +454,9 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ } await openPromise; - // Processes should be initialized here for normal virtual process terminals, however for - // tasks they are responsible for attaching the virtual process to a terminal so this - // function may be called before tasks is able to attach to the terminal. - let retries = 8; - let currentTimeout = 50; - while (retries-- > 0) { - if (this._terminalProcesses[id]) { - (this._terminalProcesses[id] as ExtHostPseudoterminal).startSendingEvents(initialDimensions); - return; - } - await timeout(currentTimeout); - currentTimeout *= 2; + if (initialDimensions) { + this._initialDimensions[id] = initialDimensions; } - - throw new Error('Wasn\'t able to start extension terminal'); } protected _setupExtHostProcessListeners(id: number, p: ITerminalChildProcess): void { @@ -482,6 +471,12 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ p.onProcessOverrideDimensions(e => this._proxy.$sendOverrideDimensions(id, e)); } this._terminalProcesses[id] = p; + + + if (p instanceof ExtHostPseudoterminal) { + p.startSendingEvents(this._initialDimensions[id]); + delete this._initialDimensions[id]; + } } public $acceptProcessInput(id: number, data: string): void { From ed52435fe5d878fbccada2e4998fc89e9865d7de Mon Sep 17 00:00:00 2001 From: Konstantin Solomatov Date: Fri, 8 Nov 2019 11:29:09 -0800 Subject: [PATCH 3/7] Fixing tests --- src/vs/workbench/api/common/extHostTerminalService.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/vs/workbench/api/common/extHostTerminalService.ts b/src/vs/workbench/api/common/extHostTerminalService.ts index 60e3badcb1038..addaa879b71e0 100644 --- a/src/vs/workbench/api/common/extHostTerminalService.ts +++ b/src/vs/workbench/api/common/extHostTerminalService.ts @@ -454,9 +454,14 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ } await openPromise; - if (initialDimensions) { - this._initialDimensions[id] = initialDimensions; + if (this._terminalProcesses[id]) { + (this._terminalProcesses[id] as ExtHostPseudoterminal).startSendingEvents(initialDimensions); + } else { + if (initialDimensions) { + this._initialDimensions[id] = initialDimensions; + } } + } protected _setupExtHostProcessListeners(id: number, p: ITerminalChildProcess): void { @@ -472,7 +477,6 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ } this._terminalProcesses[id] = p; - if (p instanceof ExtHostPseudoterminal) { p.startSendingEvents(this._initialDimensions[id]); delete this._initialDimensions[id]; From 794ed4ff1c0d4e538749511964e89e5ece060c9b Mon Sep 17 00:00:00 2001 From: Konstantin Solomatov Date: Fri, 8 Nov 2019 11:55:23 -0800 Subject: [PATCH 4/7] Fixed test --- src/vs/workbench/api/common/extHostTerminalService.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/vs/workbench/api/common/extHostTerminalService.ts b/src/vs/workbench/api/common/extHostTerminalService.ts index addaa879b71e0..69c102e518f0e 100644 --- a/src/vs/workbench/api/common/extHostTerminalService.ts +++ b/src/vs/workbench/api/common/extHostTerminalService.ts @@ -272,6 +272,7 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ protected _terminals: ExtHostTerminal[] = []; protected _terminalProcesses: { [id: number]: ITerminalChildProcess } = {}; protected _initialDimensions: { [id: number]: ITerminalDimensionsDto } = {}; + protected _startedExtensionTerminal: { [id: number]: boolean } = {}; protected _getTerminalPromises: { [id: number]: Promise } = {}; public get activeTerminal(): ExtHostTerminal | undefined { return this._activeTerminal; } @@ -432,6 +433,7 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ public async $startExtensionTerminal(id: number, initialDimensions: ITerminalDimensionsDto | undefined): Promise { // Make sure the ExtHostTerminal exists so onDidOpenTerminal has fired before we call // Pseudoterminal.start + const terminal = await this._getTerminalByIdEventually(id); if (!terminal) { return; @@ -454,6 +456,7 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ } await openPromise; + this._startedExtensionTerminal[id] = true; if (this._terminalProcesses[id]) { (this._terminalProcesses[id] as ExtHostPseudoterminal).startSendingEvents(initialDimensions); } else { @@ -477,7 +480,7 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ } this._terminalProcesses[id] = p; - if (p instanceof ExtHostPseudoterminal) { + if (p instanceof ExtHostPseudoterminal && this._startedExtensionTerminal[id]) { p.startSendingEvents(this._initialDimensions[id]); delete this._initialDimensions[id]; } @@ -519,6 +522,7 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ // Remove process reference delete this._terminalProcesses[id]; + delete this._startedExtensionTerminal[id]; // Send exit event to main side this._proxy.$sendProcessExit(id, exitCode); From 6d7f763548b1e161cf12727cb637bd25423ffcdd Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Sun, 17 Nov 2019 12:52:17 -0800 Subject: [PATCH 5/7] Only set _extensionTerminalAwaitingStart when deferred start is needed --- .../api/common/extHostTerminalService.ts | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/vs/workbench/api/common/extHostTerminalService.ts b/src/vs/workbench/api/common/extHostTerminalService.ts index 94b1782e588b3..b26797bdef4bf 100644 --- a/src/vs/workbench/api/common/extHostTerminalService.ts +++ b/src/vs/workbench/api/common/extHostTerminalService.ts @@ -288,8 +288,8 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ protected _activeTerminal: ExtHostTerminal | undefined; protected _terminals: ExtHostTerminal[] = []; protected _terminalProcesses: { [id: number]: ITerminalChildProcess } = {}; - protected _initialDimensions: { [id: number]: ITerminalDimensionsDto } = {}; - protected _startedExtensionTerminal: { [id: number]: boolean } = {}; + protected _initialDimensions: { [id: number]: ITerminalDimensionsDto | undefined } = {}; + protected _extensionTerminalAwaitingStart: { [id: number]: boolean | undefined } = {}; protected _getTerminalPromises: { [id: number]: Promise } = {}; public get activeTerminal(): ExtHostTerminal | undefined { return this._activeTerminal; } @@ -465,13 +465,12 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ } await openPromise; - this._startedExtensionTerminal[id] = true; if (this._terminalProcesses[id]) { (this._terminalProcesses[id] as ExtHostPseudoterminal).startSendingEvents(initialDimensions); } else { - if (initialDimensions) { - this._initialDimensions[id] = initialDimensions; - } + // Defer startSendingEvents call to when _setupExtHostProcessListeners is called + this._extensionTerminalAwaitingStart[id] = true; + this._initialDimensions[id] = initialDimensions; } } @@ -489,7 +488,7 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ } this._terminalProcesses[id] = p; - if (p instanceof ExtHostPseudoterminal && this._startedExtensionTerminal[id]) { + if (this._extensionTerminalAwaitingStart[id] && p instanceof ExtHostPseudoterminal) { p.startSendingEvents(this._initialDimensions[id]); delete this._initialDimensions[id]; } @@ -531,7 +530,7 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ // Remove process reference delete this._terminalProcesses[id]; - delete this._startedExtensionTerminal[id]; + delete this._extensionTerminalAwaitingStart[id]; // Send exit event to main side this._proxy.$sendProcessExit(id, exitCode); From f329ef375319595ef6a9468784a2cce0ffcfb601 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Sun, 17 Nov 2019 12:54:38 -0800 Subject: [PATCH 6/7] Merge dimensions into awaiting object --- .../workbench/api/common/extHostTerminalService.ts | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/vs/workbench/api/common/extHostTerminalService.ts b/src/vs/workbench/api/common/extHostTerminalService.ts index b26797bdef4bf..02589872e0a0b 100644 --- a/src/vs/workbench/api/common/extHostTerminalService.ts +++ b/src/vs/workbench/api/common/extHostTerminalService.ts @@ -288,8 +288,7 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ protected _activeTerminal: ExtHostTerminal | undefined; protected _terminals: ExtHostTerminal[] = []; protected _terminalProcesses: { [id: number]: ITerminalChildProcess } = {}; - protected _initialDimensions: { [id: number]: ITerminalDimensionsDto | undefined } = {}; - protected _extensionTerminalAwaitingStart: { [id: number]: boolean | undefined } = {}; + protected _extensionTerminalAwaitingStart: { [id: number]: { initialDimensions: ITerminalDimensionsDto | undefined } | undefined } = {}; protected _getTerminalPromises: { [id: number]: Promise } = {}; public get activeTerminal(): ExtHostTerminal | undefined { return this._activeTerminal; } @@ -469,8 +468,7 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ (this._terminalProcesses[id] as ExtHostPseudoterminal).startSendingEvents(initialDimensions); } else { // Defer startSendingEvents call to when _setupExtHostProcessListeners is called - this._extensionTerminalAwaitingStart[id] = true; - this._initialDimensions[id] = initialDimensions; + this._extensionTerminalAwaitingStart[id] = { initialDimensions }; } } @@ -488,9 +486,10 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ } this._terminalProcesses[id] = p; - if (this._extensionTerminalAwaitingStart[id] && p instanceof ExtHostPseudoterminal) { - p.startSendingEvents(this._initialDimensions[id]); - delete this._initialDimensions[id]; + const awaitingStart = this._extensionTerminalAwaitingStart[id]; + if (awaitingStart && p instanceof ExtHostPseudoterminal) { + p.startSendingEvents(awaitingStart.initialDimensions); + delete this._extensionTerminalAwaitingStart[id]; } } From 649ef05dfb1a7914f87b455e4308022686946271 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Sun, 17 Nov 2019 12:55:10 -0800 Subject: [PATCH 7/7] Fix ws --- src/vs/workbench/api/common/extHostTerminalService.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/vs/workbench/api/common/extHostTerminalService.ts b/src/vs/workbench/api/common/extHostTerminalService.ts index 02589872e0a0b..43e4f6bf7539a 100644 --- a/src/vs/workbench/api/common/extHostTerminalService.ts +++ b/src/vs/workbench/api/common/extHostTerminalService.ts @@ -441,7 +441,6 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ public async $startExtensionTerminal(id: number, initialDimensions: ITerminalDimensionsDto | undefined): Promise { // Make sure the ExtHostTerminal exists so onDidOpenTerminal has fired before we call // Pseudoterminal.start - const terminal = await this._getTerminalByIdEventually(id); if (!terminal) { return;