Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce proper custom execution lifecycle #9559

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/debug/src/browser/debug-session-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -478,7 +478,7 @@ export class DebugSessionManager {
return true;
}

const taskInfo = await this.taskService.runWorkspaceTask(this.taskService.startUserAction(), workspaceFolderUri, taskName);
const taskInfo = await this.taskService.runWorkspaceTask(await this.taskService.startUserAction(), workspaceFolderUri, taskName);
if (!checkErrors) {
return true;
}
Expand Down
3 changes: 2 additions & 1 deletion packages/plugin-ext/src/common/plugin-api-rpc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1793,10 +1793,11 @@ export const MAIN_RPC_CONTEXT = {
};

export interface TasksExt {
$onDidStartUserInteraction(): Promise<void>;
$provideTasks(handle: number): Promise<TaskDto[] | undefined>;
$resolveTask(handle: number, task: TaskDto, token?: CancellationToken): Promise<TaskDto | undefined>;
$onDidStartTask(execution: TaskExecutionDto, terminalId: number): void;
$onDidEndTask(id: number): void;
$onDidEndTask(execution: TaskExecutionDto): void;
$onDidStartTaskProcess(processId: number | undefined, execution: TaskExecutionDto): void;
$onDidEndTaskProcess(exitCode: number | undefined, taskId: number): void;
}
Expand Down
13 changes: 11 additions & 2 deletions packages/plugin-ext/src/main/browser/tasks-main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import { TaskInfo, TaskExitedEvent, TaskConfiguration, TaskCustomization, TaskOu
import { TaskWatcher } from '@theia/task/lib/common/task-watcher';
import { TaskService } from '@theia/task/lib/browser/task-service';
import { TaskDefinitionRegistry } from '@theia/task/lib/browser';
import { ProvidedTaskConfigurations, TaskStartUserInteractionEvent } from '@theia/task/lib/browser/provided-task-configurations';

const revealKindMap = new Map<number | RevealKind, RevealKind | number>(
[
Expand Down Expand Up @@ -72,6 +73,11 @@ export class TasksMainImpl implements TasksMain, Disposable {
this.taskService = container.get(TaskService);
this.taskDefinitionRegistry = container.get(TaskDefinitionRegistry);

this.toDispose.push(container.get(ProvidedTaskConfigurations).onDidStartUserInteraction((event: TaskStartUserInteractionEvent) => {
// we must wait with further processing until the plugin side has had time to clean up its garbage
event.waitUntil(this.proxy.$onDidStartUserInteraction());
}));

this.toDispose.push(this.taskWatcher.onTaskCreated((event: TaskInfo) => {
this.proxy.$onDidStartTask({
id: event.taskId,
Expand All @@ -80,7 +86,10 @@ export class TasksMainImpl implements TasksMain, Disposable {
}));

this.toDispose.push(this.taskWatcher.onTaskExit((event: TaskExitedEvent) => {
this.proxy.$onDidEndTask(event.taskId);
this.proxy.$onDidEndTask({
id: event.taskId,
task: this.fromTaskConfiguration(event.config)
});
}));

this.toDispose.push(this.taskWatcher.onDidStartTaskProcess((event: TaskInfo) => {
Expand Down Expand Up @@ -128,7 +137,7 @@ export class TasksMainImpl implements TasksMain, Disposable {
return [];
}

const token: number = this.taskService.startUserAction();
const token: number = await this.taskService.startUserAction();
const [configured, provided] = await Promise.all([
this.taskService.getConfiguredTasks(token),
this.taskService.getProvidedTasks(token)
Expand Down
158 changes: 100 additions & 58 deletions packages/plugin-ext/src/plugin/tasks/tasks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,23 +25,20 @@ import * as theia from '@theia/plugin';
import * as converter from '../type-converters';
import { CustomExecution, Disposable } from '../types-impl';
import { RPCProtocol, ConnectionClosedError } from '../../common/rpc-protocol';
import { TaskProviderAdapter } from './task-provider';
tsmaeder marked this conversation as resolved.
Show resolved Hide resolved
import { Emitter, Event } from '@theia/core/lib/common/event';
import { TerminalServiceExtImpl } from '../terminal-ext';
import { UUID } from '@theia/core/shared/@phosphor/coreutils';
import { CancellationToken } from '@theia/core/lib/common/cancellation';

type ExecutionCallback = (resolvedDefinition: theia.TaskDefinition) => Thenable<theia.Pseudoterminal>;
export class TasksExtImpl implements TasksExt {
private proxy: TasksMain;

private callId = 0;
private adaptersMap = new Map<number, TaskProviderAdapter>();
private providersByHandle = new Map<number, theia.TaskProvider>();
private executions = new Map<number, theia.TaskExecution>();
protected callbackIdBase: string = UUID.uuid4();
protected callbackId: number;
protected customExecutionIds: Map<ExecutionCallback, string> = new Map();
protected customExecutionFunctions: Map<string, ExecutionCallback> = new Map();
protected callbackId: number = 0;
protected providedCustomExecutions: Map<string, theia.CustomExecution> = new Map();
protected oneOffCustomExecutions: Map<string, theia.CustomExecution> = new Map();
tsmaeder marked this conversation as resolved.
Show resolved Hide resolved
protected lastStartedTask: number | undefined;

private readonly onDidExecuteTask: Emitter<theia.TaskStartEvent> = new Emitter<theia.TaskStartEvent>();
Expand All @@ -68,11 +65,37 @@ export class TasksExtImpl implements TasksExt {
return this.onDidExecuteTask.event;
}

async $onDidStartUserInteraction(): Promise<void> {
console.info(`$onDidStartUserInteraction: clearing ${this.providedCustomExecutions.size} custom executions`);
tsmaeder marked this conversation as resolved.
Show resolved Hide resolved
this.providedCustomExecutions.clear();
return Promise.resolve();
}

private getCustomExecution(id: string | undefined): theia.CustomExecution | undefined {
if (!id) {
return undefined;
}
return this.providedCustomExecutions.get(id) ?? this.oneOffCustomExecutions.get(id);
}

private addProvidedCustomExecution(execution: theia.CustomExecution): string {
const id = this.nextCallbackId();
this.providedCustomExecutions.set(id, execution);
return id;
}

private addOneOffCustomExecution(execution: theia.CustomExecution): string {
const id = this.nextCallbackId();
this.oneOffCustomExecutions.set(id, execution);
return id;
}

async $onDidStartTask(execution: TaskExecutionDto, terminalId: number): Promise<void> {
const customExecution = this.customExecutionFunctions.get(execution.task.executionId || '');
const customExecution = this.getCustomExecution(execution.task.executionId);
if (customExecution) {
console.info(`running custom execution with id ${execution.task.executionId}`);
const taskDefinition = converter.toTask(execution.task).definition;
const pty = await customExecution(taskDefinition);
const pty = await customExecution.callback(taskDefinition);
this.terminalExt.attachPtyToTerminal(terminalId, pty);
if (pty.onDidClose) {
const disposable = pty.onDidClose((e: number | void = undefined) => {
Expand All @@ -93,13 +116,19 @@ export class TasksExtImpl implements TasksExt {
return this.onDidTerminateTask.event;
}

$onDidEndTask(id: number): void {
const taskExecution = this.executions.get(id);
$onDidEndTask(executionDto: TaskExecutionDto): void {
const taskExecution = this.executions.get(executionDto.id);
if (!taskExecution) {
throw new Error(`Task execution with id ${id} is not found`);
throw new Error(`Task execution with id ${executionDto.id} is not found`);
}

if (executionDto.task.executionId) {
if (this.oneOffCustomExecutions.delete(executionDto.task.executionId)) {
console.info(`removed one-off custom execution with id ${executionDto.task.executionId}`);
}
}

this.executions.delete(id);
this.executions.delete(executionDto.id);

this.onDidTerminateTask.fire({
execution: taskExecution
Expand Down Expand Up @@ -134,7 +163,7 @@ export class TasksExtImpl implements TasksExt {
}

registerTaskProvider(type: string, provider: theia.TaskProvider): theia.Disposable {
const callId = this.addNewAdapter(new TaskProviderAdapter(provider));
const callId = this.addProvider(provider);
this.proxy.$registerTaskProvider(callId, type);
return this.createDisposable(callId);
}
Expand All @@ -153,7 +182,7 @@ export class TasksExtImpl implements TasksExt {
// in the provided custom execution map that is cleaned up after the
// task is executed.
if (CustomExecution.is(task.execution!)) {
taskDto.executionId = this.addCustomExecution(task.execution!.callback);
taskDto.executionId = this.addOneOffCustomExecution(task.execution);
}
const executionDto = await this.proxy.$executeTask(taskDto);
if (executionDto) {
Expand All @@ -166,52 +195,67 @@ export class TasksExtImpl implements TasksExt {
}

$provideTasks(handle: number): Promise<TaskDto[] | undefined> {
const adapter = this.adaptersMap.get(handle);
if (adapter) {
return adapter.provideTasks(CancellationToken.None).then(tasks => {
const provider = this.providersByHandle.get(handle);
let addedExecutions = 0;
if (provider) {
return Promise.resolve(provider.provideTasks(CancellationToken.None)).then(tasks => {
if (tasks) {
for (const task of tasks) {
if (task.taskType === 'customExecution') {
task.executionId = this.addCustomExecution(task.callback);
task.callback = undefined;
return tasks.map(task => {
const dto = converter.fromTask(task);
if (dto && CustomExecution.is(task.execution!)) {
dto.executionId = this.addProvidedCustomExecution(task.execution);
addedExecutions++;
}
}
return dto;
}).filter((task): task is TaskDto => !!task);
} else {
return undefined;
}
}).then(tasks => {
console.info(`provideTasks: added ${addedExecutions} executions for provider ${handle}`);
return tasks;
});
} else {
return Promise.reject(new Error('No adapter found to provide tasks'));
return Promise.reject(new Error(`No task provider found for handle ${handle} `));
}
}

$resolveTask(handle: number, task: TaskDto, token: theia.CancellationToken): Promise<TaskDto | undefined> {
const adapter = this.adaptersMap.get(handle);
if (adapter) {
return adapter.resolveTask(task, token).then(resolvedTask => {
if (resolvedTask && resolvedTask.taskType === 'customExecution') {
resolvedTask.executionId = this.addCustomExecution(resolvedTask.callback);
resolvedTask.callback = undefined;
$resolveTask(handle: number, dto: TaskDto, token: theia.CancellationToken): Promise<TaskDto | undefined> {
const provider = this.providersByHandle.get(handle);
if (provider) {
const task = converter.toTask(dto);
if (task) {
const resolvedTask = provider.resolveTask(task, token);
if (resolvedTask) {
return Promise.resolve(resolvedTask).then(maybeResolvedTask => {
if (maybeResolvedTask) {
const resolvedDto = converter.fromTask(maybeResolvedTask);
if (resolvedDto && CustomExecution.is(maybeResolvedTask.execution)) {
resolvedDto.executionId = this.addProvidedCustomExecution(maybeResolvedTask.execution);
console.info('resolveTask: added custom execution');
}
return resolvedDto;
}
return undefined;
});
}
return resolvedTask;
});
}
return Promise.resolve(undefined);

} else {
return Promise.reject(new Error('No adapter found to resolve task'));
return Promise.reject(new Error('No provider found to resolve task'));
}
}

private addNewAdapter(adapter: TaskProviderAdapter): number {
const callId = this.nextCallId();
this.adaptersMap.set(callId, adapter);
private addProvider(provider: theia.TaskProvider): number {
const callId = this.callId++;
this.providersByHandle.set(callId, provider);
return callId;
}

private nextCallId(): number {
return this.callId++;
}

private createDisposable(callId: number): theia.Disposable {
return new Disposable(() => {
this.adaptersMap.delete(callId);
this.providersByHandle.delete(callId);
this.proxy.$unregister(callId);
});
}
Expand All @@ -228,33 +272,31 @@ export class TasksExtImpl implements TasksExt {
}
}

private toTaskExecution(execution: TaskExecutionDto): theia.TaskExecution {
const result = {
task: converter.toTask(execution.task),
terminate: () => {
this.proxy.$terminateTask(execution.id);
}
};
if (execution.task.executionId) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that you use executionId field to check if it's CustomExecution task.
So - if only tasks with CustomExecution have this field - should we provide some clear/more readable way for that...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you have in mind? I'm not trying to do any typing here.

Copy link
Contributor

@RomanNikitenko RomanNikitenko Aug 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only tasks with CustomExecution have this field

is it true?

Copy link
Contributor

@RomanNikitenko RomanNikitenko Sep 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know a task of any type (not only customExecution) can have execution - the object is created when a task is started ($onDidStartTask() => getTaskExecution()) and is stored to the map with executions using executionId.

I see that:

  • within one of your previous PRs executionId field was added to the TaskDto interface
  • within current PR you are using the field as a marker of a task with customExecution
    first, you create the field, like:
if (dto && CustomExecution.is(task.execution!)) {
    dto.executionId = this.addProvidedCustomExecution(task.execution);

and in the line 281 above you check - if the field exists - then result.task.execution = this.getCustomExecution(execution.task.executionId);)

So, in my comment above I meant that for me it's not obvious that executionId field of the TaskDto interface is a marker for customExecution tasks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for me it's not obvious that executionId field of the TaskDto interface is a marker for customExecution tasks.

If you think the executionId field is named confusingly, I'm all up for renaming it to something more clear. But since it wasn't introduced in this PR, it's not a concern for this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • it's not a concern for this PR

sorry, but for me it looks like within current PR you are trying to use common for all tasks field as a marker for customExecution task. My first comment here was about if (execution.task.executionId).

If I'm not mistaken, this field was introduced within one of your previous PR. If so, it's important to know what you think about it, what intention was...

result.task.execution = this.getCustomExecution(execution.task.executionId);
}
return result;
}

private getTaskExecution(execution: TaskExecutionDto): theia.TaskExecution {
const executionId = execution.id;
let result: theia.TaskExecution | undefined = this.executions.get(executionId);
if (result) {
return result;
}

result = {
task: converter.toTask(execution.task),
terminate: () => {
this.proxy.$terminateTask(executionId);
}
};
result = this.toTaskExecution(execution);
this.executions.set(executionId, result);
return result;
}

private addCustomExecution(callback: ExecutionCallback): string {
let id = this.customExecutionIds.get(callback);
if (!id) {
id = this.nextCallbackId();
this.customExecutionIds.set(callback, id);
this.customExecutionFunctions.set(id, callback);
}
return id;
}

private nextCallbackId(): string {
return this.callbackIdBase + this.callbackId++;
}
Expand Down
22 changes: 8 additions & 14 deletions packages/plugin-ext/src/plugin/type-converters.ts
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,10 @@ export function fromRangeOrRangeWithMessage(ranges: theia.Range[] | theia.Decora
};
});
} else {
return ranges.map(r => ({ range: fromRange(r) }));
return ranges.map((r): DecorationOptions =>
({
range: fromRange(r)!
}));
}
}

Expand Down Expand Up @@ -821,10 +824,10 @@ export function toTask(taskDto: TaskDto): theia.Task {
result.execution = getShellExecution(taskDto);
}

if (taskType === 'customExecution' || types.CustomExecution.is(execution)) {
result.execution = getCustomExecution(taskDto);
if (taskType === 'customExecution') {
tsmaeder marked this conversation as resolved.
Show resolved Hide resolved
// if taskType is customExecution, we need to put all the information into taskDefinition,
// because some parameters may be in taskDefinition.
// we cannot assign a custom execution because these have assigned ids which we don't have in this stateless converter
taskDefinition.label = label;
taskDefinition.command = command;
taskDefinition.args = args;
Expand Down Expand Up @@ -892,14 +895,9 @@ export function fromShellExecution(execution: theia.ShellExecution, taskDto: Tas
}

export function fromCustomExecution(execution: theia.CustomExecution, taskDto: TaskDto): TaskDto {
// handling of the execution id must be done independently
taskDto.taskType = 'customExecution';
const callback = execution.callback;
if (callback) {
taskDto.callback = callback;
return taskDto;
} else {
throw new Error('Converting CustomExecution callback is not implemented');
}
return taskDto;
}

export function getProcessExecution(taskDto: TaskDto): theia.ProcessExecution {
Expand All @@ -921,10 +919,6 @@ export function getShellExecution(taskDto: TaskDto): theia.ShellExecution {
taskDto.options || {});
}

export function getCustomExecution(taskDto: TaskDto): theia.CustomExecution {
return new types.CustomExecution(taskDto.callback);
}

export function getShellArgs(args: undefined | (string | theia.ShellQuotedString)[]): string[] {
if (!args || args.length === 0) {
return [];
Expand Down
2 changes: 1 addition & 1 deletion packages/plugin-ext/src/plugin/types-impl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1845,7 +1845,7 @@ export class CustomExecution {
return this._callback;
}

public static is(value: theia.ShellExecution | theia.ProcessExecution | theia.CustomExecution): value is CustomExecution {
public static is(value: theia.ShellExecution | theia.ProcessExecution | theia.CustomExecution | undefined): value is CustomExecution {
const candidate = value as CustomExecution;
return candidate && (!!candidate._callback);
}
Expand Down
Loading