[process/task] Normalize tasks types and processes

When executing tasks, we now always use node-pty.

The difference between a process or a shell task relies in the way the
command and its arguments are interpreted/executed:

- process: ask the system to spawn a specific executable, with args
- shell: defer the spawning to a shell that will evaluate a command line

When spawning a process, the expected paramaters are of the kind:

	spawn(executable, [arg1, arg2, arg3, ...])

While when spawning a shell task, we expect some command like

	$> executable arg1 "arg 2" ... && sleep 1 || echo fail

This commit makes Theia tasks and "terminal processes" (processes
spawned via node-pty) able to use both inputs.

Signed-off-by: Paul Maréchal <[email protected]>

[process] Mark as killed when process errors out

Removes some unhandled errors from the tests because we used to not mark
processes that errored as `killed`.

Also renamed a potential name collision with the global `process` name.

Signed-off-by: Paul Maréchal <[email protected]>

[process] Add support for arguments in shell task

VS Code supports an extra `args` field for shell tasks, and will try its
best to build a command-line as `task.command ...task.args`, it will
also take the care of escaping some arguments. It is also possible to
explicitly define what escaping style you want the task runner to use.

This commit makes a best effort to comply to most use cases.

Signed-off-by: Paul Maréchal <[email protected]>

---

IMPORTANT: Disabled `Terminal Process { shell: true }` on Windows.

Problem is that only in this scenario, a lot of garbage ends up in the
standard output of the processes, and I have no idea why. In practice
this doesn't impact the actual terminals inside Theia.

So the tests are disabled on Windows by default, you can have them run
by setting an environment variable named `THEIA_PROCESS_TEST_OVERRIDE`.

packages/core/src/node/messaging/test/test-web-socket-channel.ts

