From d563b6b94bc378ab3003bef3dbdccdde86a9b546 Mon Sep 17 00:00:00 2001 From: Esther Perelman Date: Thu, 22 Apr 2021 16:21:19 +0300 Subject: [PATCH] Fix #2961: Output of short-lived tasks is not shown Signed-off-by: Esther Perelman --- packages/process/src/node/index.ts | 1 + .../src/node/process-backend-module.ts | 10 +++++ packages/process/src/node/process-manager.ts | 3 +- packages/process/src/node/raw-process.ts | 1 + .../process/src/node/task-terminal-process.ts | 41 +++++++++++++++++++ packages/process/src/node/terminal-process.ts | 13 +++++- packages/task/src/browser/quick-open-task.ts | 2 +- packages/task/src/browser/task-service.ts | 21 ++++++---- .../src/node/process/process-task-runner.ts | 8 ++-- .../src/browser/terminal-widget-impl.ts | 1 + .../src/common/base-terminal-protocol.ts | 3 +- .../terminal/src/node/base-terminal-server.ts | 14 ++++++- 12 files changed, 99 insertions(+), 19 deletions(-) create mode 100644 packages/process/src/node/task-terminal-process.ts diff --git a/packages/process/src/node/index.ts b/packages/process/src/node/index.ts index fb7f5b4b48699..11337aa6ba35c 100644 --- 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 index 5a030086515eb..d3b88df93be75 100644 --- 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,15 @@ export default new ContainerModule(bind => { } ); + bind(TaskTerminalProcess).toSelf().inTransientScope(); + bind(TaskTerminalProcessFactory).toFactory(ctx => + (options: TerminalProcessOptions) => { + const child = ctx.container.createChild(); + 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 index 91aad5ed56b89..832979aca869c 100644 --- 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 index 567457459a350..79cf59febc64a 100644 --- 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 new file mode 100644 index 0000000000000..24af23d37723e --- /dev/null +++ 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 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 index 0aeb3c8a2c246..cf350ad7d294a 100644 --- 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 index 67f003d83a630..1f94e90b9fee6 100644 --- 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 index 3225e45932e79..07cd32438f572 100644 --- a/packages/task/src/browser/task-service.ts +++ b/packages/task/src/browser/task-service.ts @@ -64,6 +64,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; @@ -156,6 +157,9 @@ export class TaskService implements TaskConfigurationClient { @inject(OpenerService) protected readonly openerService: OpenerService; + @inject(ShellTerminalServerProxy) + protected readonly shellTerminalServer: ShellTerminalServerProxy; + @inject(TaskNameResolver) protected readonly taskNameResolver: TaskNameResolver; @@ -952,8 +956,9 @@ export class TaskService implements TaskConfigurationClient { protected async runResolvedTask(resolvedTask: TaskConfiguration, option?: RunTaskOption): Promise { 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}`); @@ -964,13 +969,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); + } } } @@ -1025,11 +1033,7 @@ export class TaskService implements TaskConfigurationClient { terminal.sendText(selectedText); } - async attach(terminalId: number, taskId: number): Promise { - // 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 { let widgetOpenMode: WidgetOpenMode = 'open'; if (taskInfo) { const terminalWidget = this.terminalService.getByTerminalId(terminalId); @@ -1045,6 +1049,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(), @@ -1058,7 +1063,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 index 0b2979d31c8a7..65f30666c097e 100644 --- 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 index 162fe01deebc2..00227599b3864 100644 --- 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 index 7a25014a31d05..1e5e09d5e86b6 100644 --- 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 getCwdURI(id: number): Promise; resize(id: number, cols: number, rows: number): Promise; attach(id: number): Promise; + onAttachAttempted(id: number): Promise; close(id: number): Promise; getDefaultShell(): Promise; @@ -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 index 57f31cd139dd6..9251334fc546d 100644 --- 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 { + 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 https://github.com/eclipse-theia/theia/issues/2961 + terminal.unregisterProcess(); + } + } + } + async getProcessId(id: number): Promise { const terminal = this.processManager.get(id); if (!(terminal instanceof TerminalProcess)) {