From 41bba3a0c3a891ab95e162fc262381aca795b878 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/process-manager.ts | 3 +-- packages/process/src/node/process.ts | 2 ++ packages/process/src/node/raw-process.ts | 1 + packages/process/src/node/terminal-process.ts | 15 +++++++++++++ packages/task/src/browser/quick-open-task.ts | 2 +- packages/task/src/browser/task-service.ts | 21 ++++++++++++------- .../src/browser/terminal-widget-impl.ts | 1 + .../src/common/base-terminal-protocol.ts | 4 +++- .../terminal/src/node/base-terminal-server.ts | 20 ++++++++++++++++++ 9 files changed, 57 insertions(+), 12 deletions(-) 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/process.ts b/packages/process/src/node/process.ts index d53b958408f17..9e8b21782d2f5 100644 --- a/packages/process/src/node/process.ts +++ b/packages/process/src/node/process.ts @@ -88,6 +88,8 @@ export abstract class Process { protected readonly closeEmitter: Emitter = new Emitter(); protected readonly errorEmitter: Emitter = new Emitter(); protected _killed = false; + public exited = false; + public attached = false; /** * The OS process id. 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/terminal-process.ts b/packages/process/src/node/terminal-process.ts index 0aeb3c8a2c246..e7546a2875d91 100644 --- a/packages/process/src/node/terminal-process.ts +++ b/packages/process/src/node/terminal-process.ts @@ -106,6 +106,13 @@ export class TerminalProcess extends Process { } else { this.emitOnExit(undefined, signame(signal)); } + this.onProcessExited(); + + // Unregister process only if terminal already attached, Fix issue #2961. + if (this.attached) { + this.unregisterProcess(); + } + process.nextTick(() => { if (signal === undefined || signal === 0) { this.emitOnClose(code, undefined); @@ -168,6 +175,14 @@ export class TerminalProcess extends Process { } } + unregisterProcess(): void { + this.processManager.unregister(this); + } + + onProcessExited(): void { + this.exited = true; + } + resize(cols: number, rows: number): void { this.checkTerminal(); this.terminal!.resize(cols, rows); 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 f44c73210b870..b84fd87b78c94 100644 --- 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 { 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.onFailedAttach(taskInfo.terminalId); + } } } @@ -1059,11 +1067,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); @@ -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/terminal/src/browser/terminal-widget-impl.ts b/packages/terminal/src/browser/terminal-widget-impl.ts index 162fe01deebc2..3a4fa95c4e395 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.onAttached(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..cfc1e9a051774 100644 --- a/packages/terminal/src/common/base-terminal-protocol.ts +++ b/packages/terminal/src/common/base-terminal-protocol.ts @@ -31,6 +31,8 @@ export interface IBaseTerminalServer extends JsonRpcServer getCwdURI(id: number): Promise; resize(id: number, cols: number, rows: number): Promise; attach(id: number): Promise; + onAttached(id: number): Promise; + onFailedAttach(id: number): Promise; close(id: number): Promise; getDefaultShell(): Promise; @@ -171,7 +173,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..15830a5b84366 100644 --- a/packages/terminal/src/node/base-terminal-server.ts +++ b/packages/terminal/src/node/base-terminal-server.ts @@ -68,6 +68,26 @@ export abstract class BaseTerminalServer implements IBaseTerminalServer { } } + async onAttached(id: number): Promise { + const terminal = this.processManager.get(id); + if (terminal instanceof TerminalProcess) { + terminal.attached = true; + if (terminal.exited) { + // Although terminal exited - waited with the `unregisterProcess` until here to enable attaching (Fix issue #2961). + terminal.unregisterProcess(); + } + } + } + + async onFailedAttach(id: number): Promise { + const terminal = this.processManager.get(id); + if (terminal instanceof TerminalProcess) { + if (terminal.exited) { + terminal.unregisterProcess(); + }; + } + } + async getProcessId(id: number): Promise { const terminal = this.processManager.get(id); if (!(terminal instanceof TerminalProcess)) {