From 1738cc823fe80362e298394f0eb384bb4e327cee Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Wed, 31 Oct 2018 17:51:15 -0400 Subject: [PATCH] Remove ProcessTaskRunner.findCommand, introduce process "started" event In the long term goal of handling properly shell tasks (allowing the user to specify "abc && def" as the command), we need to get rid of the findCommand method in ProcessTaskRunner. Otherwise, it will always return that the binary "abc && def" can't be found. Even for non-shell tasks, it doesn't seem ideal to implement ourselves the logic of looking through PATH. It's better to leave that to the OS. Instead, we want to rely on the underlying thing we use to run processes (either node's child_process or node-pty) to return us an appropriate error code. With node's child_process, we can do it out-of-the-box. With node-pty, we use our own fork available at [1] which adds the necessary features. On error, the Process emits an onError event. For the non-error case, we need to know when the process has been successfully started, so we can inform the client that the task has been successfully started. To do this, I added an 'onStart' event on Process. [1] https://github.com/theia-ide/node-pty/tree/theia Change-Id: Ie2db8909610129842912d312872a48aef3de23a5 Signed-off-by: Simon Marchi --- packages/process/package.json | 2 +- packages/process/src/node/process.ts | 31 ++++++- packages/process/src/node/raw-process.spec.ts | 87 ++++++++++--------- packages/process/src/node/raw-process.ts | 21 +++-- .../process/src/node/terminal-process.spec.ts | 75 ++++++++-------- packages/process/src/node/terminal-process.ts | 77 ++++++++++------ packages/task/src/browser/task-service.ts | 5 +- .../task/src/common/process/task-protocol.ts | 8 ++ .../src/node/process/process-task-runner.ts | 65 +++----------- .../task/src/node/process/process-task.ts | 1 - .../task/src/node/task-server.slow-spec.ts | 4 +- .../terminal/src/node/base-terminal-server.ts | 2 +- .../terminal/src/node/terminal-server.spec.ts | 34 ++------ packages/terminal/src/node/terminal-server.ts | 20 +++-- yarn.lock | 13 +-- 15 files changed, 225 insertions(+), 220 deletions(-) diff --git a/packages/process/package.json b/packages/process/package.json index 6d80a9da1ddbf..b9092eff0e1b5 100644 --- a/packages/process/package.json +++ b/packages/process/package.json @@ -4,7 +4,7 @@ "description": "Theia process support.", "dependencies": { "@theia/core": "^0.3.18", - "node-pty": "0.7.6", + "@theia/node-pty": "0.7.8-theia004", "string-argv": "^0.1.1" }, "publishConfig": { diff --git a/packages/process/src/node/process.ts b/packages/process/src/node/process.ts index 93f84c87ab6d2..da1fd1c4d0a40 100644 --- a/packages/process/src/node/process.ts +++ b/packages/process/src/node/process.ts @@ -24,6 +24,20 @@ export interface IProcessExitEvent { readonly signal?: string } +/** + * Data emitted when a process has been successfully started. + */ +export interface IProcessStartEvent { +} + +/** + * Data emitted when a process has failed to start. + */ +export interface ProcessErrorEvent { + /** An errno-like error string (e.g. ENOENT). */ + code: string; +} + export enum ProcessType { 'Raw', 'Terminal' @@ -61,8 +75,9 @@ export interface ForkOptions { export abstract class Process { readonly id: number; + protected readonly startEmitter: Emitter = new Emitter(); protected readonly exitEmitter: Emitter = new Emitter(); - protected readonly errorEmitter: Emitter = new Emitter(); + protected readonly errorEmitter: Emitter = new Emitter(); abstract readonly pid: number; protected _killed = false; @@ -81,14 +96,22 @@ export abstract class Process { return this._killed; } + get onStart(): Event { + return this.startEmitter.event; + } + get onExit(): Event { return this.exitEmitter.event; } - get onError(): Event { + get onError(): Event { return this.errorEmitter.event; } + protected emitOnStarted() { + this.startEmitter.fire({}); + } + /** * Emit the onExit event for this process. Only one of code and signal * should be defined. @@ -108,12 +131,12 @@ export abstract class Process { executable, this.options.args); } - protected emitOnError(err: Error) { + protected emitOnError(err: ProcessErrorEvent) { this.handleOnError(err); this.errorEmitter.fire(err); } - protected handleOnError(error: Error) { + protected handleOnError(error: ProcessErrorEvent) { this._killed = true; this.logger.error(error); } diff --git a/packages/process/src/node/raw-process.spec.ts b/packages/process/src/node/raw-process.spec.ts index 3bc6282872fe8..ebb6d80d0b054 100644 --- a/packages/process/src/node/raw-process.spec.ts +++ b/packages/process/src/node/raw-process.spec.ts @@ -22,6 +22,7 @@ import * as temp from 'temp'; import * as fs from 'fs'; import * as path from 'path'; import { isWindows } from '@theia/core'; +import { IProcessStartEvent, ProcessErrorEvent } from './process'; /* Allow to create temporary files, but delete them when we're done. */ const track = temp.track(); @@ -43,24 +44,21 @@ describe('RawProcess', function () { }); it('test error on non-existent path', async function () { - const p = new Promise((resolve, reject) => { - const rawProcess = rawProcessFactory({ command: '/non-existent' }); - rawProcess.onError(error => { - // tslint:disable-next-line:no-any - const code = (error as any).code; - resolve(code); - }); + const error = await new Promise((resolve, reject) => { + const proc = rawProcessFactory({ command: '/non-existent' }); + proc.onStart(reject); + proc.onError(resolve); }); - expect(await p).to.be.equal('ENOENT'); + expect(error.code).eq('ENOENT'); }); it('test error on non-executable path', async function () { - /* Create a non-executable file. */ + // Create a non-executable file. const f = track.openSync('non-executable'); fs.writeSync(f.fd, 'echo bob'); - /* Make really sure it's non-executable. */ + // Make really sure it's non-executable. let mode = fs.fstatSync(f.fd).mode; mode &= ~fs.constants.S_IXUSR; mode &= ~fs.constants.S_IXGRP; @@ -69,22 +67,25 @@ describe('RawProcess', function () { fs.closeSync(f.fd); - const p = new Promise((resolve, reject) => { - const rawProcess = rawProcessFactory({ command: f.path }); - rawProcess.onError(error => { - // tslint:disable-next-line:no-any - const code = (error as any).code; - resolve(code); - }); + const error = await new Promise((resolve, reject) => { + const proc = rawProcessFactory({ command: f.path }); + proc.onStart(reject); + proc.onError(resolve); }); - /* On Windows, we get 'UNKNOWN'. */ - let expectedCode = 'EACCES'; - if (isWindows) { - expectedCode = 'UNKNOWN'; - } + // On Windows, we get 'UNKNOWN'. + const expectedCode = isWindows ? 'UNKNOWN' : 'EACCES'; - expect(await p).to.equal(expectedCode); + expect(error.code).eq(expectedCode); + }); + + it('test start event', function () { + return new Promise(async (resolve, reject) => { + const args = ['-e', 'process.exit(3)']; + const rawProcess = rawProcessFactory({ command: process.execPath, 'args': args }); + rawProcess.onStart(resolve); + rawProcess.onError(reject); + }); }); it('test exit', async function () { @@ -109,45 +110,45 @@ describe('RawProcess', function () { }); it('test pipe stdout stream', async function () { - const args = ['--version']; - const rawProcess = rawProcessFactory({ command: process.execPath, 'args': args }); + const output = await new Promise(async (resolve, reject) => { + const args = ['-e', 'console.log("text to stdout")']; + const outStream = new stream.PassThrough(); + const rawProcess = rawProcessFactory({ command: process.execPath, 'args': args }); + rawProcess.onError(reject); - const outStream = new stream.PassThrough(); + rawProcess.output.pipe(outStream); - const p = new Promise((resolve, reject) => { - let version = ''; + let buf = ''; outStream.on('data', data => { - version += data.toString(); + buf += data.toString(); }); outStream.on('end', () => { - resolve(version.trim()); + resolve(buf.trim()); }); }); - rawProcess.output.pipe(outStream); - - expect(await p).to.be.equal(process.version); + expect(output).to.be.equal('text to stdout'); }); it('test pipe stderr stream', async function () { - const args = ['invalidarg']; - const rawProcess = rawProcessFactory({ command: process.execPath, 'args': args }); + const output = await new Promise(async (resolve, reject) => { + const args = ['-e', 'console.error("text to stderr")']; + const outStream = new stream.PassThrough(); + const rawProcess = rawProcessFactory({ command: process.execPath, 'args': args }); + rawProcess.onError(reject); - const outStream = new stream.PassThrough(); + rawProcess.errorOutput.pipe(outStream); - const p = new Promise((resolve, reject) => { - let version = ''; + let buf = ''; outStream.on('data', data => { - version += data.toString(); + buf += data.toString(); }); outStream.on('end', () => { - resolve(version.trim()); + resolve(buf.trim()); }); }); - rawProcess.errorOutput.pipe(outStream); - - expect(await p).to.have.string('Error'); + expect(output).to.be.equal('text to stderr'); }); it('test forked pipe stdout stream', async function () { diff --git a/packages/process/src/node/raw-process.ts b/packages/process/src/node/raw-process.ts index af7d9b0766a19..93307e99937d7 100644 --- a/packages/process/src/node/raw-process.ts +++ b/packages/process/src/node/raw-process.ts @@ -85,10 +85,11 @@ export class RawProcess extends Process { + ` with args: ${options.args ? options.args.join(' ') : ''}, ` + ` with options: ${JSON.stringify(options.options)}`); - /* spawn can throw exceptions, for example if the file is not - executable, it throws an error with EACCES. Here, we try to - normalize the error handling by calling the error handler - instead. */ + // About catching errors: spawn will sometimes throw directly + // (EACCES on Linux), sometimes return a Process object with the pid + // property undefined (ENOENT on Linux) and then emit an 'error' event. + // For now, we try to normalize that into always emitting an 'error' + // event. try { if (this.isForkOptions(options)) { this.process = fork( @@ -102,7 +103,11 @@ export class RawProcess extends Process { options.options); } - this.process.on('error', this.emitOnError.bind(this)); + this.process.on('error', (error: NodeJS.ErrnoException) => { + this.emitOnError({ + code: error.code || 'Unknown error', + }); + }); this.process.on('exit', (exitCode: number, signal: string) => { // node's child_process exit sets the unused parameter to null, // but we want it to be undefined instead. @@ -115,6 +120,12 @@ export class RawProcess extends Process { this.output = this.process.stdout; this.input = this.process.stdin; this.errorOutput = this.process.stderr; + + if (this.process.pid !== undefined) { + process.nextTick(() => { + this.emitOnStarted(); + }); + } } catch (error) { /* When an error is thrown, set up some fake streams, so the client code doesn't break because these field are undefined. */ diff --git a/packages/process/src/node/terminal-process.spec.ts b/packages/process/src/node/terminal-process.spec.ts index 6c27fb07ba1e9..f8979305a14be 100644 --- a/packages/process/src/node/terminal-process.spec.ts +++ b/packages/process/src/node/terminal-process.spec.ts @@ -18,7 +18,8 @@ import * as process from 'process'; import * as stream from 'stream'; import { createProcessTestContainer } from './test/process-test-container'; import { TerminalProcessFactory } from './terminal-process'; -import { isWindows } from '@theia/core/lib/common'; +import { IProcessExitEvent, ProcessErrorEvent } from './process'; +import { isWindows } from '@theia/core/lib/common/os'; /** * Globals @@ -36,58 +37,52 @@ describe('TerminalProcess', function () { }); it('test error on non existent path', async function () { + const error = await new Promise((resolve, reject) => { + const proc = terminalProcessFactory({ command: '/non-existent' }); + proc.onStart(reject); + proc.onError(resolve); + }); - /* Strangely, Linux returns exited with code 1 when using a non existing path but Windows throws an error. - This would need to be investigated more. */ - if (isWindows) { - return expect(() => terminalProcessFactory({ command: '/non-existent' })).to.throw(); - } else { - const terminalProcess = terminalProcessFactory({ command: '/non-existant' }); - const p = new Promise((resolve, reject) => { - terminalProcess.onError(error => { - reject(); - }); - - terminalProcess.onExit(event => { - if (event.code === undefined) { - reject(); - } + expect(error.code).eq('ENOENT'); + }); - resolve(event.code); - }); - }); + it('test error on trying to execute a directory', async function () { + const error = await new Promise((resolve, reject) => { + const proc = terminalProcessFactory({ command: __dirname }); + proc.onStart(reject); + proc.onError(resolve); + }); - const exitCode = await p; - expect(exitCode).equal(1); + if (isWindows) { + // On Windows, node-pty returns us a "File not found" message, so we can't really differentiate this case + // from trying to execute a non-existent file. node's child_process.spawn also returns ENOENT, so it's + // probably the best we can get. + expect(error.code).eq('ENOENT'); + } else { + expect(error.code).eq('EACCES'); } }); it('test exit', async function () { const args = ['--version']; - const terminalProcess = terminalProcessFactory({ command: process.execPath, 'args': args }); - const p = new Promise((resolve, reject) => { - terminalProcess.onError(error => { - reject(); - }); - terminalProcess.onExit(event => { - if (event.code === 0) { - resolve(); - } else { - reject(); - } - }); + const exit = await new Promise((resolve, reject) => { + const proc = terminalProcessFactory({ command: process.execPath, 'args': args }); + proc.onExit(resolve); + proc.onError(reject); }); - await p; + expect(exit.code).eq(0); }); it('test pipe stream', async function () { - const args = ['--version']; - const terminalProcess = terminalProcessFactory({ command: process.execPath, 'args': args }); + const v = await new Promise((resolve, reject) => { + const args = ['--version']; + const terminalProcess = terminalProcessFactory({ command: process.execPath, 'args': args }); + terminalProcess.onError(reject); + const outStream = new stream.PassThrough(); - const outStream = new stream.PassThrough(); + terminalProcess.createOutputStream().pipe(outStream); - const p = new Promise((resolve, reject) => { let version = ''; outStream.on('data', data => { version += data.toString(); @@ -99,9 +94,7 @@ describe('TerminalProcess', function () { }); }); - terminalProcess.createOutputStream().pipe(outStream); - /* Avoid using equal since terminal characters can be inserted at the end. */ - expect(await p).to.have.string(process.version); + expect(v).to.have.string(process.version); }); }); diff --git a/packages/process/src/node/terminal-process.ts b/packages/process/src/node/terminal-process.ts index 7ff08fea30c91..80abf8c165d3a 100644 --- a/packages/process/src/node/terminal-process.ts +++ b/packages/process/src/node/terminal-process.ts @@ -18,7 +18,7 @@ import { injectable, inject, named } from 'inversify'; import { ILogger } from '@theia/core/lib/common'; import { Process, ProcessType, ProcessOptions } from './process'; import { ProcessManager } from './process-manager'; -import { IPty, spawn } from 'node-pty'; +import { IPty, spawn } from '@theia/node-pty'; import { MultiRingBuffer, MultiRingBufferReadableStream } from './multi-ring-buffer'; import { signame } from './utils'; @@ -46,30 +46,57 @@ export class TerminalProcess extends Process { this.logger.debug('Starting terminal process', JSON.stringify(options, undefined, 2)); - this.terminal = spawn( - options.command, - options.args || [], - options.options || {}); - - this.terminal.on('exit', (code: number, signal?: number) => { - // Make sure to only pass either code or signal as !undefined, not - // both. - // - // node-pty quirk: On Linux/macOS, if the process exited through the - // exit syscall (with an exit code), signal will be 0 (an invalid - // signal value). If it was terminated because of a signal, the - // signal parameter will hold the signal number and code should - // be ignored. - if (signal === undefined || signal === 0) { - this.emitOnExit(code, undefined); - } else { - this.emitOnExit(undefined, signame(signal)); - } - }); - - this.terminal.on('data', (data: string) => { - ringBuffer.enq(data); - }); + try { + this.terminal = spawn( + options.command, + options.args || [], + options.options || {}); + + this.terminal.on('exec', (reason: string | undefined) => { + if (reason === undefined) { + this.emitOnStarted(); + } else { + this.emitOnError({ code: reason }); + } + }); + + this.terminal.on('exit', (code: number, signal?: number) => { + // Make sure to only pass either code or signal as !undefined, not + // both. + // + // node-pty quirk: On Linux/macOS, if the process exited through the + // exit syscall (with an exit code), signal will be 0 (an invalid + // signal value). If it was terminated because of a signal, the + // signal parameter will hold the signal number and code should + // be ignored. + if (signal === undefined || signal === 0) { + this.emitOnExit(code, undefined); + } else { + this.emitOnExit(undefined, signame(signal)); + } + }); + + this.terminal.on('data', (data: string) => { + ringBuffer.enq(data); + }); + } catch (err) { + // node-pty throws exceptions on Windows. + // Call the client error handler, but first give them a chance to register it. + process.nextTick(() => { + // Normalize the error to make it as close as possible as what + // node's child_process.spawn would generate in the same + // situation. + const message: string = err.message; + + if (message.startsWith('File not found: ')) { + err.errno = 'ENOENT'; + err.code = 'ENOENT'; + err.path = options.command; + } + + this.errorEmitter.fire(err); + }); + } } createOutputStream(): MultiRingBufferReadableStream { diff --git a/packages/task/src/browser/task-service.ts b/packages/task/src/browser/task-service.ts index c48646edce57f..ab966cc873922 100644 --- a/packages/task/src/browser/task-service.ts +++ b/packages/task/src/browser/task-service.ts @@ -189,8 +189,9 @@ export class TaskService implements TaskConfigurationClient { try { taskInfo = await this.taskServer.run(resolvedTask, this.getContext()); } catch (error) { - this.logger.error(`Error launching task '${taskLabel}': ${error}`); - this.messageService.error(`Error launching task '${taskLabel}': ${error}`); + const errorStr = `Error launching task '${taskLabel}': ${error.message}`; + this.logger.error(errorStr); + this.messageService.error(errorStr); return; } diff --git a/packages/task/src/common/process/task-protocol.ts b/packages/task/src/common/process/task-protocol.ts index 4eab880457484..306daa2118ee8 100644 --- a/packages/task/src/common/process/task-protocol.ts +++ b/packages/task/src/common/process/task-protocol.ts @@ -15,6 +15,7 @@ ********************************************************************************/ import { TaskConfiguration, TaskInfo } from '../task-protocol'; +import { ApplicationError } from '@theia/core/lib/common/application-error'; export type ProcessType = 'shell' | 'process'; @@ -47,3 +48,10 @@ export interface ProcessTaskInfo extends TaskInfo { /** terminal id. Defined if task is run as a terminal process */ readonly terminalId?: number, } + +export namespace ProcessTaskError { + export const CouldNotRun = ApplicationError.declare(1, (code: string) => ({ + message: `Error starting process (${code})`, + data: { code } + })); +} diff --git a/packages/task/src/node/process/process-task-runner.ts b/packages/task/src/node/process/process-task-runner.ts index e7501f2b5c4f9..9118e8508a48b 100644 --- a/packages/task/src/node/process/process-task-runner.ts +++ b/packages/task/src/node/process/process-task-runner.ts @@ -23,14 +23,15 @@ import { TerminalProcessOptions, RawProcessOptions, RawProcessFactory, - TerminalProcessFactory + TerminalProcessFactory, + ProcessErrorEvent, } from '@theia/process/lib/node'; import { TaskFactory } from './process-task'; import { TaskRunner } from '../task-runner'; import { Task } from '../task'; import { TaskConfiguration } from '../../common/task-protocol'; import * as fs from 'fs'; -import * as path from 'path'; +import { ProcessTaskError } from '../../common/process/task-protocol'; /** * Task runner that runs a task as a process or a command inside a shell. @@ -87,14 +88,6 @@ export class ProcessTaskRunner implements TaskRunner { env: process.env }; - // When we create a process to execute a command, it's difficult to know if it failed - // because the executable or script was not found, or if it was found, ran, and exited - // unsuccessfully. So here we look to see if it seems we can find a file of that name - // that is likely to be the one we want, before attempting to execute it. - const cmd = await this.findCommand(command, cwd); - if (!cmd) { - throw new Error(`Command not found: ${command}`); - } try { // use terminal or raw process let proc: TerminalProcess | RawProcess; @@ -115,9 +108,17 @@ export class ProcessTaskRunner implements TaskRunner { options: options }); } + + // Wait for the confirmation that the process is successfully started, or has failed to start. + await new Promise((resolve, reject) => { + proc.onStart(resolve); + proc.onError((error: ProcessErrorEvent) => { + reject(ProcessTaskError.CouldNotRun(error.code)); + }); + }); + return this.taskFactory({ label: taskConfig.label, - command: cmd, process: proc, processType: processType, context: ctx, @@ -136,47 +137,7 @@ export class ProcessTaskRunner implements TaskRunner { } /** - * Uses heuristics to look-for a command. Will look into the system path, if the command - * is given without a path. Will resolve if a potential match is found, else reject. There - * is no guarantee that a command we find will be the one executed, if multiple commands with - * the same name exist. - * @param command command name to look for - * @param cwd current working directory (as a fs path, not URI) - */ - protected async findCommand(command: string, cwd: string): Promise { - const systemPath = process.env.PATH; - const pathDelimiter = path.delimiter; - - if (path.isAbsolute(command)) { - if (await this.executableFileExists(command)) { - return command; - } - } else { - // look for command relative to cwd - const resolvedCommand = FileUri.fsPath(FileUri.create(cwd).resolve(command)); - - if (await this.executableFileExists(resolvedCommand)) { - return resolvedCommand; - } else { - // just a command to find in the system path? - if (path.basename(command) === command) { - // search for this command in the system path - if (systemPath !== undefined) { - const pathArray: string[] = systemPath.split(pathDelimiter); - - for (const p of pathArray) { - const candidate = FileUri.fsPath(FileUri.create(p).resolve(command)); - if (await this.executableFileExists(candidate)) { - return candidate; - } - } - } - } - } - } - } - - /** + * Remove ProcessTaskRunner.findCommand, introduce process "started" event * Checks for the existence of a file, at the provided path, and make sure that * it's readable and executable. */ diff --git a/packages/task/src/node/process/process-task.ts b/packages/task/src/node/process/process-task.ts index d309529728189..df5c8c5afc315 100644 --- a/packages/task/src/node/process/process-task.ts +++ b/packages/task/src/node/process/process-task.ts @@ -23,7 +23,6 @@ import { ProcessType, ProcessTaskInfo } from '../../common/process/task-protocol export const TaskProcessOptions = Symbol('TaskProcessOptions'); export interface TaskProcessOptions extends TaskOptions { - command: string, process: Process, processType: ProcessType } diff --git a/packages/task/src/node/task-server.slow-spec.ts b/packages/task/src/node/task-server.slow-spec.ts index aba2e603d822c..b443666c8bced 100644 --- a/packages/task/src/node/task-server.slow-spec.ts +++ b/packages/task/src/node/task-server.slow-spec.ts @@ -242,12 +242,12 @@ describe('Task server / back-end', function () { it('task using terminal process can handle command that does not exist', async function () { const p = taskServer.run(createProcessTaskConfig2('shell', bogusCommand, []), wsRoot); - await expectThrowsAsync(p, `Command not found: ${bogusCommand}`); + await expectThrowsAsync(p, 'ENOENT'); }); it('task using raw process can handle command that does not exist', async function () { const p = taskServer.run(createProcessTaskConfig2('process', bogusCommand, []), wsRoot); - await expectThrowsAsync(p, `Command not found: ${bogusCommand}`); + await expectThrowsAsync(p, 'ENOENT'); }); it('getTasks(ctx) returns tasks according to created context', async function () { diff --git a/packages/terminal/src/node/base-terminal-server.ts b/packages/terminal/src/node/base-terminal-server.ts index aac006eccca0d..6faf3cfa544e5 100644 --- a/packages/terminal/src/node/base-terminal-server.ts +++ b/packages/terminal/src/node/base-terminal-server.ts @@ -85,7 +85,7 @@ export abstract class BaseTerminalServer implements IBaseTerminalServer { if (this.client !== undefined) { this.client.onTerminalError({ 'terminalId': term.id, - 'error': error + 'error': new Error(`Failed to execute terminal process (${error.code})`), }); } })); diff --git a/packages/terminal/src/node/terminal-server.spec.ts b/packages/terminal/src/node/terminal-server.spec.ts index b477903b38768..36d70058b1a06 100644 --- a/packages/terminal/src/node/terminal-server.spec.ts +++ b/packages/terminal/src/node/terminal-server.spec.ts @@ -16,10 +16,7 @@ import * as chai from 'chai'; import { createTerminalTestContainer } from './test/terminal-test-container'; -import { TerminalWatcher } from '../common/terminal-watcher'; import { ITerminalServer } from '../common/terminal-protocol'; -import { IBaseTerminalExitEvent } from '../common/base-terminal-protocol'; -import { isWindows } from '@theia/core/lib/common'; /** * Globals @@ -31,41 +28,20 @@ describe('TermninalServer', function () { this.timeout(5000); let terminalServer: ITerminalServer; - let terminalWatcher: TerminalWatcher; beforeEach(() => { const container = createTerminalTestContainer(); terminalServer = container.get(ITerminalServer); - terminalWatcher = container.get(TerminalWatcher); }); it('test terminal create', async function () { const args = ['--version']; - const createResult = terminalServer.create({ command: process.execPath, 'args': args }); - expect(await createResult).to.be.greaterThan(-1); + const createResult = await terminalServer.create({ command: process.execPath, 'args': args }); + expect(createResult).to.be.greaterThan(-1); }); - it('test terminal create from non-existant path', async function () { - const createResult = terminalServer.create({ command: '/non-existant' }); - if (isWindows) { - expect(await createResult).to.be.equal(-1); - } else { - const errorPromise = new Promise((resolve, reject) => { - createResult.then((termId: number) => { - terminalWatcher.onTerminalExit((event: IBaseTerminalExitEvent) => { - if (event.terminalId === termId) { - if (event.code === 1) { - resolve(); - } else { - reject(); - } - } - }); - }); - }); - - expect(await createResult).to.be.greaterThan(-1); - await errorPromise; - } + it('test terminal create from non-existent path', async function () { + const createError = await terminalServer.create({ command: '/non-existent' }); + expect(createError).eq(-1); }); }); diff --git a/packages/terminal/src/node/terminal-server.ts b/packages/terminal/src/node/terminal-server.ts index cb832fd5dcb9a..afc526922c97c 100644 --- a/packages/terminal/src/node/terminal-server.ts +++ b/packages/terminal/src/node/terminal-server.ts @@ -34,14 +34,18 @@ export class TerminalServer extends BaseTerminalServer implements ITerminalServe super(processManager, logger); } - async create(options: ITerminalServerOptions): Promise { - try { + create(options: ITerminalServerOptions): Promise { + return new Promise((resolve, reject) => { const term = this.terminalFactory(options); - this.postCreate(term); - return term.id; - } catch (error) { - this.logger.error('Error while creating terminal', error); - return -1; - } + term.onStart(_ => { + this.postCreate(term); + resolve(term.id); + }); + term.onError(error => { + this.logger.error('Error while creating terminal', error); + resolve(-1); + }); + }); + } } diff --git a/yarn.lock b/yarn.lock index abaef30ef3769..1bec1632c0d47 100644 --- a/yarn.lock +++ b/yarn.lock @@ -118,6 +118,13 @@ dependencies: samsam "1.3.0" +"@theia/node-pty@0.7.8-theia004": + version "0.7.8-theia004" + resolved "https://registry.yarnpkg.com/@theia/node-pty/-/node-pty-0.7.8-theia004.tgz#0fe31b958df9315352d5fbeea7075047cf69c935" + integrity sha512-GetaD2p1qVPq/xbNCHCwKYjIr9IWjksf9V2iiv/hV6f885cJ+ie0Osr4+C159PrwzGRYW2jQVUtXghBJoyOCLg== + dependencies: + nan "2.10.0" + "@typefox/monaco-editor-core@^0.14.6": version "0.14.6" resolved "https://registry.yarnpkg.com/@typefox/monaco-editor-core/-/monaco-editor-core-0.14.6.tgz#32e378f3430913504ea9c7063944444a04429892" @@ -6694,12 +6701,6 @@ node-pre-gyp@^0.10.0: semver "^5.3.0" tar "^4" -node-pty@0.7.6: - version "0.7.6" - resolved "https://registry.yarnpkg.com/node-pty/-/node-pty-0.7.6.tgz#bff6148c9c5836ca7e73c7aaaec067dcbdac2f7b" - dependencies: - nan "2.10.0" - nodegit-promise@~4.0.0: version "4.0.0" resolved "https://registry.yarnpkg.com/nodegit-promise/-/nodegit-promise-4.0.0.tgz#5722b184f2df7327161064a791d2e842c9167b34"