Fix eclipse-theia#2961: Output of short-lived tasks is not shown

Signed-off-by: Esther Perelman <Esther.Perelman@sap.com>
EstherPerelman · May 9, 2021 · 1923945 · 1923945
1 parent a8dec8d
commit 1923945
Show file tree

Hide file tree

Showing 13 changed files with 103 additions and 20 deletions.
diff --git a/packages/process/src/node/index.ts b/packages/process/src/node/index.ts
@@ -18,4 +18,5 @@ export * from './process-manager';
 export * from './process';
 export * from './raw-process';
 export * from './terminal-process';
+export * from './task-terminal-process';
 export * from './multi-ring-buffer';
diff --git a/packages/process/src/node/process-backend-module.ts b/packages/process/src/node/process-backend-module.ts
@@ -17,6 +17,7 @@
 import { ContainerModule, Container } from '@theia/core/shared/inversify';
 import { RawProcess, RawProcessOptions, RawProcessFactory, RawForkOptions } from './raw-process';
 import { TerminalProcess, TerminalProcessOptions, TerminalProcessFactory } from './terminal-process';
+import { TaskTerminalProcess, TaskTerminalProcessFactory } from './task-terminal-process';
 import { BackendApplicationContribution } from '@theia/core/lib/node';
 import { ProcessManager } from './process-manager';
 import { ILogger } from '@theia/core/lib/common';
@@ -51,6 +52,17 @@ export default new ContainerModule(bind => {
         }
     );
 
+    bind(TaskTerminalProcess).toSelf().inTransientScope();
+    bind(TaskTerminalProcessFactory).toFactory(ctx =>
+        (options: TerminalProcessOptions) => {
+            const child = new Container({ defaultScope: 'Singleton' });
+            child.parent = ctx.container;
+
+            child.bind(TerminalProcessOptions).toConstantValue(options);
+            return child.get(TaskTerminalProcess);
+        }
+    );
+
     bind(MultiRingBuffer).toSelf().inTransientScope();
     /* 1MB size, TODO should be a user preference. */
     bind(MultiRingBufferOptions).toConstantValue({ size: 1048576 });

