From 6691f881fc6d9a031755ed79faa6bd4b543c9542 Mon Sep 17 00:00:00 2001 From: Cai Xuye Date: Fri, 8 May 2020 11:07:02 +0800 Subject: [PATCH] fix #2961: a workaround to show output and title of short-lived tasks. Signed-off-by: Cai Xuye --- packages/process/src/node/process-manager.ts | 22 ++++++++ packages/task/src/browser/quick-open-task.ts | 4 +- packages/task/src/browser/task-service.ts | 53 ++++++++----------- packages/task/src/common/task-protocol.ts | 16 +++++- .../terminal/src/node/base-terminal-server.ts | 40 +++++++------- 5 files changed, 81 insertions(+), 54 deletions(-) diff --git a/packages/process/src/node/process-manager.ts b/packages/process/src/node/process-manager.ts index 11f177ce42ba0..f42943b87476e 100644 --- a/packages/process/src/node/process-manager.ts +++ b/packages/process/src/node/process-manager.ts @@ -24,12 +24,15 @@ export class ProcessManager implements BackendApplicationContribution { protected id: number = 0; protected readonly processes: Map; + protected readonly willReapProcesses: Map; protected readonly deleteEmitter: Emitter; + protected readonly REAP_TIMEOUT: number = 1000; constructor( @inject(ILogger) @named('process') protected logger: ILogger ) { this.processes = new Map(); + this.willReapProcesses = new Map(); this.deleteEmitter = new Emitter(); } @@ -63,13 +66,32 @@ export class ProcessManager implements BackendApplicationContribution { } if (this.processes.delete(process.id)) { this.deleteEmitter.fire(process.id); + // Process will be recorded in willReapProcesses for a while so that it's still possible to get information of it before it's automatically reaped + this.willReapProcesses.set(process.id, process); + setTimeout(() => { + this.willReapProcesses.delete(process.id); + }, this.REAP_TIMEOUT); this.logger.debug(`The process was successfully unregistered. ${processLabel}`); } else { this.logger.warn(`This process was not registered or was already unregistered. ${processLabel}`); } } + /** + * Get a process given its id. The process could be either alive, or dead and will be reaped soon. + * + * @param id the process id. + */ get(id: number): Process | undefined { + return this.processes.get(id) || this.willReapProcesses.get(id); + } + + /** + * Get a process given its id. Only process marked alive will be returned. + * + * @param id the process id. + */ + getAlive(id: number): Process | undefined { return this.processes.get(id); } diff --git a/packages/task/src/browser/quick-open-task.ts b/packages/task/src/browser/quick-open-task.ts index 98da9ad7c48ab..b36b4b07698bd 100644 --- a/packages/task/src/browser/quick-open-task.ts +++ b/packages/task/src/browser/quick-open-task.ts @@ -214,7 +214,9 @@ export class QuickOpenTask implements QuickOpenModel, QuickOpenHandler { if (mode !== QuickOpenMode.OPEN) { return false; } - this.taskService.attach(task.terminalId!, task.taskId); + if (TaskInfo.isAttachableTaskInfo(task)) { + this.taskService.attach(task); + } return true; } }, diff --git a/packages/task/src/browser/task-service.ts b/packages/task/src/browser/task-service.ts index a8434eebe2d2e..ba510bf6a3837 100644 --- a/packages/task/src/browser/task-service.ts +++ b/packages/task/src/browser/task-service.ts @@ -48,7 +48,8 @@ import { DependsOrder, RevealKind, ApplyToKind, - TaskOutputPresentation + TaskOutputPresentation, + AttachableTaskInfo } from '../common'; import { TaskWatcher } from '../common/task-watcher'; import { ProvidedTaskConfigurations } from './provided-task-configurations'; @@ -929,14 +930,9 @@ export class TaskService implements TaskConfigurationClient { this.lastTask = { source, taskLabel, scope: resolvedTask._scope }; this.logger.debug(`Task created. Task id: ${taskInfo.taskId}`); - /** - * open terminal widget if the task is based on a terminal process (type: 'shell' or 'process') - * - * @todo Use a different mechanism to determine if the task should be attached? - * 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); + // Open terminal widget if the task is attachable. + if (TaskInfo.isAttachableTaskInfo(taskInfo)) { + this.attach(taskInfo); } return taskInfo; } catch (error) { @@ -997,24 +993,19 @@ 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(taskInfo: AttachableTaskInfo): Promise { let widgetOpenMode: WidgetOpenMode = 'open'; - if (taskInfo) { - const terminalWidget = this.terminalService.getByTerminalId(terminalId); - if (terminalWidget) { - this.messageService.error('Task is already running in terminal'); - return this.terminalService.open(terminalWidget, { mode: 'activate' }); - } - if (TaskOutputPresentation.shouldAlwaysRevealTerminal(taskInfo.config)) { - if (TaskOutputPresentation.shouldSetFocusToTerminal(taskInfo.config)) { // assign focus to the terminal if presentation.focus is true - widgetOpenMode = 'activate'; - } else { // show the terminal but not assign focus - widgetOpenMode = 'reveal'; - } + const { taskId, terminalId } = taskInfo; + const terminalWidget = this.terminalService.getByTerminalId(terminalId); + if (terminalWidget) { + this.messageService.error('Task is already running in terminal'); + return this.terminalService.open(terminalWidget, { mode: 'activate' }); + } + if (TaskOutputPresentation.shouldAlwaysRevealTerminal(taskInfo.config)) { + if (TaskOutputPresentation.shouldSetFocusToTerminal(taskInfo.config)) { // assign focus to the terminal if presentation.focus is true + widgetOpenMode = 'activate'; + } else { // show the terminal but not assign focus + widgetOpenMode = 'reveal'; } } // Create / find a terminal widget to display an execution output of a task that was launched as a command inside a shell. @@ -1026,11 +1017,11 @@ export class TaskService implements TaskConfigurationClient { : `Task: #${taskId}`, destroyTermOnClose: true }, { - taskId, - widgetOptions: { area: 'bottom' }, - mode: widgetOpenMode, - taskInfo - }); + taskId, + widgetOptions: { area: 'bottom' }, + mode: widgetOpenMode, + taskInfo + }); widget.start(terminalId); } diff --git a/packages/task/src/common/task-protocol.ts b/packages/task/src/common/task-protocol.ts index 542457e2301c6..36d6b9a384490 100644 --- a/packages/task/src/common/task-protocol.ts +++ b/packages/task/src/common/task-protocol.ts @@ -177,8 +177,6 @@ export interface TaskIdentifier { export interface TaskInfo { /** internal unique task id */ readonly taskId: number, - /** terminal id. Defined if task is run as a terminal process */ - readonly terminalId?: number, /** context that was passed as part of task creation, if any */ readonly ctx?: string, /** task config used for launching a task */ @@ -187,6 +185,20 @@ export interface TaskInfo { // eslint-disable-next-line @typescript-eslint/no-explicit-any readonly [key: string]: any; } +export namespace TaskInfo { + export function isAttachableTaskInfo(taskInfo: TaskInfo): taskInfo is AttachableTaskInfo { + return typeof taskInfo.terminalId === 'number'; + } +} + +/** + * @todo Use a different mechanism to determine if the task should be attached? + * Reason: Maybe a new task type wants to also be displayed in a terminal. + */ +export interface AttachableTaskInfo extends TaskInfo { + /** terminal id. Defined if task is run as a terminal process */ + readonly terminalId: number, +} export interface TaskServer extends JsonRpcServer { /** Run a task. Optionally pass a context. */ diff --git a/packages/terminal/src/node/base-terminal-server.ts b/packages/terminal/src/node/base-terminal-server.ts index c59cf2fa10e9c..bfeb400543872 100644 --- a/packages/terminal/src/node/base-terminal-server.ts +++ b/packages/terminal/src/node/base-terminal-server.ts @@ -41,10 +41,10 @@ export abstract class BaseTerminalServer implements IBaseTerminalServer { abstract create(options: IBaseTerminalServerOptions): Promise; async attach(id: number): Promise { - const term = this.processManager.get(id); + const terminal = this.processManager.get(id); - if (term && term instanceof TerminalProcess) { - return term.id; + if (terminal && terminal instanceof TerminalProcess) { + return terminal.id; } else { this.logger.error(`Couldn't attach - can't find terminal with id: ${id} `); return -1; @@ -53,7 +53,7 @@ export abstract class BaseTerminalServer implements IBaseTerminalServer { async getProcessId(id: number): Promise { const terminal = this.processManager.get(id); - if (!(terminal instanceof TerminalProcess)) { + if (!(terminal && terminal instanceof TerminalProcess)) { throw new Error(`terminal "${id}" does not exist`); } return terminal.pid; @@ -61,7 +61,7 @@ export abstract class BaseTerminalServer implements IBaseTerminalServer { async getProcessInfo(id: number): Promise { const terminal = this.processManager.get(id); - if (!(terminal instanceof TerminalProcess)) { + if (!(terminal && terminal instanceof TerminalProcess)) { throw new Error(`terminal "${id}" does not exist`); } return { @@ -71,18 +71,18 @@ export abstract class BaseTerminalServer implements IBaseTerminalServer { } async getCwdURI(id: number): Promise { - const terminal = this.processManager.get(id); - if (!(terminal instanceof TerminalProcess)) { + const terminal = this.processManager.getAlive(id); + if (!(terminal && terminal instanceof TerminalProcess)) { throw new Error(`terminal "${id}" does not exist`); } return terminal.getCwdURI(); } async close(id: number): Promise { - const term = this.processManager.get(id); + const terminal = this.processManager.getAlive(id); - if (term instanceof TerminalProcess) { - term.kill(); + if (terminal && terminal instanceof TerminalProcess) { + terminal.kill(); } } @@ -95,9 +95,9 @@ export abstract class BaseTerminalServer implements IBaseTerminalServer { } async resize(id: number, cols: number, rows: number): Promise { - const term = this.processManager.get(id); - if (term && term instanceof TerminalProcess) { - term.resize(cols, rows); + const terminal = this.processManager.getAlive(id); + if (terminal && terminal instanceof TerminalProcess) { + terminal.resize(cols, rows); } else { console.error("Couldn't resize terminal " + id + ", because it doesn't exist."); } @@ -108,31 +108,31 @@ export abstract class BaseTerminalServer implements IBaseTerminalServer { this.client = client; } - protected postCreate(term: TerminalProcess): void { + protected postCreate(terminal: TerminalProcess): void { const toDispose = new DisposableCollection(); - toDispose.push(term.onError(error => { - this.logger.error(`Terminal pid: ${term.pid} error: ${error}, closing it.`); + toDispose.push(terminal.onError(error => { + this.logger.error(`Terminal pid: ${terminal.pid} error: ${error}, closing it.`); if (this.client !== undefined) { this.client.onTerminalError({ - 'terminalId': term.id, + 'terminalId': terminal.id, 'error': new Error(`Failed to execute terminal process (${error.code})`), }); } })); - toDispose.push(term.onExit(event => { + toDispose.push(terminal.onExit(event => { if (this.client !== undefined) { this.client.onTerminalExitChanged({ - 'terminalId': term.id, + 'terminalId': terminal.id, 'code': event.code, 'signal': event.signal }); } })); - this.terminalToDispose.set(term.id, toDispose); + this.terminalToDispose.set(terminal.id, toDispose); } }