Skip to content

Commit

Permalink
Remove ProcessTaskRunner.findCommand, introduce process "started" event
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@simark
Simon Marchi authored and simark committed Feb 14, 2019
1 parent 45c95f3 commit f24de1c
Show file tree
Hide file tree
Showing 15 changed files with 225 additions and 220 deletions.
2 changes: 1 addition & 1 deletion packages/process/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
"description": "Theia process support.",
"dependencies": {
"@theia/core": "^0.3.19",
"node-pty": "0.7.6",
"@theia/node-pty": "0.7.8-theia004",
"string-argv": "^0.1.1"
},
"publishConfig": {
Expand Down
31 changes: 27 additions & 4 deletions packages/process/src/node/process.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -61,8 +75,9 @@ export interface ForkOptions {
export abstract class Process {

readonly id: number;
protected readonly startEmitter: Emitter<IProcessStartEvent> = new Emitter<IProcessStartEvent>();
protected readonly exitEmitter: Emitter<IProcessExitEvent> = new Emitter<IProcessExitEvent>();
protected readonly errorEmitter: Emitter<Error> = new Emitter<Error>();
protected readonly errorEmitter: Emitter<ProcessErrorEvent> = new Emitter<ProcessErrorEvent>();
abstract readonly pid: number;
protected _killed = false;

Expand All @@ -81,14 +96,22 @@ export abstract class Process {
return this._killed;
}

get onStart(): Event<IProcessStartEvent> {
return this.startEmitter.event;
}

get onExit(): Event<IProcessExitEvent> {
return this.exitEmitter.event;
}

get onError(): Event<Error> {
get onError(): Event<ProcessErrorEvent> {
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.
Expand All @@ -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);
}
Expand Down
87 changes: 44 additions & 43 deletions packages/process/src/node/raw-process.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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<ProcessErrorEvent>((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;
Expand All @@ -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<ProcessErrorEvent>((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<IProcessStartEvent>(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 () {
Expand All @@ -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<string>(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<string>((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<string>(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<string>((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 () {
Expand Down
21 changes: 16 additions & 5 deletions packages/process/src/node/raw-process.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,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(
Expand All @@ -104,7 +105,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.
Expand All @@ -117,6 +122,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. */
Expand Down
75 changes: 34 additions & 41 deletions packages/process/src/node/terminal-process.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -36,58 +37,52 @@ describe('TerminalProcess', function () {
});

it('test error on non existent path', async function () {
const error = await new Promise<ProcessErrorEvent>((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<number>((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<ProcessErrorEvent>((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<IProcessExitEvent>((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<string>((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<string>((resolve, reject) => {
let version = '';
outStream.on('data', data => {
version += data.toString();
Expand All @@ -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);
});
});
Loading

0 comments on commit f24de1c

Please sign in to comment.