diff --git a/packages/process/src/node/process-manager.ts b/packages/process/src/node/process-manager.ts
@@ -42,7 +42,6 @@ export class ProcessManager implements BackendApplicationContribution {
     register(process: Process): number {
         const id = this.id;
         this.processes.set(id, process);
-        process.onExit(() => this.unregister(process));
         process.onError(() => this.unregister(process));
         this.id++;
         return id;
@@ -54,7 +53,7 @@ export class ProcessManager implements BackendApplicationContribution {
      *
      * @param process the process to unregister from this process manager.
      */
-    protected unregister(process: Process): void {
+    unregister(process: Process): void {
         const processLabel = this.getProcessLabel(process);
         this.logger.debug(`Unregistering process. ${processLabel}`);
         if (!process.killed) {

diff --git a/packages/process/src/node/raw-process.ts b/packages/process/src/node/raw-process.ts
@@ -128,6 +128,7 @@ export class RawProcess extends Process {
                     typeof exitCode === 'number' ? exitCode : undefined,
                     typeof signal === 'string' ? signal : undefined,
                 );
+                this.processManager.unregister(this);
             });
 
             this.process.on('close', (exitCode, signal) => {

diff --git a/packages/process/src/node/task-terminal-process.ts b/packages/process/src/node/task-terminal-process.ts
@@ -0,0 +1,41 @@
+/********************************************************************************
+ * Copyright (c) 2021 SAP SE or an SAP affiliate company 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 { injectable } from '@theia/core/shared/inversify';
+import { TerminalProcess, TerminalProcessOptions } from './terminal-process';
+
+export const TaskTerminalProcessFactory = Symbol('TaskTerminalProcessFactory');
+export interface TaskTerminalProcessFactory {
+    (options: TerminalProcessOptions): TaskTerminalProcess;
+}
+
+@injectable()
+export class TaskTerminalProcess extends TerminalProcess {
+
+    public exited = false;
+    public attachmentAttempted = false;
+
+    protected onTerminalExit(code: number | undefined, signal: string | undefined): void {
+        this.emitOnExit(code, signal);
+        this.exited = true;
+        // Unregister process only if task terminal already attached (or failed attach),
+        // Fixes issue [#2961](https://github.com/eclipse-theia/theia/issues/2961).
+        if (this.attachmentAttempted) {
+            this.unregisterProcess();
+        }
+    }
+
+}
diff --git a/packages/process/src/node/terminal-process.ts b/packages/process/src/node/terminal-process.ts
@@ -102,9 +102,9 @@ export class TerminalProcess extends Process {
                 // signal parameter will hold the signal number and code should
                 // be ignored.
                 if (signal === undefined || signal === 0) {
-                    this.emitOnExit(code, undefined);
+                    this.onTerminalExit(code, undefined);
                 } else {
-                    this.emitOnExit(undefined, signame(signal));
+                    this.onTerminalExit(undefined, signame(signal));
                 }
                 process.nextTick(() => {
                     if (signal === undefined || signal === 0) {
@@ -162,6 +162,15 @@ export class TerminalProcess extends Process {
         return this.options.args || [];
     }
 
+    protected onTerminalExit(code: number | undefined, signal: string | undefined): void {
+        this.emitOnExit(code, signal);
+        this.unregisterProcess();
+    }
+
+    unregisterProcess(): void {
+        this.processManager.unregister(this);
+    }
+
     kill(signal?: string): void {
         if (this.terminal && this.killed === false) {
             this.terminal.kill(signal);

diff --git a/packages/task/src/browser/quick-open-task.ts b/packages/task/src/browser/quick-open-task.ts
@@ -215,7 +215,7 @@ export class QuickOpenTask implements QuickOpenModel, QuickOpenHandler {
                                     if (mode !== QuickOpenMode.OPEN) {
                                         return false;
                                     }
-                                    this.taskService.attach(task.terminalId!, task.taskId);
+                                    this.taskService.attach(task.terminalId!, task);
                                     return true;
                                 }
                             },

diff --git a/packages/task/src/browser/task-service.ts b/packages/task/src/browser/task-service.ts
@@ -65,6 +65,7 @@ import { PROBLEMS_WIDGET_ID, ProblemWidget } from '@theia/markers/lib/browser/pr
 import { TaskNode } from './task-node';
 import { MonacoWorkspace } from '@theia/monaco/lib/browser/monaco-workspace';
 import { TaskTerminalWidgetManager } from './task-terminal-widget-manager';
+import { ShellTerminalServerProxy } from '@theia/terminal/lib/common/shell-terminal-protocol';
 
 export interface QuickPickProblemMatcherItem {
     problemMatchers: NamedProblemMatcher[] | undefined;
@@ -157,6 +158,9 @@ export class TaskService implements TaskConfigurationClient {
     @inject(OpenerService)
     protected readonly openerService: OpenerService;
 
+    @inject(ShellTerminalServerProxy)
+    protected readonly shellTerminalServer: ShellTerminalServerProxy;
+
     @inject(TaskNameResolver)
     protected readonly taskNameResolver: TaskNameResolver;
 
@@ -986,8 +990,9 @@ export class TaskService implements TaskConfigurationClient {
     protected async runResolvedTask(resolvedTask: TaskConfiguration, option?: RunTaskOption): Promise<TaskInfo | undefined> {
         const source = resolvedTask._source;
         const taskLabel = resolvedTask.label;
+        let taskInfo: TaskInfo | undefined;
         try {
-            const taskInfo = await this.taskServer.run(resolvedTask, this.getContext(), option);
+            taskInfo = await this.taskServer.run(resolvedTask, this.getContext(), option);
             this.lastTask = { source, taskLabel, scope: resolvedTask._scope };
             this.logger.debug(`Task created. Task id: ${taskInfo.taskId}`);
 
@@ -998,13 +1003,16 @@ export class TaskService implements TaskConfigurationClient {
              *       Reason: Maybe a new task type wants to also be displayed in a terminal.
              */
             if (typeof taskInfo.terminalId === 'number') {
-                this.attach(taskInfo.terminalId, taskInfo.taskId);
+                await this.attach(taskInfo.terminalId, taskInfo);
             }
             return taskInfo;
         } catch (error) {
             const errorStr = `Error launching task '${taskLabel}': ${error.message}`;
             this.logger.error(errorStr);
             this.messageService.error(errorStr);
+            if (taskInfo && typeof taskInfo.terminalId === 'number') {
+                this.shellTerminalServer.onAttachAttempted(taskInfo.terminalId);
+            }
         }
     }
 
@@ -1059,11 +1067,7 @@ export class TaskService implements TaskConfigurationClient {
         terminal.sendText(selectedText);
     }
 
-    async attach(terminalId: number, taskId: number): Promise<void> {
-        // Get the list of all available running tasks.
-        const runningTasks: TaskInfo[] = await this.getRunningTasks();
-        // Get the corresponding task information based on task id if available.
-        const taskInfo: TaskInfo | undefined = runningTasks.find((t: TaskInfo) => t.taskId === taskId);
+    async attach(terminalId: number, taskInfo: TaskInfo): Promise<number | void> {
         let widgetOpenMode: WidgetOpenMode = 'open';
         if (taskInfo) {
             const terminalWidget = this.terminalService.getByTerminalId(terminalId);
@@ -1079,6 +1083,7 @@ export class TaskService implements TaskConfigurationClient {
                 }
             }
         }
+        const { taskId } = taskInfo;
         // Create / find a terminal widget to display an execution output of a task that was launched as a command inside a shell.
         const widget = await this.taskTerminalWidgetManager.open({
             created: new Date().toString(),
@@ -1092,7 +1097,7 @@ export class TaskService implements TaskConfigurationClient {
             mode: widgetOpenMode,
             taskInfo
         });
-        widget.start(terminalId);
+        return widget.start(terminalId);
     }
 
     protected getTerminalWidgetId(terminalId: number): string | undefined {

diff --git a/packages/task/src/node/process/process-task-runner.ts b/packages/task/src/node/process/process-task-runner.ts
@@ -24,10 +24,10 @@ import { isWindows, isOSX, ILogger } from '@theia/core';
 import { FileUri } from '@theia/core/lib/node';
 import {
     RawProcessFactory,
-    TerminalProcessFactory,
     ProcessErrorEvent,
     Process,
     TerminalProcessOptions,
+    TaskTerminalProcessFactory,
 } from '@theia/process/lib/node';
 import {
     ShellQuotedString, ShellQuotingFunctions, BashQuotingFunctions, CmdQuotingFunctions, PowershellQuotingFunctions, createShellCommandLine, ShellQuoting,
@@ -53,8 +53,8 @@ export class ProcessTaskRunner implements TaskRunner {
     @inject(RawProcessFactory)
     protected readonly rawProcessFactory: RawProcessFactory;
 
-    @inject(TerminalProcessFactory)
-    protected readonly terminalProcessFactory: TerminalProcessFactory;
+    @inject(TaskTerminalProcessFactory)
+    protected readonly taskTerminalProcessFactory: TaskTerminalProcessFactory;
 
     @inject(TaskFactory)
     protected readonly taskFactory: TaskFactory;
@@ -73,7 +73,7 @@ export class ProcessTaskRunner implements TaskRunner {
             // - process: directly look for an executable and pass a specific set of arguments/options.
             // - shell: defer the spawning to a shell that will evaluate a command line with our executable.
             const terminalProcessOptions = this.getResolvedCommand(taskConfig);
-            const terminal: Process = this.terminalProcessFactory(terminalProcessOptions);
+            const terminal: Process = this.taskTerminalProcessFactory(terminalProcessOptions);
 
             // Wait for the confirmation that the process is successfully started, or has failed to start.
             await new Promise((resolve, reject) => {

diff --git a/packages/terminal/src/browser/terminal-widget-impl.ts b/packages/terminal/src/browser/terminal-widget-impl.ts
@@ -371,6 +371,7 @@ export class TerminalWidgetImpl extends TerminalWidget implements StatefulWidget
         this.connectTerminalProcess();
         if (IBaseTerminalServer.validateId(this.terminalId)) {
             this.onDidOpenEmitter.fire(undefined);
+            await this.shellTerminalServer.onAttachAttempted(this._terminalId);
             return this.terminalId;
         }
         this.onDidOpenFailureEmitter.fire(undefined);

diff --git a/packages/terminal/src/common/base-terminal-protocol.ts b/packages/terminal/src/common/base-terminal-protocol.ts
@@ -31,6 +31,7 @@ export interface IBaseTerminalServer extends JsonRpcServer<IBaseTerminalClient>
     getCwdURI(id: number): Promise<string>;
     resize(id: number, cols: number, rows: number): Promise<void>;
     attach(id: number): Promise<number>;
+    onAttachAttempted(id: number): Promise<void>;
     close(id: number): Promise<void>;
     getDefaultShell(): Promise<string>;
 
@@ -171,7 +172,7 @@ export interface MergedEnvironmentVariableCollection {
     /**
      * Applies this collection to a process environment.
      */
-    applyToProcessEnvironment(env: { [key: string]: string | null } ): void;
+    applyToProcessEnvironment(env: { [key: string]: string | null }): void;
 }
 
 export interface SerializableExtensionEnvironmentVariableCollection {

diff --git a/packages/terminal/src/node/base-terminal-server.ts b/packages/terminal/src/node/base-terminal-server.ts
@@ -30,7 +30,7 @@ import {
     EnvironmentVariableCollectionWithPersistence,
     SerializableExtensionEnvironmentVariableCollection
 } from '../common/base-terminal-protocol';
-import { TerminalProcess, ProcessManager } from '@theia/process/lib/node';
+import { TerminalProcess, ProcessManager, TaskTerminalProcess } from '@theia/process/lib/node';
 import { ShellProcess } from './shell-process';
 
 @injectable()
@@ -68,6 +68,18 @@ export abstract class BaseTerminalServer implements IBaseTerminalServer {
         }
     }
 
+    async onAttachAttempted(id: number): Promise<void> {
+        const terminal = this.processManager.get(id);
+        if (terminal instanceof TaskTerminalProcess) {
+            terminal.attachmentAttempted = true;
+            if (terminal.exited) {
+                // Didn't execute `unregisterProcess` on terminal `exit` event to enable attaching task output to terminal,
+                // Fixes issue [#2961](https://github.com/eclipse-theia/theia/issues/2961).
+                terminal.unregisterProcess();
+            }
+        }
+    }
+
     async getProcessId(id: number): Promise<number> {
         const terminal = this.processManager.get(id);
         if (!(terminal instanceof TerminalProcess)) {

diff --git a/packages/terminal/src/node/terminal-server.ts b/packages/terminal/src/node/terminal-server.ts
@@ -21,13 +21,14 @@ import {
     ITerminalServerOptions
 } from '../common/terminal-protocol';
 import { BaseTerminalServer } from './base-terminal-server';
-import { TerminalProcessFactory, ProcessManager } from '@theia/process/lib/node';
+import { TerminalProcessFactory, ProcessManager, TaskTerminalProcessFactory } from '@theia/process/lib/node';
 
 @injectable()
 export class TerminalServer extends BaseTerminalServer implements ITerminalServer {
 
     constructor(
         @inject(TerminalProcessFactory) protected readonly terminalFactory: TerminalProcessFactory,
+        @inject(TerminalProcessFactory) protected readonly taskTerminalFactory: TaskTerminalProcessFactory,
         @inject(ProcessManager) protected readonly processManager: ProcessManager,
         @inject(ILogger) @named('terminal') protected readonly logger: ILogger
     ) {