@@ -35,9 +35,9 @@ export class TestWebSocketChannel extends WebSocketChannel {
         socket.on('close', (code, reason) =>
             this.fireClose(code, reason)
         );
-        socket.on('message', data =>
-            this.handleMessage(JSON.parse(data.toString()))
-        );
+        socket.on('message', data => {
+            this.handleMessage(JSON.parse(data.toString()));
+        });
         socket.on('open', () =>
             this.open(path)
         );

packages/debug/src/node/debug-adapter-factory.ts

@@ -55,8 +55,8 @@ export class LaunchBasedDebugAdapterFactory implements DebugAdapterFactory {
 
         // FIXME: propagate onError + onExit
         return {
-            input: process.input,
-            output: process.output,
+            input: process.inputStream,
+            output: process.outputStream,
             dispose: () => process.kill()
         };
     }

packages/file-search/src/node/file-search-service-impl.ts

@@ -108,12 +108,12 @@ export class FileSearchServiceImpl implements FileSearchService {
                 // TODO: why not just child_process.spawn, theia process are supposed to be used for user processes like tasks and terminals, not internal
                 const process = this.rawProcessFactory({ command: rgPath, args, options: { cwd } });
                 process.onError(reject);
-                process.output.on('close', resolve);
+                process.outputStream.on('close', resolve);
                 token.onCancellationRequested(() => process.kill());
 
                 const lineReader = readline.createInterface({
-                    input: process.output,
-                    output: process.input
+                    input: process.outputStream,
+                    output: process.inputStream
                 });
                 lineReader.on('line', line => {
                     if (token.isCancellationRequested) {

packages/languages/src/node/language-server-contribution.ts

@@ -78,7 +78,7 @@ export abstract class BaseLanguageServerContribution implements LanguageServerCo
 
         const process = await this.spawnProcessAsync(command, args, options);
         const [outSock, inSock] = await Promise.all([outSocket, inSocket]);
-        return createProcessSocketConnection(process.process, outSock, inSock);
+        return createProcessSocketConnection(process.process!, outSock, inSock);
     }
 
     /**
@@ -92,22 +92,22 @@ export abstract class BaseLanguageServerContribution implements LanguageServerCo
 
     protected async createProcessStreamConnectionAsync(command: string, args?: string[], options?: cp.SpawnOptions): Promise<IConnection> {
         const process = await this.spawnProcessAsync(command, args, options);
-        return createStreamConnection(process.output, process.input, () => process.kill());
+        return createStreamConnection(process.outputStream, process.inputStream, () => process.kill());
     }
 
     /**
      * @deprecated use `spawnProcessAsync` instead.
      */
     protected spawnProcess(command: string, args?: string[], options?: cp.SpawnOptions): RawProcess {
         const rawProcess = this.processFactory({ command, args, options });
-        rawProcess.process.once('error', this.onDidFailSpawnProcess.bind(this));
-        rawProcess.process.stderr.on('data', this.logError.bind(this));
+        rawProcess.onError(this.onDidFailSpawnProcess.bind(this));
+        rawProcess.errorStream.on('data', this.logError.bind(this));
         return rawProcess;
     }
 
     protected spawnProcessAsync(command: string, args?: string[], options?: cp.SpawnOptions): Promise<RawProcess> {
         const rawProcess = this.processFactory({ command, args, options });
-        rawProcess.process.stderr.on('data', this.logError.bind(this));
+        rawProcess.errorStream.on('data', this.logError.bind(this));
         return new Promise<RawProcess>((resolve, reject) => {
             rawProcess.onError((error: ProcessErrorEvent) => {
                 this.onDidFailSpawnProcess(error);

packages/process/src/node/dev-null-stream.ts

@@ -0,0 +1,34 @@
+/********************************************************************************
+ * Copyright (C) 2019 Ericsson and others.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v. 2.0 which is available at
+ * http://www.eclipse.org/legal/epl-2.0.
+ *
+ * This Source Code may also be made available under the following Secondary
+ * Licenses when the conditions for such availability set forth in the Eclipse
+ * Public License v. 2.0 are satisfied: GNU General Public License, version 2
+ * with the GNU Classpath Exception which is available at
+ * https://www.gnu.org/software/classpath/license.html.
+ *
+ * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
+ ********************************************************************************/
+
+import stream = require('stream');
+
+/**
+ * A Node stream like `/dev/null`.
+ *
+ * Writing goes to a black hole, reading returns `EOF`.
+ */
+export class DevNullStream extends stream.Duplex {
+    // tslint:disable-next-line:no-any
+    _write(chunk: any, encoding: string, callback: (err?: Error) => void): void {
+        callback();
+    }
+
+    _read(size: number): void {
+        // tslint:disable-next-line:no-null-keyword
+        this.push(null);
+    }
+}

packages/process/src/node/process.ts

@@ -17,6 +17,7 @@
 import { injectable, unmanaged } from 'inversify';
 import { ProcessManager } from './process-manager';
 import { ILogger, Emitter, Event } from '@theia/core/lib/common';
+import { Readable, Writable } from 'stream';
 
 export interface IProcessExitEvent {
     // Exactly one of code and signal will be set.
@@ -51,10 +52,13 @@ export enum ProcessType {
  *
  *   https://nodejs.org/api/child_process.html#child_process_child_process_spawn_command_args_options
  */
-export interface ProcessOptions {
+export interface ProcessOptions<T = string> {
     readonly command: string,
-    args?: string[],
-    options?: object
+    args?: T[],
+    options?: {
+        // tslint:disable-next-line:no-any
+        [key: string]: any
+    }
 }
 
 /**
@@ -78,9 +82,28 @@ export abstract class Process {
     protected readonly startEmitter: Emitter<IProcessStartEvent> = new Emitter<IProcessStartEvent>();
     protected readonly exitEmitter: Emitter<IProcessExitEvent> = new Emitter<IProcessExitEvent>();
     protected readonly errorEmitter: Emitter<ProcessErrorEvent> = new Emitter<ProcessErrorEvent>();
-    abstract readonly pid: number;
     protected _killed = false;
 
+    /**
+     * The OS process id.
+     */
+    abstract readonly pid: number;
+
+    /**
+     * The stdout stream.
+     */
+    abstract readonly outputStream: Readable;
+
+    /**
+     * The stderr stream.
+     */
+    abstract readonly errorStream: Readable;
+
+    /**
+     * The stdin stream.
+     */
+    abstract readonly inputStream: Writable;
+
     constructor(
         protected readonly processManager: ProcessManager,
         protected readonly logger: ILogger,
@@ -136,6 +159,10 @@ export abstract class Process {
         this.errorEmitter.fire(err);
     }
 
+    protected async emitOnErrorAsync(error: ProcessErrorEvent) {
+        process.nextTick(this.emitOnError.bind(this), error);
+    }
+
     protected handleOnError(error: ProcessErrorEvent) {
         this._killed = true;
         this.logger.error(error);

packages/process/src/node/raw-process.spec.ts

@@ -36,18 +36,24 @@ const FORK_TEST_FILE = path.join(__dirname, '../../src/node/test/process-fork-te
 
 describe('RawProcess', function () {
 
-    this.timeout(5000);
+    this.timeout(20_000);
+
     let rawProcessFactory: RawProcessFactory;
 
     beforeEach(() => {
         rawProcessFactory = createProcessTestContainer().get<RawProcessFactory>(RawProcessFactory);
     });
 
+    after(() => {
+        track.cleanupSync();
+    });
+
     it('test error on non-existent path', async function () {
         const error = await new Promise<ProcessErrorEvent>((resolve, reject) => {
             const proc = rawProcessFactory({ command: '/non-existent' });
             proc.onStart(reject);
             proc.onError(resolve);
+            proc.onExit(reject);
         });
 
         expect(error.code).eq('ENOENT');
@@ -71,6 +77,7 @@ describe('RawProcess', function () {
             const proc = rawProcessFactory({ command: f.path });
             proc.onStart(reject);
             proc.onError(resolve);
+            proc.onExit(reject);
         });
 
         // On Windows, we get 'UNKNOWN'.
@@ -85,6 +92,7 @@ describe('RawProcess', function () {
             const rawProcess = rawProcessFactory({ command: process.execPath, 'args': args });
             rawProcess.onStart(resolve);
             rawProcess.onError(reject);
+            rawProcess.onExit(reject);
         });
     });
 
@@ -116,7 +124,7 @@ describe('RawProcess', function () {
             const rawProcess = rawProcessFactory({ command: process.execPath, 'args': args });
             rawProcess.onError(reject);
 
-            rawProcess.output.pipe(outStream);
+            rawProcess.outputStream.pipe(outStream);
 
             let buf = '';
             outStream.on('data', data => {
@@ -137,7 +145,7 @@ describe('RawProcess', function () {
             const rawProcess = rawProcessFactory({ command: process.execPath, 'args': args });
             rawProcess.onError(reject);
 
-            rawProcess.errorOutput.pipe(outStream);
+            rawProcess.errorStream.pipe(outStream);
 
             let buf = '';
             outStream.on('data', data => {
@@ -167,7 +175,7 @@ describe('RawProcess', function () {
             });
         });
 
-        rawProcess.output.pipe(outStream);
+        rawProcess.outputStream.pipe(outStream);
 
         expect(await p).to.be.equal('1.0.0');
     });
@@ -187,7 +195,7 @@ describe('RawProcess', function () {
             });
         });
 
-        rawProcess.errorOutput.pipe(outStream);
+        rawProcess.errorStream.pipe(outStream);
 
         expect(await p).to.have.string('Error');
     });

packages/process/src/node/raw-process.ts

@@ -21,6 +21,10 @@ import { Process, ProcessType, ProcessOptions, ForkOptions, ProcessErrorEvent }
 import { ChildProcess, spawn, fork } from 'child_process';
 import * as stream from 'stream';
 
+// The class was here before, exporting to not break anything.
+export { DevNullStream } from './dev-null-stream';
+import { DevNullStream } from './dev-null-stream';
+
 export const RawProcessOptions = Symbol('RawProcessOptions');
 
 /**
@@ -50,30 +54,32 @@ export interface RawProcessFactory {
     (options: RawProcessOptions | RawForkOptions): RawProcess;
 }
 
-/**
- * A Node stream like `/dev/null`.
- *
- * Writing goes to a black hole, reading returns `EOF`.
- */
-class DevNullStream extends stream.Duplex {
-    // tslint:disable-next-line:no-any
-    _write(chunk: any, encoding: string, callback: (err?: Error) => void): void {
-        callback();
-    }
-
-    _read(size: number): void {
-        // tslint:disable-next-line:no-null-keyword
-        this.push(null);
-    }
-}
-
 @injectable()
 export class RawProcess extends Process {
 
-    readonly input: stream.Writable;
-    readonly output: stream.Readable;
-    readonly errorOutput: stream.Readable;
-    readonly process: ChildProcess;
+    /**
+     * @deprecated use `inputStream` instead.
+     */
+    get input(): stream.Writable { return this.inputStream; }
+
+    /**
+     * @deprecated use `outputStream` instead.
+     */
+    get output(): stream.Readable { return this.outputStream; }
+
+    /**
+     * @deprecated use `errorStream` instead.
+     */
+    get errorOutput(): stream.Readable { return this.errorStream; }
+
+    /**
+     * If the process fails to launch, it will be undefined.
+     */
+    readonly process: ChildProcess | undefined;
+
+    readonly outputStream: stream.Readable;
+    readonly errorStream: stream.Readable;
+    readonly inputStream: stream.Writable;
 
     constructor(
         @inject(RawProcessOptions) options: RawProcessOptions | RawForkOptions,
@@ -118,35 +124,34 @@ export class RawProcess extends Process {
                 );
             });
 
-            this.output = this.process.stdout || new DevNullStream();
-            this.input = this.process.stdin || new DevNullStream();
-            this.errorOutput = this.process.stderr || new DevNullStream();
+            this.outputStream = this.process.stdout || new DevNullStream();
+            this.inputStream = this.process.stdin || new DevNullStream();
+            this.errorStream = this.process.stderr || new DevNullStream();
 
             if (this.process.pid !== undefined) {
-                process.nextTick(() => {
-                    this.emitOnStarted();
-                });
+                process.nextTick(this.emitOnStarted.bind(this));
             }
         } catch (error) {
             /* When an error is thrown, set up some fake streams, so the client
                code doesn't break because these field are undefined.  */
-            this.output = new DevNullStream();
-            this.input = new DevNullStream();
-            this.errorOutput = new DevNullStream();
+            this.outputStream = new DevNullStream();
+            this.inputStream = new DevNullStream();
+            this.errorStream = new DevNullStream();
 
             /* Call the client error handler, but first give them a chance to register it.  */
-            process.nextTick(() => {
-                this.errorEmitter.fire(error);
-            });
+            this.emitOnErrorAsync(error);
         }
     }
 
     get pid() {
+        if (!this.process) {
+            throw new Error('process did not start correctly');
+        }
         return this.process.pid;
     }
 
     kill(signal?: string) {
-        if (this.killed === false) {
+        if (this.process && this.killed === false) {
             this.process.kill(signal);
         }
     }