From edadf1c7194f9121087eac8730dbca5cbdff5ef1 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Mon, 1 Mar 2021 07:46:34 -0800 Subject: [PATCH] Implement shouldPersist for all terminals processes shouldPersist was not being set on RemoteTerminalProcess, causing reconnect to fail for all remote terminals. The fix was to push the persistent logic up to where the process gets created in TerminalProcessManager just like is done with local processes. The property was made non-optional to prevent this sort of thing happening again. Part of #117896 --- src/vs/platform/terminal/common/terminal.ts | 2 +- src/vs/platform/terminal/node/terminalProcess.ts | 1 + src/vs/workbench/api/common/extHostTerminalService.ts | 1 + .../contrib/terminal/browser/remoteTerminalService.ts | 8 +++++--- src/vs/workbench/contrib/terminal/browser/terminal.ts | 2 +- .../terminal/browser/terminalProcessExtHostProxy.ts | 1 + .../contrib/terminal/browser/terminalProcessManager.ts | 5 +++-- 7 files changed, 13 insertions(+), 7 deletions(-) diff --git a/src/vs/platform/terminal/common/terminal.ts b/src/vs/platform/terminal/common/terminal.ts index 10d57ab71cc..b0d023f6e25 100644 --- a/src/vs/platform/terminal/common/terminal.ts +++ b/src/vs/platform/terminal/common/terminal.ts @@ -255,7 +255,7 @@ export interface ITerminalChildProcess { /** * Whether the process should be persisted across reloads. */ - shouldPersist?: boolean; + shouldPersist: boolean; onProcessData: Event; onProcessExit: Event; diff --git a/src/vs/platform/terminal/node/terminalProcess.ts b/src/vs/platform/terminal/node/terminalProcess.ts index 446404f7b76..91fc99c4c32 100644 --- a/src/vs/platform/terminal/node/terminalProcess.ts +++ b/src/vs/platform/terminal/node/terminalProcess.ts @@ -26,6 +26,7 @@ const WRITE_INTERVAL_MS = 5; export class TerminalProcess extends Disposable implements ITerminalChildProcess { readonly id = 0; + readonly shouldPersist = false; private _exitCode: number | undefined; private _exitMessage: string | undefined; diff --git a/src/vs/workbench/api/common/extHostTerminalService.ts b/src/vs/workbench/api/common/extHostTerminalService.ts index 014f440346d..aeac665bd64 100644 --- a/src/vs/workbench/api/common/extHostTerminalService.ts +++ b/src/vs/workbench/api/common/extHostTerminalService.ts @@ -185,6 +185,7 @@ export class ExtHostTerminal { export class ExtHostPseudoterminal implements ITerminalChildProcess { readonly id = 0; + readonly shouldPersist = false; private readonly _onProcessData = new Emitter(); public readonly onProcessData: Event = this._onProcessData.event; diff --git a/src/vs/workbench/contrib/terminal/browser/remoteTerminalService.ts b/src/vs/workbench/contrib/terminal/browser/remoteTerminalService.ts index 29035c0c009..9280347603c 100644 --- a/src/vs/workbench/contrib/terminal/browser/remoteTerminalService.ts +++ b/src/vs/workbench/contrib/terminal/browser/remoteTerminalService.ts @@ -17,6 +17,7 @@ import { IRemoteTerminalProcessExecCommandEvent, IShellLaunchConfigDto, RemoteTe import { IRemoteTerminalAttachTarget, ITerminalConfigHelper } from 'vs/workbench/contrib/terminal/common/terminal'; import { IRemoteAgentService } from 'vs/workbench/services/remote/common/remoteAgentService'; import { IProcessDataEvent, IShellLaunchConfig, ITerminalChildProcess, ITerminalDimensionsOverride, ITerminalLaunchError, ITerminalsLayoutInfo, ITerminalsLayoutInfoById } from 'vs/platform/terminal/common/terminal'; + export class RemoteTerminalService extends Disposable implements IRemoteTerminalService { public _serviceBrand: undefined; @@ -39,7 +40,7 @@ export class RemoteTerminalService extends Disposable implements IRemoteTerminal } } - public async createRemoteTerminalProcess(terminalId: number, shellLaunchConfig: IShellLaunchConfig, activeWorkspaceRootUri: URI | undefined, cols: number, rows: number, configHelper: ITerminalConfigHelper,): Promise { + public async createRemoteTerminalProcess(terminalId: number, shellLaunchConfig: IShellLaunchConfig, activeWorkspaceRootUri: URI | undefined, cols: number, rows: number, shouldPersist: boolean, configHelper: ITerminalConfigHelper): Promise { if (!this._remoteTerminalChannel) { throw new Error(`Cannot create remote terminal when there is no remote!`); } @@ -52,7 +53,7 @@ export class RemoteTerminalService extends Disposable implements IRemoteTerminal }); } - return new RemoteTerminalProcess(terminalId, shellLaunchConfig, activeWorkspaceRootUri, cols, rows, configHelper, isPreconnectionTerminal, this._remoteTerminalChannel, this._remoteAgentService, this._logService, this._commandService); + return new RemoteTerminalProcess(terminalId, shouldPersist, shellLaunchConfig, activeWorkspaceRootUri, cols, rows, configHelper, isPreconnectionTerminal, this._remoteTerminalChannel, this._remoteAgentService, this._logService, this._commandService); } public async listTerminals(isInitialization = false): Promise { @@ -108,6 +109,7 @@ export class RemoteTerminalProcess extends Disposable implements ITerminalChildP constructor( readonly id: number, + readonly shouldPersist: boolean, private readonly _shellLaunchConfig: IShellLaunchConfig, private readonly _activeWorkspaceRootUri: URI | undefined, private readonly _cols: number, @@ -154,7 +156,7 @@ export class RemoteTerminalProcess extends Disposable implements ITerminalChildP const result = await this._remoteTerminalChannel.createTerminalProcess( shellLaunchConfigDto, this._activeWorkspaceRootUri, - !this._shellLaunchConfig.isFeatureTerminal && this._configHelper.config.enablePersistentSessions, + this.shouldPersist, this._cols, this._rows, isWorkspaceShellAllowed, diff --git a/src/vs/workbench/contrib/terminal/browser/terminal.ts b/src/vs/workbench/contrib/terminal/browser/terminal.ts index 164eaa63472..2ad68dde9e0 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminal.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminal.ts @@ -200,7 +200,7 @@ export interface IRemoteTerminalService { readonly _serviceBrand: undefined; dispose(): void; listTerminals(isInitialization?: boolean): Promise; - createRemoteTerminalProcess(terminalId: number, shellLaunchConfig: IShellLaunchConfig, activeWorkspaceRootUri: URI | undefined, cols: number, rows: number, configHelper: ITerminalConfigHelper,): Promise; + createRemoteTerminalProcess(terminalId: number, shellLaunchConfig: IShellLaunchConfig, activeWorkspaceRootUri: URI | undefined, cols: number, rows: number, shouldPersist: boolean, configHelper: ITerminalConfigHelper,): Promise; setTerminalLayoutInfo(layout: ITerminalsLayoutInfoById): Promise; getTerminalLayoutInfo(): Promise; diff --git a/src/vs/workbench/contrib/terminal/browser/terminalProcessExtHostProxy.ts b/src/vs/workbench/contrib/terminal/browser/terminalProcessExtHostProxy.ts index 92f865cf81e..94ee138afff 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalProcessExtHostProxy.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalProcessExtHostProxy.ts @@ -11,6 +11,7 @@ import { ITerminalProcessExtHostProxy } from 'vs/workbench/contrib/terminal/comm export class TerminalProcessExtHostProxy extends Disposable implements ITerminalChildProcess, ITerminalProcessExtHostProxy { readonly id = 0; + readonly shouldPersist = false; private readonly _onProcessData = this._register(new Emitter()); public readonly onProcessData: Event = this._onProcessData.event; diff --git a/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts b/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts index 156340e2ab9..80dfb6d00c1 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts @@ -90,7 +90,7 @@ export class TerminalProcessManager extends Disposable implements ITerminalProce public get environmentVariableInfo(): IEnvironmentVariableInfo | undefined { return this._environmentVariableInfo; } public get persistentTerminalId(): number | undefined { return this._process?.id; } - public get shouldPersist(): boolean { return this._process?.shouldPersist || false; } + public get shouldPersist(): boolean { return this._process ? this._process.shouldPersist : false; } public get hasWrittenData(): boolean { return this._hasWrittenData; @@ -185,7 +185,8 @@ export class TerminalProcessManager extends Disposable implements ITerminalProce // this is a copy of what the merged environment collection is on the remote side await this._setupEnvVariableInfo(activeWorkspaceRootUri, shellLaunchConfig); - this._process = await this._remoteTerminalService.createRemoteTerminalProcess(this._terminalId, shellLaunchConfig, activeWorkspaceRootUri, cols, rows, this._configHelper); + const shouldPersist = !shellLaunchConfig.isFeatureTerminal && this._configHelper.config.enablePersistentSessions; + this._process = await this._remoteTerminalService.createRemoteTerminalProcess(this._terminalId, shellLaunchConfig, activeWorkspaceRootUri, cols, rows, shouldPersist, this._configHelper); } else { // Flow control is not needed for ptys hosted in the same process (ie. the electron // renderer).