From 314446f954bbc0fbb6678ce25ca60bc764dd3d82 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Thu, 11 Jun 2020 11:30:51 -0700 Subject: [PATCH 01/13] Move off deprecated node-pty on API --- .../workbench/contrib/terminal/node/terminalProcess.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/vs/workbench/contrib/terminal/node/terminalProcess.ts b/src/vs/workbench/contrib/terminal/node/terminalProcess.ts index 1c04b6137fd..6d49ecb8d38 100644 --- a/src/vs/workbench/contrib/terminal/node/terminalProcess.ts +++ b/src/vs/workbench/contrib/terminal/node/terminalProcess.ts @@ -124,15 +124,15 @@ export class TerminalProcess extends Disposable implements ITerminalChildProcess this._processStartupComplete = new Promise(c => { this.onProcessReady(() => c()); }); - ptyProcess.on('data', data => { + ptyProcess.onData(data => { this._onProcessData.fire(data); if (this._closeTimeout) { clearTimeout(this._closeTimeout); this._queueProcessExit(); } }); - ptyProcess.on('exit', code => { - this._exitCode = code; + ptyProcess.onExit(e => { + this._exitCode = e.exitCode; this._queueProcessExit(); }); this._setupTitlePolling(ptyProcess); @@ -200,8 +200,8 @@ export class TerminalProcess extends Disposable implements ITerminalChildProcess this.dispose(); } - private _sendProcessId(ptyProcess: pty.IPty) { - this._onProcessReady.fire({ pid: ptyProcess.pid, cwd: this._initialCwd }); + private _sendProcessId(pid: number) { + this._onProcessReady.fire({ pid, cwd: this._initialCwd }); } private _sendProcessTitle(ptyProcess: pty.IPty): void { From e6c745ebef0b38667f092ee97c0f6d2b4ac5bace Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 12 Jun 2020 07:02:27 -0700 Subject: [PATCH 02/13] Refactor terminal launch logic to improve error Fixes #99905 Part of #99996 --- .../api/common/extHostTerminalService.ts | 4 + .../terminal/browser/terminalInstance.ts | 78 +++++----- .../browser/terminalProcessExtHostProxy.ts | 52 ++++--- .../browser/terminalProcessManager.ts | 11 +- .../contrib/terminal/common/terminal.ts | 18 ++- .../contrib/terminal/node/terminalProcess.ts | 135 ++++++++++-------- 6 files changed, 176 insertions(+), 122 deletions(-) diff --git a/src/vs/workbench/api/common/extHostTerminalService.ts b/src/vs/workbench/api/common/extHostTerminalService.ts index c385606345f..e3c69055cdb 100644 --- a/src/vs/workbench/api/common/extHostTerminalService.ts +++ b/src/vs/workbench/api/common/extHostTerminalService.ts @@ -243,6 +243,10 @@ export class ExtHostPseudoterminal implements ITerminalChildProcess { constructor(private readonly _pty: vscode.Pseudoterminal) { } + async start(): Promise { + return undefined; + } + shutdown(): void { this._pty.close(); } diff --git a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts index ac5745e13ed..5ae66e068ca 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts @@ -25,7 +25,7 @@ import { activeContrastBorder, scrollbarSliderActiveBackground, scrollbarSliderB import { ICssStyleCollector, IColorTheme, IThemeService, registerThemingParticipant } from 'vs/platform/theme/common/themeService'; import { PANEL_BACKGROUND, SIDE_BAR_BACKGROUND } from 'vs/workbench/common/theme'; import { TerminalWidgetManager } from 'vs/workbench/contrib/terminal/browser/widgets/widgetManager'; -import { IShellLaunchConfig, ITerminalDimensions, ITerminalProcessManager, KEYBINDING_CONTEXT_TERMINAL_TEXT_SELECTED, NEVER_MEASURE_RENDER_TIME_STORAGE_KEY, ProcessState, TERMINAL_VIEW_ID, IWindowsShellHelper, SHELL_PATH_INVALID_EXIT_CODE, SHELL_PATH_DIRECTORY_EXIT_CODE, SHELL_CWD_INVALID_EXIT_CODE, KEYBINDING_CONTEXT_TERMINAL_A11Y_TREE_FOCUS, INavigationMode, TitleEventSource, LEGACY_CONSOLE_MODE_EXIT_CODE, DEFAULT_COMMANDS_TO_SKIP_SHELL } from 'vs/workbench/contrib/terminal/common/terminal'; +import { IShellLaunchConfig, ITerminalDimensions, ITerminalProcessManager, KEYBINDING_CONTEXT_TERMINAL_TEXT_SELECTED, NEVER_MEASURE_RENDER_TIME_STORAGE_KEY, ProcessState, TERMINAL_VIEW_ID, IWindowsShellHelper, KEYBINDING_CONTEXT_TERMINAL_A11Y_TREE_FOCUS, INavigationMode, TitleEventSource, LEGACY_CONSOLE_MODE_EXIT_CODE, DEFAULT_COMMANDS_TO_SKIP_SHELL, ITerminalLaunchError } from 'vs/workbench/contrib/terminal/common/terminal'; import { ansiColorIdentifiers, TERMINAL_BACKGROUND_COLOR, TERMINAL_CURSOR_BACKGROUND_COLOR, TERMINAL_CURSOR_FOREGROUND_COLOR, TERMINAL_FOREGROUND_COLOR, TERMINAL_SELECTION_BACKGROUND_COLOR } from 'vs/workbench/contrib/terminal/common/terminalColorRegistry'; import { TerminalConfigHelper } from 'vs/workbench/contrib/terminal/browser/terminalConfigHelper'; import { TerminalLinkManager } from 'vs/workbench/contrib/terminal/browser/links/terminalLinkManager'; @@ -911,9 +911,12 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { // Create the process asynchronously to allow the terminal's container // to be created so dimensions are accurate - setTimeout(() => { - this._processManager.createProcess(this._shellLaunchConfig, this._cols, this._rows, this._accessibilityService.isScreenReaderOptimized()); - }, 0); + this._processManager.createProcess(this._shellLaunchConfig, this._cols, this._rows, this._accessibilityService.isScreenReaderOptimized()).then(error => { + if (error) { + // TODO: Tear down + this._onProcessExit(error); + } + }); } private getShellType(executable: string): TerminalShellType { @@ -948,48 +951,53 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { * @param exitCode The exit code of the process, this is undefined when the terminal was exited * through user action. */ - private _onProcessExit(exitCode?: number): void { + private _onProcessExit(exitCodeOrError?: number | ITerminalLaunchError): void { // Prevent dispose functions being triggered multiple times if (this._isExiting) { return; } - this._logService.debug(`Terminal process exit (id: ${this.id}) with code ${exitCode}`); + this._logService.debug(`Terminal process exit (id: ${this.id}) with code ${this._exitCode}`); - this._exitCode = exitCode; this._isExiting = true; let exitCodeMessage: string | undefined; // Create exit code message - if (exitCode) { - if (exitCode === SHELL_PATH_INVALID_EXIT_CODE) { - exitCodeMessage = nls.localize('terminal.integrated.exitedWithInvalidPath', 'The terminal shell path "{0}" does not exist', this._shellLaunchConfig.executable); - } else if (exitCode === SHELL_PATH_DIRECTORY_EXIT_CODE) { - exitCodeMessage = nls.localize('terminal.integrated.exitedWithInvalidPathDirectory', 'The terminal shell path "{0}" is a directory', this._shellLaunchConfig.executable); - } else if (exitCode === SHELL_CWD_INVALID_EXIT_CODE && this._shellLaunchConfig.cwd) { - exitCodeMessage = nls.localize('terminal.integrated.exitedWithInvalidCWD', 'The terminal shell CWD "{0}" does not exist', this._shellLaunchConfig.cwd.toString()); - } else if (exitCode === LEGACY_CONSOLE_MODE_EXIT_CODE) { - exitCodeMessage = nls.localize('terminal.integrated.legacyConsoleModeError', 'The terminal failed to launch properly because your system has legacy console mode enabled, uncheck "Use legacy console" cmd.exe\'s properties to fix this.'); - } else if (this._processManager.processState === ProcessState.KILLED_DURING_LAUNCH) { - let args = ''; - if (typeof this._shellLaunchConfig.args === 'string') { - args = ` ${this._shellLaunchConfig.args}`; - } else if (this._shellLaunchConfig.args && this._shellLaunchConfig.args.length) { - args = ' ' + this._shellLaunchConfig.args.map(a => { - if (typeof a === 'string' && a.indexOf(' ') !== -1) { - return `'${a}'`; - } - return a; - }).join(' '); - } - if (this._shellLaunchConfig.executable) { - exitCodeMessage = nls.localize('terminal.integrated.launchFailed', 'The terminal process command \'{0}{1}\' failed to launch (exit code: {2})', this._shellLaunchConfig.executable, args, exitCode); + switch (typeof exitCodeOrError) { + case 'number': + this._exitCode = exitCodeOrError; + // TODO: Add button for all failures to a help page + + if (this._exitCode === LEGACY_CONSOLE_MODE_EXIT_CODE) { + exitCodeMessage = nls.localize('terminal.integrated.legacyConsoleModeError', 'The terminal failed to launch properly because your system has legacy console mode enabled, uncheck "Use legacy console" cmd.exe\'s properties to fix this.'); + } else if (this._processManager.processState === ProcessState.KILLED_DURING_LAUNCH) { + let args = ''; + if (typeof this._shellLaunchConfig.args === 'string') { + args = ` ${this._shellLaunchConfig.args}`; + } else if (this._shellLaunchConfig.args && this._shellLaunchConfig.args.length) { + args = ' ' + this._shellLaunchConfig.args.map(a => { + if (typeof a === 'string' && a.indexOf(' ') !== -1) { + return `'${a}'`; + } + return a; + }).join(' '); + } + if (this._shellLaunchConfig.executable) { + console.log('FAIL 1', this._shellLaunchConfig.executable); + console.log('FAIL 2', args); + console.log('FAIL 3', '"' + this._exitCode + '"'); + exitCodeMessage = nls.localize('terminal.integrated.launchFailed', 'The terminal process command \'{0}{1}\' failed to launch (exit code: {2})', this._shellLaunchConfig.executable, args, this._exitCode); + } else { + exitCodeMessage = nls.localize('terminal.integrated.launchFailedExtHost', 'The terminal process failed to launch (exit code: {0})', this._exitCode); + } } else { - exitCodeMessage = nls.localize('terminal.integrated.launchFailedExtHost', 'The terminal process failed to launch (exit code: {0})', exitCode); + exitCodeMessage = nls.localize('terminal.integrated.exitedWithCode', 'The terminal process terminated with exit code: {0}', this._exitCode); } - } else { - exitCodeMessage = nls.localize('terminal.integrated.exitedWithCode', 'The terminal process terminated with exit code: {0}', exitCode); - } + break; + case 'object': + this._exitCode = exitCodeOrError.code; + exitCodeMessage = `${nls.localize('launchError.failed', "The terminal failed to launch:")} ${exitCodeOrError.message}`; + break; } this._logService.debug(`Terminal process exit (id: ${this.id}) state ${this._processManager.processState}`); @@ -1028,7 +1036,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { } } - this._onExit.fire(exitCode); + this._onExit.fire(this._exitCode); } private _attachPressAnyKeyToCloseListener(xterm: XTermTerminal) { diff --git a/src/vs/workbench/contrib/terminal/browser/terminalProcessExtHostProxy.ts b/src/vs/workbench/contrib/terminal/browser/terminalProcessExtHostProxy.ts index eee8ddafca5..4237d02462e 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalProcessExtHostProxy.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalProcessExtHostProxy.ts @@ -4,7 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import { Event, Emitter } from 'vs/base/common/event'; -import { ITerminalProcessExtHostProxy, IShellLaunchConfig, ITerminalChildProcess, ITerminalConfigHelper, ITerminalDimensions } from 'vs/workbench/contrib/terminal/common/terminal'; +import { ITerminalProcessExtHostProxy, IShellLaunchConfig, ITerminalChildProcess, ITerminalConfigHelper, ITerminalDimensions, ITerminalLaunchError } from 'vs/workbench/contrib/terminal/common/terminal'; import { Disposable } from 'vs/base/common/lifecycle'; import { URI } from 'vs/base/common/uri'; import { IRemoteAgentService } from 'vs/workbench/services/remote/common/remoteAgentService'; @@ -28,6 +28,8 @@ export class TerminalProcessExtHostProxy extends Disposable implements ITerminal private readonly _onProcessResolvedShellLaunchConfig = this._register(new Emitter()); public get onProcessResolvedShellLaunchConfig(): Event { return this._onProcessResolvedShellLaunchConfig.event; } + private readonly _onStart = this._register(new Emitter()); + public readonly onStart: Event = this._onStart.event; private readonly _onInput = this._register(new Emitter()); public readonly onInput: Event = this._onInput.event; private readonly _onResize: Emitter<{ cols: number, rows: number }> = this._register(new Emitter<{ cols: number, rows: number }>()); @@ -47,31 +49,15 @@ export class TerminalProcessExtHostProxy extends Disposable implements ITerminal constructor( public terminalId: number, - shellLaunchConfig: IShellLaunchConfig, - activeWorkspaceRootUri: URI | undefined, - cols: number, - rows: number, - configHelper: ITerminalConfigHelper, + private _shellLaunchConfig: IShellLaunchConfig, + private _activeWorkspaceRootUri: URI | undefined, + private _cols: number, + private _rows: number, + private _configHelper: ITerminalConfigHelper, @ITerminalService private readonly _terminalService: ITerminalService, - @IRemoteAgentService readonly remoteAgentService: IRemoteAgentService + @IRemoteAgentService private readonly _remoteAgentService: IRemoteAgentService ) { super(); - - // Request a process if needed, if this is a virtual process this step can be skipped as - // there is no real "process" and we know it's ready on the ext host already. - if (shellLaunchConfig.isExtensionTerminal) { - this._terminalService.requestStartExtensionTerminal(this, cols, rows); - } else { - remoteAgentService.getEnvironment().then(env => { - if (!env) { - throw new Error('Could not fetch environment'); - } - this._terminalService.requestSpawnExtHostProcess(this, shellLaunchConfig, activeWorkspaceRootUri, cols, rows, configHelper.checkWorkspaceShellPermissions(env.os)); - }); - if (!hasReceivedResponse) { - setTimeout(() => this._onProcessTitleChanged.fire(nls.localize('terminal.integrated.starting', "Starting...")), 0); - } - } } public emitData(data: string): void { @@ -118,6 +104,26 @@ export class TerminalProcessExtHostProxy extends Disposable implements ITerminal } } + public async start(): Promise { + // Request a process if needed, if this is a virtual process this step can be skipped as + // there is no real "process" and we know it's ready on the ext host already. + if (this._shellLaunchConfig.isExtensionTerminal) { + this._terminalService.requestStartExtensionTerminal(this, this._cols, this._rows); + } else { + const env = await this._remoteAgentService.getEnvironment(); + if (!env) { + throw new Error('Could not fetch environment'); + } + this._terminalService.requestSpawnExtHostProcess(this, this._shellLaunchConfig, this._activeWorkspaceRootUri, this._cols, this._rows, this._configHelper.checkWorkspaceShellPermissions(env.os)); + if (!hasReceivedResponse) { + setTimeout(() => this._onProcessTitleChanged.fire(nls.localize('terminal.integrated.starting', "Starting...")), 0); + } + } + + // TODO: Wait and return + return undefined; + } + public shutdown(immediate: boolean): void { this._onShutdown.fire(immediate); } diff --git a/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts b/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts index 2cf53d92986..f81b442e8b9 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts @@ -6,7 +6,7 @@ import * as platform from 'vs/base/common/platform'; import * as terminalEnvironment from 'vs/workbench/contrib/terminal/common/terminalEnvironment'; import { env as processEnv } from 'vs/base/common/process'; -import { ProcessState, ITerminalProcessManager, IShellLaunchConfig, ITerminalConfigHelper, ITerminalChildProcess, IBeforeProcessDataEvent, ITerminalEnvironment, ITerminalDimensions } from 'vs/workbench/contrib/terminal/common/terminal'; +import { ProcessState, ITerminalProcessManager, IShellLaunchConfig, ITerminalConfigHelper, ITerminalChildProcess, IBeforeProcessDataEvent, ITerminalEnvironment, ITerminalDimensions, ITerminalLaunchError } from 'vs/workbench/contrib/terminal/common/terminal'; import { ILogService } from 'vs/platform/log/common/log'; import { Emitter, Event } from 'vs/base/common/event'; import { IHistoryService } from 'vs/workbench/services/history/common/history'; @@ -127,7 +127,7 @@ export class TerminalProcessManager extends Disposable implements ITerminalProce cols: number, rows: number, isScreenReaderModeEnabled: boolean - ): Promise { + ): Promise { if (shellLaunchConfig.isExtensionTerminal) { this._processType = ProcessType.ExtensionTerminal; this._process = this._instantiationService.createInstance(TerminalProcessExtHostProxy, this._terminalId, shellLaunchConfig, undefined, cols, rows, this._configHelper); @@ -162,7 +162,12 @@ export class TerminalProcessManager extends Disposable implements ITerminalProce this._process = await this._launchProcess(shellLaunchConfig, cols, rows, this.userHome, isScreenReaderModeEnabled); } } + this.processState = ProcessState.LAUNCHING; + const error = await this._process.start(); + if (error) { + return error; + } this._process.onProcessData(data => { const beforeProcessDataEvent: IBeforeProcessDataEvent = { data }; @@ -198,6 +203,8 @@ export class TerminalProcessManager extends Disposable implements ITerminalProce this.processState = ProcessState.RUNNING; } }, LAUNCHING_DURATION); + + return undefined; } private async _launchProcess( diff --git a/src/vs/workbench/contrib/terminal/common/terminal.ts b/src/vs/workbench/contrib/terminal/common/terminal.ts index dcb635fca96..ae04a256cb5 100644 --- a/src/vs/workbench/contrib/terminal/common/terminal.ts +++ b/src/vs/workbench/contrib/terminal/common/terminal.ts @@ -70,9 +70,6 @@ export const TERMINAL_ACTION_CATEGORY = nls.localize('terminalCategory', "Termin export const DEFAULT_LETTER_SPACING = 0; export const MINIMUM_LETTER_SPACING = -5; export const DEFAULT_LINE_HEIGHT = 1; -export const SHELL_PATH_INVALID_EXIT_CODE = -1; -export const SHELL_PATH_DIRECTORY_EXIT_CODE = -2; -export const SHELL_CWD_INVALID_EXIT_CODE = -3; export const LEGACY_CONSOLE_MODE_EXIT_CODE = 3221225786; // microsoft/vscode#73790 export type FontWeight = 'normal' | 'bold' | '100' | '200' | '300' | '400' | '500' | '600' | '700' | '800' | '900'; @@ -308,7 +305,7 @@ export interface ITerminalProcessManager extends IDisposable { readonly onEnvironmentVariableInfoChanged: Event; dispose(immediate?: boolean): void; - createProcess(shellLaunchConfig: IShellLaunchConfig, cols: number, rows: number, isScreenReaderModeEnabled: boolean): Promise; + createProcess(shellLaunchConfig: IShellLaunchConfig, cols: number, rows: number, isScreenReaderModeEnabled: boolean): Promise; write(data: string): void; setDimensions(cols: number, rows: number): void; @@ -402,6 +399,11 @@ export interface IWindowsShellHelper extends IDisposable { getShellName(): Promise; } +export interface ITerminalLaunchError { + message: string; + code?: number; +} + /** * An interface representing a raw terminal child process, this contains a subset of the * child_process.ChildProcess node.js interface. @@ -414,6 +416,14 @@ export interface ITerminalChildProcess { onProcessOverrideDimensions?: Event; onProcessResolvedShellLaunchConfig?: Event; + /** + * Starts the process. + * + * @returns undefined when the process was successfully started, otherwise an object containing + * information on what went wrong. + */ + start(): Promise; + /** * Shutdown the terminal process. * diff --git a/src/vs/workbench/contrib/terminal/node/terminalProcess.ts b/src/vs/workbench/contrib/terminal/node/terminalProcess.ts index 6d49ecb8d38..0b26a04ccb2 100644 --- a/src/vs/workbench/contrib/terminal/node/terminalProcess.ts +++ b/src/vs/workbench/contrib/terminal/node/terminalProcess.ts @@ -11,22 +11,27 @@ import * as fs from 'fs'; import { Event, Emitter } from 'vs/base/common/event'; import { getWindowsBuildNumber } from 'vs/workbench/contrib/terminal/node/terminal'; import { Disposable } from 'vs/base/common/lifecycle'; -import { IShellLaunchConfig, ITerminalChildProcess, SHELL_PATH_INVALID_EXIT_CODE, SHELL_PATH_DIRECTORY_EXIT_CODE, SHELL_CWD_INVALID_EXIT_CODE } from 'vs/workbench/contrib/terminal/common/terminal'; +import { IShellLaunchConfig, ITerminalChildProcess, ITerminalLaunchError } from 'vs/workbench/contrib/terminal/common/terminal'; import { exec } from 'child_process'; import { ILogService } from 'vs/platform/log/common/log'; import { stat } from 'vs/base/node/pfs'; import { findExecutable } from 'vs/workbench/contrib/terminal/node/terminalEnvironment'; import { URI } from 'vs/base/common/uri'; +import { localize } from 'vs/nls'; export class TerminalProcess extends Disposable implements ITerminalChildProcess { private _exitCode: number | undefined; + private _exitMessage: string | undefined; private _closeTimeout: any; private _ptyProcess: pty.IPty | undefined; private _currentTitle: string = ''; private _processStartupComplete: Promise | undefined; private _isDisposed: boolean = false; private _titleInterval: NodeJS.Timer | null = null; - private _initialCwd: string; + private readonly _initialCwd: string; + private readonly _ptyOptions: pty.IPtyForkOptions | pty.IWindowsPtyForkOptions; + + public get exitMessage(): string | undefined { return this._exitMessage; } private readonly _onProcessData = this._register(new Emitter()); public get onProcessData(): Event { return this._onProcessData.event; } @@ -38,7 +43,7 @@ export class TerminalProcess extends Disposable implements ITerminalChildProcess public get onProcessTitleChanged(): Event { return this._onProcessTitleChanged.event; } constructor( - shellLaunchConfig: IShellLaunchConfig, + private readonly _shellLaunchConfig: IShellLaunchConfig, cwd: string, cols: number, rows: number, @@ -47,73 +52,89 @@ export class TerminalProcess extends Disposable implements ITerminalChildProcess @ILogService private readonly _logService: ILogService ) { super(); - let shellName: string; + let name: string; if (os.platform() === 'win32') { - shellName = path.basename(shellLaunchConfig.executable || ''); + name = path.basename(this._shellLaunchConfig.executable || ''); } else { // Using 'xterm-256color' here helps ensure that the majority of Linux distributions will use a // color prompt as defined in the default ~/.bashrc file. - shellName = 'xterm-256color'; + name = 'xterm-256color'; } - this._initialCwd = cwd; - const useConpty = windowsEnableConpty && process.platform === 'win32' && getWindowsBuildNumber() >= 18309; - const options: pty.IPtyForkOptions | pty.IWindowsPtyForkOptions = { - name: shellName, + this._ptyOptions = { + name, cwd, env, cols, rows, useConpty, // This option will force conpty to not redraw the whole viewport on launch - conptyInheritCursor: useConpty && !!shellLaunchConfig.initialText + conptyInheritCursor: useConpty && !!_shellLaunchConfig.initialText }; - - // TODO: Pull verification out into its own function - const cwdVerification = stat(cwd).then(async stat => { - if (!stat.isDirectory()) { - return Promise.reject(SHELL_CWD_INVALID_EXIT_CODE); - } - return undefined; - }, async err => { - if (err && err.code === 'ENOENT') { - // So we can include in the error message the specified CWD - shellLaunchConfig.cwd = cwd; - return Promise.reject(SHELL_CWD_INVALID_EXIT_CODE); - } - return undefined; - }); - - const executableVerification = stat(shellLaunchConfig.executable!).then(async stat => { - if (!stat.isFile() && !stat.isSymbolicLink()) { - return Promise.reject(stat.isDirectory() ? SHELL_PATH_DIRECTORY_EXIT_CODE : SHELL_PATH_INVALID_EXIT_CODE); - } - return undefined; - }, async (err) => { - if (err && err.code === 'ENOENT') { - let cwd = shellLaunchConfig.cwd instanceof URI ? shellLaunchConfig.cwd.path : shellLaunchConfig.cwd!; - // Try to get path - const envPaths: string[] | undefined = (shellLaunchConfig.env && shellLaunchConfig.env.PATH) ? shellLaunchConfig.env.PATH.split(path.delimiter) : undefined; - const executable = await findExecutable(shellLaunchConfig.executable!, cwd, envPaths); - if (!executable) { - return Promise.reject(SHELL_PATH_INVALID_EXIT_CODE); - } - } - return undefined; - }); - - Promise.all([cwdVerification, executableVerification]).then(() => { - this.setupPtyProcess(shellLaunchConfig, options); - }).catch((exitCode: number) => { - return this._launchFailed(exitCode); - }); } - private _launchFailed(exitCode: number): void { - this._exitCode = exitCode; - this._queueProcessExit(); - this._processStartupComplete = Promise.resolve(undefined); + public async start(): Promise { + const results = await Promise.all([this._validateCwd(), this._validateExecutable()]); + const firstError = results.find(r => r !== undefined); + if (firstError) { + // TODO: Anything else we need to do here? + // this._processStartupComplete = Promise.resolve(undefined); + // this._onProcessExit.fire(0); + + return firstError; + } + + try { + this.setupPtyProcess(this._shellLaunchConfig, this._ptyOptions); + return undefined; + } catch (err) { + + // TODO: Anything else we need to do here? + + // TODO: Extract code and message from the native exception, currently they're combined + // A native exception occurred + this._logService.trace('IPty#spawn native exception', err); + return { message: `A native exception occurred during launch (${err.message})` }; + } + } + + private async _validateCwd(): Promise { + try { + const result = await stat(this._initialCwd); + if (!result.isDirectory()) { + return { message: localize('launchFail.cwdNotDirectory', 'Starting directory (cwd) "{0}" is not a directory', this._initialCwd.toString()) }; + } + } catch (err) { + if (err?.code === 'ENOENT') { + return { message: localize('launchFail.cwdDoesNotExist', 'Starting directory (cwd) "{0}" does not exist', this._initialCwd.toString()) }; + } + } + return undefined; + } + + private async _validateExecutable(): Promise { + const slc = this._shellLaunchConfig; + if (!slc.executable) { + throw new Error('IShellLaunchConfig.executable not set'); + } + try { + const result = await stat(slc.executable); + if (!result.isFile() && !result.isSymbolicLink()) { + return { message: localize('launchFail.executableIsNotFileOrSymlink', 'Shell path "{0}" is not a file of a symlink', slc.executable) }; + } + } catch (err) { + if (err?.code === 'ENOENT') { + // The executable isn't an absolute path, try find it on the PATH or CWD + let cwd = slc.cwd instanceof URI ? slc.cwd.path : slc.cwd!; + const envPaths: string[] | undefined = (slc.env && slc.env.PATH) ? slc.env.PATH.split(path.delimiter) : undefined; + const executable = await findExecutable(slc.executable!, cwd, envPaths); + if (!executable) { + return { message: localize('launchFail.executableDoesNotExist', 'Shell path "{0}" does not exist') }; + } + } + } + return undefined; } private setupPtyProcess(shellLaunchConfig: IShellLaunchConfig, options: pty.IPtyForkOptions): void { @@ -132,14 +153,12 @@ export class TerminalProcess extends Disposable implements ITerminalChildProcess } }); ptyProcess.onExit(e => { + console.log('onExit', e); this._exitCode = e.exitCode; this._queueProcessExit(); }); this._setupTitlePolling(ptyProcess); - // TODO: We should no longer need to delay this since pty.spawn is sync - setTimeout(() => { - this._sendProcessId(ptyProcess); - }, 500); + this._sendProcessId(ptyProcess.pid); } public dispose(): void { From d9c1fd04bceedb76d4a64560b1b24f7fa4701406 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 12 Jun 2020 08:38:00 -0700 Subject: [PATCH 03/13] Add troubleshoot link --- .../terminal/browser/terminalActions.ts | 14 +++++++++++++ .../terminal/browser/terminalInstance.ts | 21 ++++++++++++------- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/src/vs/workbench/contrib/terminal/browser/terminalActions.ts b/src/vs/workbench/contrib/terminal/browser/terminalActions.ts index 4aa1cc3e8cf..8165776de8a 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalActions.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalActions.ts @@ -37,6 +37,7 @@ import { KeyMod, KeyCode } from 'vs/base/common/keyCodes'; import { KeybindingWeight } from 'vs/platform/keybinding/common/keybindingsRegistry'; import { localize } from 'vs/nls'; import { CONTEXT_ACCESSIBILITY_MODE_ENABLED } from 'vs/platform/accessibility/common/accessibility'; +import { IOpenerService } from 'vs/platform/opener/common/opener'; async function getCwdForSplit(configHelper: ITerminalConfigHelper, instance: ITerminalInstance, folders?: IWorkspaceFolder[], commandService?: ICommandService): Promise { switch (configHelper.config.splitCwd) { @@ -392,6 +393,19 @@ export class ClearTerminalAction extends Action { } } +export class TerminalLaunchTroubleshootAction extends Action { + + constructor( + @IOpenerService private readonly _openerService: IOpenerService + ) { + super('workbench.action.terminal.launchHelp', localize('terminalLaunchTroubleshoot', "Troubleshoot")); + } + + async run(): Promise { + this._openerService.open('https://aka.ms/vscode-troubleshoot-terminal-launch'); + } +} + export function registerTerminalActions() { const category: ILocalizedString = { value: TERMINAL_ACTION_CATEGORY, original: 'Terminal' }; diff --git a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts index 5ae66e068ca..e0b8ef20bb1 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts @@ -42,6 +42,7 @@ import { IEditorOptions } from 'vs/editor/common/config/editorOptions'; import { IViewsService, IViewDescriptorService, ViewContainerLocation } from 'vs/workbench/common/views'; import { EnvironmentVariableInfoWidget } from 'vs/workbench/contrib/terminal/browser/widgets/environmentVariableInfoWidget'; import { IEnvironmentVariableInfo } from 'vs/workbench/contrib/terminal/common/environmentVariable'; +import { TerminalLaunchTroubleshootAction } from 'vs/workbench/contrib/terminal/browser/terminalActions'; // How long in milliseconds should an average frame take to render for a notification to appear // which suggests the fallback DOM-based renderer @@ -996,7 +997,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { break; case 'object': this._exitCode = exitCodeOrError.code; - exitCodeMessage = `${nls.localize('launchError.failed', "The terminal failed to launch:")} ${exitCodeOrError.message}`; + exitCodeMessage = `${nls.localize('launchError.failed', "The terminal process failed to launch:")} ${exitCodeOrError.message}`; break; } @@ -1024,14 +1025,18 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { } else { this.dispose(); if (exitCodeMessage) { - if (this._processManager.processState === ProcessState.KILLED_DURING_LAUNCH) { - this._notificationService.error(exitCodeMessage); + const failedDuringLaunch = this._processManager.processState === ProcessState.KILLED_DURING_LAUNCH; + if (failedDuringLaunch || this._configHelper.config.showExitAlert) { + // Always show launch failures + this._notificationService.notify({ + message: exitCodeMessage, + severity: Severity.Error, + actions: { primary: [this._instantiationService.createInstance(TerminalLaunchTroubleshootAction)] } + }); } else { - if (this._configHelper.config.showExitAlert) { - this._notificationService.error(exitCodeMessage); - } else { - console.warn(exitCodeMessage); - } + // Log to help surface the error in case users report issues with showExitAlert + // disabled + this._logService.warn(exitCodeMessage); } } } From c507c4393a2c47467cfc671da0c471512e68a27e Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 12 Jun 2020 09:39:46 -0700 Subject: [PATCH 04/13] Fix executable missing from error notification --- src/vs/workbench/contrib/terminal/node/terminalProcess.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vs/workbench/contrib/terminal/node/terminalProcess.ts b/src/vs/workbench/contrib/terminal/node/terminalProcess.ts index 0b26a04ccb2..a3401030785 100644 --- a/src/vs/workbench/contrib/terminal/node/terminalProcess.ts +++ b/src/vs/workbench/contrib/terminal/node/terminalProcess.ts @@ -130,7 +130,7 @@ export class TerminalProcess extends Disposable implements ITerminalChildProcess const envPaths: string[] | undefined = (slc.env && slc.env.PATH) ? slc.env.PATH.split(path.delimiter) : undefined; const executable = await findExecutable(slc.executable!, cwd, envPaths); if (!executable) { - return { message: localize('launchFail.executableDoesNotExist', 'Shell path "{0}" does not exist') }; + return { message: localize('launchFail.executableDoesNotExist', 'Shell path "{0}" does not exist', slc.executable) }; } } } From 1dd4c04384a819a12edfe27fc64ab56b75f8c014 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 12 Jun 2020 11:05:45 -0700 Subject: [PATCH 05/13] Fix launching in ext host/pty terminals --- .../api/browser/mainThreadTerminalService.ts | 36 +++++++-------- .../workbench/api/common/extHost.protocol.ts | 6 +-- .../api/common/extHostTerminalService.ts | 20 ++++----- .../api/node/extHostTerminalService.ts | 14 ++++-- .../contrib/terminal/browser/terminal.ts | 6 +-- .../terminal/browser/terminalInstance.ts | 45 +++++++++---------- .../browser/terminalProcessExtHostProxy.ts | 31 +++++++------ .../browser/terminalProcessManager.ts | 9 ++-- .../terminal/browser/terminalService.ts | 15 ++++--- .../contrib/terminal/common/terminal.ts | 3 +- .../contrib/terminal/node/terminalProcess.ts | 9 ++-- 11 files changed, 103 insertions(+), 91 deletions(-) diff --git a/src/vs/workbench/api/browser/mainThreadTerminalService.ts b/src/vs/workbench/api/browser/mainThreadTerminalService.ts index 54b299991ec..e913ce5323d 100644 --- a/src/vs/workbench/api/browser/mainThreadTerminalService.ts +++ b/src/vs/workbench/api/browser/mainThreadTerminalService.ts @@ -23,7 +23,6 @@ export class MainThreadTerminalService implements MainThreadTerminalServiceShape private _remoteAuthority: string | null; private readonly _toDispose = new DisposableStore(); private readonly _terminalProcesses = new Map>(); - private readonly _terminalProcessesReady = new Map void>(); private _dataEventTracker: TerminalDataEventTracker | undefined; private _linkHandler: IDisposable | undefined; @@ -106,7 +105,6 @@ export class MainThreadTerminalService implements MainThreadTerminalServiceShape isExtensionTerminal: launchConfig.isExtensionTerminal }; const terminal = this._terminalService.createTerminal(shellLaunchConfig); - this._terminalProcesses.set(terminal.id, new Promise(r => this._terminalProcessesReady.set(terminal.id, r))); return Promise.resolve({ id: terminal.id, name: terminal.title @@ -232,13 +230,7 @@ export class MainThreadTerminalService implements MainThreadTerminalServiceShape } const proxy = request.proxy; - const ready = this._terminalProcessesReady.get(proxy.terminalId); - if (ready) { - ready(proxy); - this._terminalProcessesReady.delete(proxy.terminalId); - } else { - this._terminalProcesses.set(proxy.terminalId, Promise.resolve(proxy)); - } + this._terminalProcesses.set(proxy.terminalId, Promise.resolve(proxy)); const shellLaunchConfigDto: IShellLaunchConfigDto = { name: request.shellLaunchConfig.name, executable: request.shellLaunchConfig.executable, @@ -246,7 +238,16 @@ export class MainThreadTerminalService implements MainThreadTerminalServiceShape cwd: request.shellLaunchConfig.cwd, env: request.shellLaunchConfig.env }; - this._proxy.$spawnExtHostProcess(proxy.terminalId, shellLaunchConfigDto, request.activeWorkspaceRootUri, request.cols, request.rows, request.isWorkspaceShellAllowed); + + this._proxy.$spawnExtHostProcess( + proxy.terminalId, + shellLaunchConfigDto, + request.activeWorkspaceRootUri, + request.cols, + request.rows, + request.isWorkspaceShellAllowed + ).then(request.callback); + proxy.onInput(data => this._proxy.$acceptProcessInput(proxy.terminalId, data)); proxy.onResize(dimensions => this._proxy.$acceptProcessResize(proxy.terminalId, dimensions.cols, dimensions.rows)); proxy.onShutdown(immediate => this._proxy.$acceptProcessShutdown(proxy.terminalId, immediate)); @@ -257,13 +258,7 @@ export class MainThreadTerminalService implements MainThreadTerminalServiceShape private _onRequestStartExtensionTerminal(request: IStartExtensionTerminalRequest): void { const proxy = request.proxy; - const ready = this._terminalProcessesReady.get(proxy.terminalId); - if (!ready) { - this._terminalProcesses.set(proxy.terminalId, Promise.resolve(proxy)); - } else { - ready(proxy); - this._terminalProcessesReady.delete(proxy.terminalId); - } + this._terminalProcesses.set(proxy.terminalId, Promise.resolve(proxy)); // Note that onReisze is not being listened to here as it needs to fire when max dimensions // change, excluding the dimension override @@ -271,7 +266,12 @@ export class MainThreadTerminalService implements MainThreadTerminalServiceShape columns: request.cols, rows: request.rows } : undefined; - this._proxy.$startExtensionTerminal(proxy.terminalId, initialDimensions); + + this._proxy.$startExtensionTerminal( + proxy.terminalId, + initialDimensions + ).then(request.callback); + proxy.onInput(data => this._proxy.$acceptProcessInput(proxy.terminalId, data)); proxy.onShutdown(immediate => this._proxy.$acceptProcessShutdown(proxy.terminalId, immediate)); proxy.onRequestCwd(() => this._proxy.$acceptProcessRequestCwd(proxy.terminalId)); diff --git a/src/vs/workbench/api/common/extHost.protocol.ts b/src/vs/workbench/api/common/extHost.protocol.ts index b662b07ac07..4687dce7649 100644 --- a/src/vs/workbench/api/common/extHost.protocol.ts +++ b/src/vs/workbench/api/common/extHost.protocol.ts @@ -41,7 +41,7 @@ import * as tasks from 'vs/workbench/api/common/shared/tasks'; import { IRevealOptions, ITreeItem } from 'vs/workbench/common/views'; import { IAdapterDescriptor, IConfig, IDebugSessionReplMode } from 'vs/workbench/contrib/debug/common/debug'; import { ITextQueryBuilderOptions } from 'vs/workbench/contrib/search/common/queryBuilder'; -import { ITerminalDimensions, IShellLaunchConfig } from 'vs/workbench/contrib/terminal/common/terminal'; +import { ITerminalDimensions, IShellLaunchConfig, ITerminalLaunchError } from 'vs/workbench/contrib/terminal/common/terminal'; import { ExtensionActivationError } from 'vs/workbench/services/extensions/common/extensions'; import { createExtHostContextProxyIdentifier as createExtId, createMainContextProxyIdentifier as createMainId, IRPCProtocol } from 'vs/workbench/services/extensions/common/proxyIdentifier'; import * as search from 'vs/workbench/services/search/common/search'; @@ -1388,8 +1388,8 @@ export interface ExtHostTerminalServiceShape { $acceptTerminalTitleChange(id: number, name: string): void; $acceptTerminalDimensions(id: number, cols: number, rows: number): void; $acceptTerminalMaximumDimensions(id: number, cols: number, rows: number): void; - $spawnExtHostProcess(id: number, shellLaunchConfig: IShellLaunchConfigDto, activeWorkspaceRootUri: UriComponents | undefined, cols: number, rows: number, isWorkspaceShellAllowed: boolean): void; - $startExtensionTerminal(id: number, initialDimensions: ITerminalDimensionsDto | undefined): void; + $spawnExtHostProcess(id: number, shellLaunchConfig: IShellLaunchConfigDto, activeWorkspaceRootUri: UriComponents | undefined, cols: number, rows: number, isWorkspaceShellAllowed: boolean): Promise; + $startExtensionTerminal(id: number, initialDimensions: ITerminalDimensionsDto | undefined): Promise; $acceptProcessInput(id: number, data: string): void; $acceptProcessResize(id: number, cols: number, rows: number): void; $acceptProcessShutdown(id: number, immediate: boolean): void; diff --git a/src/vs/workbench/api/common/extHostTerminalService.ts b/src/vs/workbench/api/common/extHostTerminalService.ts index e3c69055cdb..2cc0aeea3c3 100644 --- a/src/vs/workbench/api/common/extHostTerminalService.ts +++ b/src/vs/workbench/api/common/extHostTerminalService.ts @@ -9,7 +9,7 @@ import { ExtHostTerminalServiceShape, MainContext, MainThreadTerminalServiceShap import { ExtHostConfigProvider } from 'vs/workbench/api/common/extHostConfiguration'; import { createDecorator } from 'vs/platform/instantiation/common/instantiation'; import { URI, UriComponents } from 'vs/base/common/uri'; -import { ITerminalChildProcess, ITerminalDimensions, EXT_HOST_CREATION_DELAY } from 'vs/workbench/contrib/terminal/common/terminal'; +import { ITerminalChildProcess, ITerminalDimensions, EXT_HOST_CREATION_DELAY, ITerminalLaunchError } from 'vs/workbench/contrib/terminal/common/terminal'; import { timeout } from 'vs/base/common/async'; import { IExtHostRpcService } from 'vs/workbench/api/common/extHostRpcService'; import { TerminalDataBufferer } from 'vs/workbench/contrib/terminal/common/terminalDataBuffering'; @@ -17,6 +17,7 @@ import { IDisposable, DisposableStore } from 'vs/base/common/lifecycle'; import { Disposable as VSCodeDisposable, EnvironmentVariableMutatorType } from './extHostTypes'; import { IExtensionDescription } from 'vs/platform/extensions/common/extensions'; import { ISerializableEnvironmentVariableCollection } from 'vs/workbench/contrib/terminal/common/environmentVariable'; +import { localize } from 'vs/nls'; export interface IExtHostTerminalService extends ExtHostTerminalServiceShape { @@ -336,7 +337,7 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ public abstract createTerminalFromOptions(options: vscode.TerminalOptions): vscode.Terminal; public abstract getDefaultShell(useAutomationShell: boolean, configProvider: ExtHostConfigProvider): string; public abstract getDefaultShellArgs(useAutomationShell: boolean, configProvider: ExtHostConfigProvider): string[] | string; - public abstract $spawnExtHostProcess(id: number, shellLaunchConfigDto: IShellLaunchConfigDto, activeWorkspaceRootUriComponents: UriComponents, cols: number, rows: number, isWorkspaceShellAllowed: boolean): Promise; + public abstract $spawnExtHostProcess(id: number, shellLaunchConfigDto: IShellLaunchConfigDto, activeWorkspaceRootUriComponents: UriComponents, cols: number, rows: number, isWorkspaceShellAllowed: boolean): Promise; public abstract $getAvailableShells(): Promise; public abstract $getDefaultShellAndArgs(useAutomationShell: boolean): Promise; public abstract $acceptWorkspacePermissionsChanged(isAllowed: boolean): void; @@ -458,20 +459,17 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ } } - public async $startExtensionTerminal(id: number, initialDimensions: ITerminalDimensionsDto | undefined): Promise { + 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; + return { message: localize('launchFail.idMissingOnExtHost', "Could not find the terminal with id {0} on the extension host", id) }; } // Wait for onDidOpenTerminal to fire - let openPromise: Promise; - if (terminal.isOpen) { - openPromise = Promise.resolve(); - } else { - openPromise = new Promise(r => { + if (!terminal.isOpen) { + await new Promise(r => { // Ensure open is called after onDidOpenTerminal const listener = this.onDidOpenTerminal(async e => { if (e === terminal) { @@ -481,7 +479,6 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ }); }); } - await openPromise; if (this._terminalProcesses[id]) { (this._terminalProcesses[id] as ExtHostPseudoterminal).startSendingEvents(initialDimensions); @@ -490,6 +487,7 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ this._extensionTerminalAwaitingStart[id] = { initialDimensions }; } + return undefined; } protected _setupExtHostProcessListeners(id: number, p: ITerminalChildProcess): IDisposable { @@ -725,7 +723,7 @@ export class WorkerExtHostTerminalService extends BaseExtHostTerminalService { throw new Error('Not implemented'); } - public $spawnExtHostProcess(id: number, shellLaunchConfigDto: IShellLaunchConfigDto, activeWorkspaceRootUriComponents: UriComponents, cols: number, rows: number, isWorkspaceShellAllowed: boolean): Promise { + public $spawnExtHostProcess(id: number, shellLaunchConfigDto: IShellLaunchConfigDto, activeWorkspaceRootUriComponents: UriComponents, cols: number, rows: number, isWorkspaceShellAllowed: boolean): Promise { throw new Error('Not implemented'); } diff --git a/src/vs/workbench/api/node/extHostTerminalService.ts b/src/vs/workbench/api/node/extHostTerminalService.ts index ec6a950f155..ca0e5a2adb5 100644 --- a/src/vs/workbench/api/node/extHostTerminalService.ts +++ b/src/vs/workbench/api/node/extHostTerminalService.ts @@ -12,7 +12,7 @@ import * as terminalEnvironment from 'vs/workbench/contrib/terminal/common/termi import { IShellLaunchConfigDto, IShellDefinitionDto, IShellAndArgsDto } from 'vs/workbench/api/common/extHost.protocol'; import { ExtHostConfiguration, ExtHostConfigProvider, IExtHostConfiguration } from 'vs/workbench/api/common/extHostConfiguration'; import { ILogService } from 'vs/platform/log/common/log'; -import { IShellLaunchConfig, ITerminalEnvironment } from 'vs/workbench/contrib/terminal/common/terminal'; +import { IShellLaunchConfig, ITerminalEnvironment, ITerminalLaunchError } from 'vs/workbench/contrib/terminal/common/terminal'; import { TerminalProcess } from 'vs/workbench/contrib/terminal/node/terminalProcess'; import { ExtHostWorkspace, IExtHostWorkspace } from 'vs/workbench/api/common/extHostWorkspace'; import { IWorkspaceFolder } from 'vs/platform/workspace/common/workspace'; @@ -129,7 +129,7 @@ export class ExtHostTerminalService extends BaseExtHostTerminalService { this._variableResolver = new ExtHostVariableResolverService(workspaceFolders || [], this._extHostDocumentsAndEditors, configProvider, process.env as platform.IProcessEnvironment); } - public async $spawnExtHostProcess(id: number, shellLaunchConfigDto: IShellLaunchConfigDto, activeWorkspaceRootUriComponents: UriComponents | undefined, cols: number, rows: number, isWorkspaceShellAllowed: boolean): Promise { + public async $spawnExtHostProcess(id: number, shellLaunchConfigDto: IShellLaunchConfigDto, activeWorkspaceRootUriComponents: UriComponents | undefined, cols: number, rows: number, isWorkspaceShellAllowed: boolean): Promise { const shellLaunchConfig: IShellLaunchConfig = { name: shellLaunchConfigDto.name, executable: shellLaunchConfigDto.executable, @@ -209,7 +209,15 @@ export class ExtHostTerminalService extends BaseExtHostTerminalService { // TODO: Support conpty on remote, it doesn't seem to work for some reason? // TODO: When conpty is enabled, only enable it when accessibilityMode is off const enableConpty = false; //terminalConfig.get('windowsEnableConpty') as boolean; - this._setupExtHostProcessListeners(id, new TerminalProcess(shellLaunchConfig, initialCwd, cols, rows, env, enableConpty, this._logService)); + + const terminalProcess = new TerminalProcess(shellLaunchConfig, initialCwd, cols, rows, env, enableConpty, this._logService); + this._setupExtHostProcessListeners(id, terminalProcess); + const error = await terminalProcess.start(); + if (error) { + // TODO: Teardown? + return error; + } + return undefined; } public $getAvailableShells(): Promise { diff --git a/src/vs/workbench/contrib/terminal/browser/terminal.ts b/src/vs/workbench/contrib/terminal/browser/terminal.ts index e953370e103..8b6d089f44c 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminal.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminal.ts @@ -7,7 +7,7 @@ import { Terminal as XTermTerminal } from 'xterm'; import { SearchAddon as XTermSearchAddon } from 'xterm-addon-search'; import { Unicode11Addon as XTermUnicode11Addon } from 'xterm-addon-unicode11'; import { WebglAddon as XTermWebglAddon } from 'xterm-addon-webgl'; -import { IWindowsShellHelper, ITerminalConfigHelper, ITerminalChildProcess, IShellLaunchConfig, IDefaultShellAndArgsRequest, ISpawnExtHostProcessRequest, IStartExtensionTerminalRequest, IAvailableShellsRequest, ITerminalProcessExtHostProxy, ICommandTracker, INavigationMode, TitleEventSource, ITerminalDimensions } from 'vs/workbench/contrib/terminal/common/terminal'; +import { IWindowsShellHelper, ITerminalConfigHelper, ITerminalChildProcess, IShellLaunchConfig, IDefaultShellAndArgsRequest, ISpawnExtHostProcessRequest, IStartExtensionTerminalRequest, IAvailableShellsRequest, ITerminalProcessExtHostProxy, ICommandTracker, INavigationMode, TitleEventSource, ITerminalDimensions, ITerminalLaunchError } from 'vs/workbench/contrib/terminal/common/terminal'; import { createDecorator } from 'vs/platform/instantiation/common/instantiation'; import { IProcessEnvironment, Platform } from 'vs/base/common/platform'; import { Event } from 'vs/base/common/event'; @@ -161,8 +161,8 @@ export interface ITerminalService { preparePathForTerminalAsync(path: string, executable: string | undefined, title: string, shellType: TerminalShellType): Promise; extHostReady(remoteAuthority: string): void; - requestSpawnExtHostProcess(proxy: ITerminalProcessExtHostProxy, shellLaunchConfig: IShellLaunchConfig, activeWorkspaceRootUri: URI | undefined, cols: number, rows: number, isWorkspaceShellAllowed: boolean): void; - requestStartExtensionTerminal(proxy: ITerminalProcessExtHostProxy, cols: number, rows: number): void; + requestSpawnExtHostProcess(proxy: ITerminalProcessExtHostProxy, shellLaunchConfig: IShellLaunchConfig, activeWorkspaceRootUri: URI | undefined, cols: number, rows: number, isWorkspaceShellAllowed: boolean): Promise; + requestStartExtensionTerminal(proxy: ITerminalProcessExtHostProxy, cols: number, rows: number): Promise; } export interface ISearchOptions { diff --git a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts index e0b8ef20bb1..b696df22aae 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts @@ -25,7 +25,7 @@ import { activeContrastBorder, scrollbarSliderActiveBackground, scrollbarSliderB import { ICssStyleCollector, IColorTheme, IThemeService, registerThemingParticipant } from 'vs/platform/theme/common/themeService'; import { PANEL_BACKGROUND, SIDE_BAR_BACKGROUND } from 'vs/workbench/common/theme'; import { TerminalWidgetManager } from 'vs/workbench/contrib/terminal/browser/widgets/widgetManager'; -import { IShellLaunchConfig, ITerminalDimensions, ITerminalProcessManager, KEYBINDING_CONTEXT_TERMINAL_TEXT_SELECTED, NEVER_MEASURE_RENDER_TIME_STORAGE_KEY, ProcessState, TERMINAL_VIEW_ID, IWindowsShellHelper, KEYBINDING_CONTEXT_TERMINAL_A11Y_TREE_FOCUS, INavigationMode, TitleEventSource, LEGACY_CONSOLE_MODE_EXIT_CODE, DEFAULT_COMMANDS_TO_SKIP_SHELL, ITerminalLaunchError } from 'vs/workbench/contrib/terminal/common/terminal'; +import { IShellLaunchConfig, ITerminalDimensions, ITerminalProcessManager, KEYBINDING_CONTEXT_TERMINAL_TEXT_SELECTED, NEVER_MEASURE_RENDER_TIME_STORAGE_KEY, ProcessState, TERMINAL_VIEW_ID, IWindowsShellHelper, KEYBINDING_CONTEXT_TERMINAL_A11Y_TREE_FOCUS, INavigationMode, TitleEventSource, DEFAULT_COMMANDS_TO_SKIP_SHELL, ITerminalLaunchError } from 'vs/workbench/contrib/terminal/common/terminal'; import { ansiColorIdentifiers, TERMINAL_BACKGROUND_COLOR, TERMINAL_CURSOR_BACKGROUND_COLOR, TERMINAL_CURSOR_FOREGROUND_COLOR, TERMINAL_FOREGROUND_COLOR, TERMINAL_SELECTION_BACKGROUND_COLOR } from 'vs/workbench/contrib/terminal/common/terminalColorRegistry'; import { TerminalConfigHelper } from 'vs/workbench/contrib/terminal/browser/terminalConfigHelper'; import { TerminalLinkManager } from 'vs/workbench/contrib/terminal/browser/links/terminalLinkManager'; @@ -967,37 +967,34 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { switch (typeof exitCodeOrError) { case 'number': this._exitCode = exitCodeOrError; - // TODO: Add button for all failures to a help page - if (this._exitCode === LEGACY_CONSOLE_MODE_EXIT_CODE) { - exitCodeMessage = nls.localize('terminal.integrated.legacyConsoleModeError', 'The terminal failed to launch properly because your system has legacy console mode enabled, uncheck "Use legacy console" cmd.exe\'s properties to fix this.'); - } else if (this._processManager.processState === ProcessState.KILLED_DURING_LAUNCH) { - let args = ''; + let commandLine: string | undefined = undefined; + if (this._shellLaunchConfig.executable) { + commandLine = this._shellLaunchConfig.executable; if (typeof this._shellLaunchConfig.args === 'string') { - args = ` ${this._shellLaunchConfig.args}`; + commandLine += ` ${this._shellLaunchConfig.args}`; } else if (this._shellLaunchConfig.args && this._shellLaunchConfig.args.length) { - args = ' ' + this._shellLaunchConfig.args.map(a => { - if (typeof a === 'string' && a.indexOf(' ') !== -1) { - return `'${a}'`; - } - return a; - }).join(' '); + commandLine += this._shellLaunchConfig.args.map(a => ` '${a}'`).join(); } - if (this._shellLaunchConfig.executable) { - console.log('FAIL 1', this._shellLaunchConfig.executable); - console.log('FAIL 2', args); - console.log('FAIL 3', '"' + this._exitCode + '"'); - exitCodeMessage = nls.localize('terminal.integrated.launchFailed', 'The terminal process command \'{0}{1}\' failed to launch (exit code: {2})', this._shellLaunchConfig.executable, args, this._exitCode); - } else { - exitCodeMessage = nls.localize('terminal.integrated.launchFailedExtHost', 'The terminal process failed to launch (exit code: {0})', this._exitCode); - } - } else { - exitCodeMessage = nls.localize('terminal.integrated.exitedWithCode', 'The terminal process terminated with exit code: {0}', this._exitCode); } + + if (this._processManager.processState === ProcessState.KILLED_DURING_LAUNCH) { + if (commandLine) { + exitCodeMessage = nls.localize('launchFailed.exitCodeAndCommandLine', "The terminal process \"{0}\" failed to launch (exit code: {1})", commandLine, this._exitCode); + break; + } + exitCodeMessage = nls.localize('launchFailed.exitCodeOnly', "The terminal process failed to launch (exit code: {0})", this._exitCode); + break; + } + if (commandLine) { + exitCodeMessage = nls.localize('terminated.exitCodeAndCommandLine', "The terminal process \"{0}\" terminated with exit code: {1}", commandLine, this._exitCode); + break; + } + exitCodeMessage = nls.localize('terminated.exitCodeOnly', "The terminal process terminated with exit code: {0}", this._exitCode); break; case 'object': this._exitCode = exitCodeOrError.code; - exitCodeMessage = `${nls.localize('launchError.failed', "The terminal process failed to launch:")} ${exitCodeOrError.message}`; + exitCodeMessage = nls.localize('launchFailed.errorMessage', "The terminal process failed to launch: {0}", exitCodeOrError.message); break; } diff --git a/src/vs/workbench/contrib/terminal/browser/terminalProcessExtHostProxy.ts b/src/vs/workbench/contrib/terminal/browser/terminalProcessExtHostProxy.ts index 4237d02462e..7d7623c1a0d 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalProcessExtHostProxy.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalProcessExtHostProxy.ts @@ -11,7 +11,7 @@ import { IRemoteAgentService } from 'vs/workbench/services/remote/common/remoteA import * as nls from 'vs/nls'; import { ITerminalService } from 'vs/workbench/contrib/terminal/browser/terminal'; -let hasReceivedResponse: boolean = false; +let hasReceivedResponseFromRemoteExtHost: boolean = false; export class TerminalProcessExtHostProxy extends Disposable implements ITerminalChildProcess, ITerminalProcessExtHostProxy { @@ -65,7 +65,7 @@ export class TerminalProcessExtHostProxy extends Disposable implements ITerminal } public emitTitle(title: string): void { - hasReceivedResponse = true; + hasReceivedResponseFromRemoteExtHost = true; this._onProcessTitleChanged.fire(title); } @@ -108,20 +108,23 @@ export class TerminalProcessExtHostProxy extends Disposable implements ITerminal // Request a process if needed, if this is a virtual process this step can be skipped as // there is no real "process" and we know it's ready on the ext host already. if (this._shellLaunchConfig.isExtensionTerminal) { - this._terminalService.requestStartExtensionTerminal(this, this._cols, this._rows); - } else { - const env = await this._remoteAgentService.getEnvironment(); - if (!env) { - throw new Error('Could not fetch environment'); - } - this._terminalService.requestSpawnExtHostProcess(this, this._shellLaunchConfig, this._activeWorkspaceRootUri, this._cols, this._rows, this._configHelper.checkWorkspaceShellPermissions(env.os)); - if (!hasReceivedResponse) { - setTimeout(() => this._onProcessTitleChanged.fire(nls.localize('terminal.integrated.starting', "Starting...")), 0); - } + return this._terminalService.requestStartExtensionTerminal(this, this._cols, this._rows); } - // TODO: Wait and return - return undefined; + // Add a loading title if the extension host has not started yet as there could be a + // decent wait for the user + if (!hasReceivedResponseFromRemoteExtHost) { + setTimeout(() => this._onProcessTitleChanged.fire(nls.localize('terminal.integrated.starting', "Starting...")), 0); + } + + // Fetch the environment to check shell permissions + const env = await this._remoteAgentService.getEnvironment(); + if (!env) { + // Extension host processes are only allowed in remote extension hosts currently + throw new Error('Could not fetch remote environment'); + } + + return this._terminalService.requestSpawnExtHostProcess(this, this._shellLaunchConfig, this._activeWorkspaceRootUri, this._cols, this._rows, this._configHelper.checkWorkspaceShellPermissions(env.os)); } public shutdown(immediate: boolean): void { diff --git a/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts b/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts index f81b442e8b9..1fe17eecc7f 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts @@ -164,10 +164,6 @@ export class TerminalProcessManager extends Disposable implements ITerminalProce } this.processState = ProcessState.LAUNCHING; - const error = await this._process.start(); - if (error) { - return error; - } this._process.onProcessData(data => { const beforeProcessDataEvent: IBeforeProcessDataEvent = { data }; @@ -204,6 +200,11 @@ export class TerminalProcessManager extends Disposable implements ITerminalProce } }, LAUNCHING_DURATION); + const error = await this._process.start(); + if (error) { + return error; + } + return undefined; } diff --git a/src/vs/workbench/contrib/terminal/browser/terminalService.ts b/src/vs/workbench/contrib/terminal/browser/terminalService.ts index 40fdb32a03d..5fc901882ea 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalService.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalService.ts @@ -4,7 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import * as nls from 'vs/nls'; -import { TERMINAL_VIEW_ID, IShellLaunchConfig, ITerminalConfigHelper, ITerminalNativeService, ISpawnExtHostProcessRequest, IStartExtensionTerminalRequest, IAvailableShellsRequest, KEYBINDING_CONTEXT_TERMINAL_FOCUS, KEYBINDING_CONTEXT_TERMINAL_FIND_VISIBLE, KEYBINDING_CONTEXT_TERMINAL_IS_OPEN, ITerminalProcessExtHostProxy, IShellDefinition, LinuxDistro, KEYBINDING_CONTEXT_TERMINAL_SHELL_TYPE } from 'vs/workbench/contrib/terminal/common/terminal'; +import { TERMINAL_VIEW_ID, IShellLaunchConfig, ITerminalConfigHelper, ITerminalNativeService, ISpawnExtHostProcessRequest, IStartExtensionTerminalRequest, IAvailableShellsRequest, KEYBINDING_CONTEXT_TERMINAL_FOCUS, KEYBINDING_CONTEXT_TERMINAL_FIND_VISIBLE, KEYBINDING_CONTEXT_TERMINAL_IS_OPEN, ITerminalProcessExtHostProxy, IShellDefinition, LinuxDistro, KEYBINDING_CONTEXT_TERMINAL_SHELL_TYPE, ITerminalLaunchError } from 'vs/workbench/contrib/terminal/common/terminal'; import { IContextKeyService, IContextKey } from 'vs/platform/contextkey/common/contextkey'; import { IWorkbenchLayoutService } from 'vs/workbench/services/layout/browser/layoutService'; import { ILifecycleService } from 'vs/platform/lifecycle/common/lifecycle'; @@ -147,18 +147,23 @@ export class TerminalService implements ITerminalService { return activeInstance ? activeInstance : this.createTerminal(undefined); } - public async requestSpawnExtHostProcess(proxy: ITerminalProcessExtHostProxy, shellLaunchConfig: IShellLaunchConfig, activeWorkspaceRootUri: URI | undefined, cols: number, rows: number, isWorkspaceShellAllowed: boolean): Promise { + public async requestSpawnExtHostProcess(proxy: ITerminalProcessExtHostProxy, shellLaunchConfig: IShellLaunchConfig, activeWorkspaceRootUri: URI | undefined, cols: number, rows: number, isWorkspaceShellAllowed: boolean): Promise { await this._extensionService.whenInstalledExtensionsRegistered(); // Wait for the remoteAuthority to be ready (and listening for events) before firing // the event to spawn the ext host process const conn = this._remoteAgentService.getConnection(); const remoteAuthority = conn ? conn.remoteAuthority : 'null'; await this._whenExtHostReady(remoteAuthority); - this._onInstanceRequestSpawnExtHostProcess.fire({ proxy, shellLaunchConfig, activeWorkspaceRootUri, cols, rows, isWorkspaceShellAllowed }); + return new Promise(callback => { + this._onInstanceRequestSpawnExtHostProcess.fire({ proxy, shellLaunchConfig, activeWorkspaceRootUri, cols, rows, isWorkspaceShellAllowed, callback }); + }); } - public requestStartExtensionTerminal(proxy: ITerminalProcessExtHostProxy, cols: number, rows: number): void { - this._onInstanceRequestStartExtensionTerminal.fire({ proxy, cols, rows }); + public requestStartExtensionTerminal(proxy: ITerminalProcessExtHostProxy, cols: number, rows: number): Promise { + // The initial request came from the extension host, no need to wait for it + return new Promise(callback => { + this._onInstanceRequestStartExtensionTerminal.fire({ proxy, cols, rows, callback }); + }); } public async extHostReady(remoteAuthority: string): Promise { diff --git a/src/vs/workbench/contrib/terminal/common/terminal.ts b/src/vs/workbench/contrib/terminal/common/terminal.ts index ae04a256cb5..0f2cb5a4e1f 100644 --- a/src/vs/workbench/contrib/terminal/common/terminal.ts +++ b/src/vs/workbench/contrib/terminal/common/terminal.ts @@ -70,7 +70,6 @@ export const TERMINAL_ACTION_CATEGORY = nls.localize('terminalCategory', "Termin export const DEFAULT_LETTER_SPACING = 0; export const MINIMUM_LETTER_SPACING = -5; export const DEFAULT_LINE_HEIGHT = 1; -export const LEGACY_CONSOLE_MODE_EXIT_CODE = 3221225786; // microsoft/vscode#73790 export type FontWeight = 'normal' | 'bold' | '100' | '200' | '300' | '400' | '500' | '600' | '700' | '800' | '900'; @@ -361,12 +360,14 @@ export interface ISpawnExtHostProcessRequest { cols: number; rows: number; isWorkspaceShellAllowed: boolean; + callback: (error: ITerminalLaunchError | undefined) => void; } export interface IStartExtensionTerminalRequest { proxy: ITerminalProcessExtHostProxy; cols: number; rows: number; + callback: (error: ITerminalLaunchError | undefined) => void; } export interface IAvailableShellsRequest { diff --git a/src/vs/workbench/contrib/terminal/node/terminalProcess.ts b/src/vs/workbench/contrib/terminal/node/terminalProcess.ts index a3401030785..27e9ff1bafe 100644 --- a/src/vs/workbench/contrib/terminal/node/terminalProcess.ts +++ b/src/vs/workbench/contrib/terminal/node/terminalProcess.ts @@ -103,11 +103,11 @@ export class TerminalProcess extends Disposable implements ITerminalChildProcess try { const result = await stat(this._initialCwd); if (!result.isDirectory()) { - return { message: localize('launchFail.cwdNotDirectory', 'Starting directory (cwd) "{0}" is not a directory', this._initialCwd.toString()) }; + return { message: localize('launchFail.cwdNotDirectory', "Starting directory (cwd) \"{0}\" is not a directory", this._initialCwd.toString()) }; } } catch (err) { if (err?.code === 'ENOENT') { - return { message: localize('launchFail.cwdDoesNotExist', 'Starting directory (cwd) "{0}" does not exist', this._initialCwd.toString()) }; + return { message: localize('launchFail.cwdDoesNotExist', "Starting directory (cwd) \"{0}\" does not exist", this._initialCwd.toString()) }; } } return undefined; @@ -121,7 +121,7 @@ export class TerminalProcess extends Disposable implements ITerminalChildProcess try { const result = await stat(slc.executable); if (!result.isFile() && !result.isSymbolicLink()) { - return { message: localize('launchFail.executableIsNotFileOrSymlink', 'Shell path "{0}" is not a file of a symlink', slc.executable) }; + return { message: localize('launchFail.executableIsNotFileOrSymlink', "Shell path \"{0}\" is not a file of a symlink", slc.executable) }; } } catch (err) { if (err?.code === 'ENOENT') { @@ -130,7 +130,7 @@ export class TerminalProcess extends Disposable implements ITerminalChildProcess const envPaths: string[] | undefined = (slc.env && slc.env.PATH) ? slc.env.PATH.split(path.delimiter) : undefined; const executable = await findExecutable(slc.executable!, cwd, envPaths); if (!executable) { - return { message: localize('launchFail.executableDoesNotExist', 'Shell path "{0}" does not exist', slc.executable) }; + return { message: localize('launchFail.executableDoesNotExist', "Shell path \"{0}\" does not exist", slc.executable) }; } } } @@ -153,7 +153,6 @@ export class TerminalProcess extends Disposable implements ITerminalChildProcess } }); ptyProcess.onExit(e => { - console.log('onExit', e); this._exitCode = e.exitCode; this._queueProcessExit(); }); From d8a2d483da89b38ae4e08163d86db8bf9f76ba07 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 12 Jun 2020 11:15:55 -0700 Subject: [PATCH 06/13] Remove unneeded async --- .../api/browser/mainThreadTerminalService.ts | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/vs/workbench/api/browser/mainThreadTerminalService.ts b/src/vs/workbench/api/browser/mainThreadTerminalService.ts index e913ce5323d..81a63841a71 100644 --- a/src/vs/workbench/api/browser/mainThreadTerminalService.ts +++ b/src/vs/workbench/api/browser/mainThreadTerminalService.ts @@ -22,7 +22,7 @@ export class MainThreadTerminalService implements MainThreadTerminalServiceShape private _proxy: ExtHostTerminalServiceShape; private _remoteAuthority: string | null; private readonly _toDispose = new DisposableStore(); - private readonly _terminalProcesses = new Map>(); + private readonly _terminalProcessProxies = new Map(); private _dataEventTracker: TerminalDataEventTracker | undefined; private _linkHandler: IDisposable | undefined; @@ -230,7 +230,7 @@ export class MainThreadTerminalService implements MainThreadTerminalServiceShape } const proxy = request.proxy; - this._terminalProcesses.set(proxy.terminalId, Promise.resolve(proxy)); + this._terminalProcessProxies.set(proxy.terminalId, proxy); const shellLaunchConfigDto: IShellLaunchConfigDto = { name: request.shellLaunchConfig.name, executable: request.shellLaunchConfig.executable, @@ -258,7 +258,7 @@ export class MainThreadTerminalService implements MainThreadTerminalServiceShape private _onRequestStartExtensionTerminal(request: IStartExtensionTerminalRequest): void { const proxy = request.proxy; - this._terminalProcesses.set(proxy.terminalId, Promise.resolve(proxy)); + this._terminalProcessProxies.set(proxy.terminalId, proxy); // Note that onReisze is not being listened to here as it needs to fire when max dimensions // change, excluding the dimension override @@ -280,38 +280,38 @@ export class MainThreadTerminalService implements MainThreadTerminalServiceShape } public $sendProcessTitle(terminalId: number, title: string): void { - this._getTerminalProcess(terminalId).then(e => e.emitTitle(title)); + this._getTerminalProcess(terminalId).emitTitle(title); } public $sendProcessData(terminalId: number, data: string): void { - this._getTerminalProcess(terminalId).then(e => e.emitData(data)); + this._getTerminalProcess(terminalId).emitData(data); } public $sendProcessReady(terminalId: number, pid: number, cwd: string): void { - this._getTerminalProcess(terminalId).then(e => e.emitReady(pid, cwd)); + this._getTerminalProcess(terminalId).emitReady(pid, cwd); } public $sendProcessExit(terminalId: number, exitCode: number | undefined): void { - this._getTerminalProcess(terminalId).then(e => e.emitExit(exitCode)); - this._terminalProcesses.delete(terminalId); + this._getTerminalProcess(terminalId).emitExit(exitCode); + this._terminalProcessProxies.delete(terminalId); } public $sendOverrideDimensions(terminalId: number, dimensions: ITerminalDimensions | undefined): void { - this._getTerminalProcess(terminalId).then(e => e.emitOverrideDimensions(dimensions)); + this._getTerminalProcess(terminalId).emitOverrideDimensions(dimensions); } public $sendProcessInitialCwd(terminalId: number, initialCwd: string): void { - this._getTerminalProcess(terminalId).then(e => e.emitInitialCwd(initialCwd)); + this._getTerminalProcess(terminalId).emitInitialCwd(initialCwd); } public $sendProcessCwd(terminalId: number, cwd: string): void { - this._getTerminalProcess(terminalId).then(e => e.emitCwd(cwd)); + this._getTerminalProcess(terminalId).emitCwd(cwd); } public $sendResolvedLaunchConfig(terminalId: number, shellLaunchConfig: IShellLaunchConfig): void { const instance = this._terminalService.getInstanceFromId(terminalId); if (instance) { - this._getTerminalProcess(terminalId).then(e => e.emitResolvedShellLaunchConfig(shellLaunchConfig)); + this._getTerminalProcess(terminalId).emitResolvedShellLaunchConfig(shellLaunchConfig); } } @@ -324,7 +324,7 @@ export class MainThreadTerminalService implements MainThreadTerminalServiceShape sw.stop(); sum += sw.elapsed(); } - this._getTerminalProcess(terminalId).then(e => e.emitLatency(sum / COUNT)); + this._getTerminalProcess(terminalId).emitLatency(sum / COUNT); } private _isPrimaryExtHost(): boolean { @@ -349,8 +349,8 @@ export class MainThreadTerminalService implements MainThreadTerminalServiceShape } } - private _getTerminalProcess(terminalId: number): Promise { - const terminal = this._terminalProcesses.get(terminalId); + private _getTerminalProcess(terminalId: number): ITerminalProcessExtHostProxy { + const terminal = this._terminalProcessProxies.get(terminalId); if (!terminal) { throw new Error(`Unknown terminal: ${terminalId}`); } From 7ad78e6573a690ff1137e43441f977f260d97a49 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 12 Jun 2020 11:23:47 -0700 Subject: [PATCH 07/13] Only show error in tasks when exit code is non-zero --- .../workbench/contrib/terminal/browser/terminalInstance.ts | 4 ++++ src/vs/workbench/contrib/terminal/node/terminalProcess.ts | 7 ++----- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts index b696df22aae..ce53f2749eb 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts @@ -966,7 +966,11 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { // Create exit code message switch (typeof exitCodeOrError) { case 'number': + // Only show the error if the exit code is non-zero this._exitCode = exitCodeOrError; + if (this._exitCode === 0) { + break; + } let commandLine: string | undefined = undefined; if (this._shellLaunchConfig.executable) { diff --git a/src/vs/workbench/contrib/terminal/node/terminalProcess.ts b/src/vs/workbench/contrib/terminal/node/terminalProcess.ts index 27e9ff1bafe..502cbfacf58 100644 --- a/src/vs/workbench/contrib/terminal/node/terminalProcess.ts +++ b/src/vs/workbench/contrib/terminal/node/terminalProcess.ts @@ -78,7 +78,7 @@ export class TerminalProcess extends Disposable implements ITerminalChildProcess const results = await Promise.all([this._validateCwd(), this._validateExecutable()]); const firstError = results.find(r => r !== undefined); if (firstError) { - // TODO: Anything else we need to do here? + // TODO: Anything else we need to do here to tear down the terminal instance? // this._processStartupComplete = Promise.resolve(undefined); // this._onProcessExit.fire(0); @@ -89,10 +89,7 @@ export class TerminalProcess extends Disposable implements ITerminalChildProcess this.setupPtyProcess(this._shellLaunchConfig, this._ptyOptions); return undefined; } catch (err) { - - // TODO: Anything else we need to do here? - - // TODO: Extract code and message from the native exception, currently they're combined + // TODO: Anything else we need to do here to tear down the terminal instance? // A native exception occurred this._logService.trace('IPty#spawn native exception', err); return { message: `A native exception occurred during launch (${err.message})` }; From af056cd159a0427e682815ca48971b9fa26d8d02 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 12 Jun 2020 11:33:32 -0700 Subject: [PATCH 08/13] Remove resolved TODOs --- src/vs/workbench/contrib/terminal/node/terminalProcess.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/vs/workbench/contrib/terminal/node/terminalProcess.ts b/src/vs/workbench/contrib/terminal/node/terminalProcess.ts index 502cbfacf58..a66322eb69a 100644 --- a/src/vs/workbench/contrib/terminal/node/terminalProcess.ts +++ b/src/vs/workbench/contrib/terminal/node/terminalProcess.ts @@ -78,10 +78,6 @@ export class TerminalProcess extends Disposable implements ITerminalChildProcess const results = await Promise.all([this._validateCwd(), this._validateExecutable()]); const firstError = results.find(r => r !== undefined); if (firstError) { - // TODO: Anything else we need to do here to tear down the terminal instance? - // this._processStartupComplete = Promise.resolve(undefined); - // this._onProcessExit.fire(0); - return firstError; } @@ -89,8 +85,6 @@ export class TerminalProcess extends Disposable implements ITerminalChildProcess this.setupPtyProcess(this._shellLaunchConfig, this._ptyOptions); return undefined; } catch (err) { - // TODO: Anything else we need to do here to tear down the terminal instance? - // A native exception occurred this._logService.trace('IPty#spawn native exception', err); return { message: `A native exception occurred during launch (${err.message})` }; } From 0fff54abd653ba94273d1eb5bc426d5f5a538d39 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Fri, 12 Jun 2020 12:22:08 -0700 Subject: [PATCH 09/13] Remove todo --- .../workbench/contrib/terminal/browser/terminalInstance.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts index ce53f2749eb..1ff52b62114 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts @@ -910,11 +910,10 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { }); } - // Create the process asynchronously to allow the terminal's container - // to be created so dimensions are accurate + // Create the process asynchronously to allow the terminal's container to be created so + // dimensions are accurate this._processManager.createProcess(this._shellLaunchConfig, this._cols, this._rows, this._accessibilityService.isScreenReaderOptimized()).then(error => { if (error) { - // TODO: Tear down this._onProcessExit(error); } }); From d1e834a73e00584fe415ca198cd5f675974abd1c Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Sat, 13 Jun 2020 09:33:35 -0700 Subject: [PATCH 10/13] Don't show terminal in test Seems to be causing a Chromium error in containers related to gpu --- .../vscode-api-tests/src/singlefolder-tests/terminal.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts b/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts index 194fdd3b7f6..b3c617304d2 100644 --- a/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts +++ b/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts @@ -79,7 +79,6 @@ import { doesNotThrow, equal, ok, deepEqual, throws } from 'assert'; TEST: '`' } }); - terminal.show(); doesNotThrow(() => { // Print an environment variable value so the echo statement doesn't get matched if (process.platform === 'win32') { From 1a2bc0feb7bd9d203772e26947ea785dee4cfd80 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Sat, 13 Jun 2020 12:21:53 -0700 Subject: [PATCH 11/13] Try use dom renderer --- .../vscode-api-tests/src/singlefolder-tests/terminal.test.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts b/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts index b3c617304d2..6d62d40ec6c 100644 --- a/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts +++ b/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts @@ -22,6 +22,8 @@ import { doesNotThrow, equal, ok, deepEqual, throws } from 'assert'; await config.update('windowsEnableConpty', false, ConfigurationTarget.Global); // Disable exit alerts as tests may trigger then and we're not testing the notifications await config.update('showExitAlert', false, ConfigurationTarget.Global); + // Canvas can cause problems when running in a container + await config.update('rendererType', 'dom', ConfigurationTarget.Global); }); suite('Terminal', () => { @@ -79,6 +81,7 @@ import { doesNotThrow, equal, ok, deepEqual, throws } from 'assert'; TEST: '`' } }); + terminal.show(); doesNotThrow(() => { // Print an environment variable value so the echo statement doesn't get matched if (process.platform === 'win32') { From 4307ebe69b00289b3c7e1269c23eb5e84020bb6c Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Sun, 14 Jun 2020 05:38:44 -0700 Subject: [PATCH 12/13] Log to debug on ci --- .../src/singlefolder-tests/terminal.test.ts | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts b/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts index 6d62d40ec6c..517c7a28bbb 100644 --- a/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts +++ b/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts @@ -50,30 +50,44 @@ import { doesNotThrow, equal, ok, deepEqual, throws } from 'assert'; }); test('echo works in the default shell', (done) => { + console.log(1); disposables.push(window.onDidOpenTerminal(term => { + console.log(2); try { equal(terminal, term); } catch (e) { done(e); return; } + console.log(3); let data = ''; const dataDisposable = window.onDidWriteTerminalData(e => { + console.log(4); try { equal(terminal, e.terminal); } catch (e) { done(e); return; } + console.log(5); data += e.data; + console.log(6); if (data.indexOf(expected) !== 0) { + console.log(7); dataDisposable.dispose(); + console.log(8); terminal.dispose(); - disposables.push(window.onDidCloseTerminal(() => done())); + console.log(9); + disposables.push(window.onDidCloseTerminal(() => { + console.log(10); + done(); + })); + console.log(11); } }); disposables.push(dataDisposable); })); + console.log(12); // Use a single character to avoid winpty/conpty issues with injected sequences const expected = '`'; const terminal = window.createTerminal({ @@ -81,14 +95,18 @@ import { doesNotThrow, equal, ok, deepEqual, throws } from 'assert'; TEST: '`' } }); + console.log(13); terminal.show(); + console.log(14); doesNotThrow(() => { + console.log(15); // Print an environment variable value so the echo statement doesn't get matched if (process.platform === 'win32') { terminal.sendText(`$env:TEST`); } else { terminal.sendText(`echo $TEST`); } + console.log(16); }); }); From 63e04d0b073fbebc9113ef706ff5c75301784b20 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Sun, 14 Jun 2020 13:59:53 -0700 Subject: [PATCH 13/13] Remove logging --- .../src/singlefolder-tests/terminal.test.ts | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts b/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts index 517c7a28bbb..249c7d90cf6 100644 --- a/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts +++ b/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts @@ -22,7 +22,7 @@ import { doesNotThrow, equal, ok, deepEqual, throws } from 'assert'; await config.update('windowsEnableConpty', false, ConfigurationTarget.Global); // Disable exit alerts as tests may trigger then and we're not testing the notifications await config.update('showExitAlert', false, ConfigurationTarget.Global); - // Canvas can cause problems when running in a container + // Canvas may cause problems when running in a container await config.update('rendererType', 'dom', ConfigurationTarget.Global); }); @@ -50,44 +50,32 @@ import { doesNotThrow, equal, ok, deepEqual, throws } from 'assert'; }); test('echo works in the default shell', (done) => { - console.log(1); disposables.push(window.onDidOpenTerminal(term => { - console.log(2); try { equal(terminal, term); } catch (e) { done(e); return; } - console.log(3); let data = ''; const dataDisposable = window.onDidWriteTerminalData(e => { - console.log(4); try { equal(terminal, e.terminal); } catch (e) { done(e); return; } - console.log(5); data += e.data; - console.log(6); if (data.indexOf(expected) !== 0) { - console.log(7); dataDisposable.dispose(); - console.log(8); terminal.dispose(); - console.log(9); disposables.push(window.onDidCloseTerminal(() => { - console.log(10); done(); })); - console.log(11); } }); disposables.push(dataDisposable); })); - console.log(12); // Use a single character to avoid winpty/conpty issues with injected sequences const expected = '`'; const terminal = window.createTerminal({ @@ -95,18 +83,14 @@ import { doesNotThrow, equal, ok, deepEqual, throws } from 'assert'; TEST: '`' } }); - console.log(13); terminal.show(); - console.log(14); doesNotThrow(() => { - console.log(15); // Print an environment variable value so the echo statement doesn't get matched if (process.platform === 'win32') { terminal.sendText(`$env:TEST`); } else { terminal.sendText(`echo $TEST`); } - console.log(16); }); });