Fix #2961: Output of short-lived tasks is not shown by EstherPerelman · Pull Request #9409 · eclipse-theia/theia

EstherPerelman · 2021-04-28T09:08:57Z

Signed-off-by: Esther Perelman Esther.Perelman@sap.com

Most of PR description copied from #7776

What it does

Fixes Output of short-lived tasks is not shown #2961: Prevents process registration from being canceled until it is attached

How to test

Run a task that lives only for a short time, like echo hello, and check the task terminal widget for its title and output;
Please also check if the change introduced any new bugs into Theia.

Demo:

Before
After

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

EstherPerelman · 2021-04-28T09:26:51Z

packages/process/src/node/terminal-process.ts

                // be ignored.
                if (signal === undefined || signal === 0) {
                    this.emitOnExit(code, undefined);
+                    this.onProcessExited();


More info on exit event ant its params you can find here

EstherPerelman · 2021-04-28T10:10:24Z

packages/task/src/browser/task-service.ts

-        // 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);


In short lived tasks- the task finishes before attached, But still needed to be attached...

EstherPerelman · 2021-04-28T12:41:04Z

@simark @marechal-p @vince-fugnitto @akosyakov or any reviewer?

colin-grant-work

This seems to be working for me with both tasks that conclude successfully (echo hello) and those that error out (ls /non-existent-folder), and I believe it addresses the criticism of #7776 that it relied on uncertain timings. Is there any reason to worry that someone might start terminal processes that never get attached to? This seems bind the behavior of the TerminalProcess fairly tightly to a set of expectations that may only apply to the task system.

packages/process/src/node/process.ts

packages/process/src/node/terminal-process.ts

colin-grant-work · 2021-04-28T17:11:35Z

I believe this PR would also address #9350 - at least #9350 should be treated along with #2961, since they're likely the same underlying issue.

(Sorry for the accidental closure - mouse slipped to wrong button when I was posting this comment)

packages/task/src/browser/task-service.ts

colin-grant-work · 2021-05-05T15:18:41Z

@EstherPerelman, sorry for the delay. I will take another look at this later today or tomorrow.

packages/terminal/src/node/base-terminal-server.ts

colin-grant-work

@marechal-p, I know this something you've given some thought to. Do you have any opinions about the approach adopted here? I think it works, ensures cleanup, and is relatively non-breaking, so I'm inclined to accept it, for now, but maybe there's a better option available without a major refactor?

packages/terminal/src/node/base-terminal-server.ts

colin-grant-work · 2021-05-06T22:57:20Z

packages/terminal/src/node/terminal-server.ts

                resolve(-1);
            });
+
+            const taskTerm = this.taskTerminalFactory(options);


It looks like we're creating both a taskTerminal and a normal terminal in this Promise. Is that intended, or should we create or the other, depending on some conditional?

I added this after the builds here failed but I think it can be omitted. (removed)

EstherPerelman · 2021-05-11T13:22:04Z

@marechal-p, I know this something you've given some thought to. Do you have any opinions about the approach adopted here? I think it works, ensures cleanup, and is relatively non-breaking, so I'm inclined to accept it, for now, but maybe there's a better option available without a major refactor?

@marechal-p @colin-grant-work WDYS? Can it be merged?

colin-grant-work

I'll give my tentative approval, but I'm still hoping someone else will be willing to give a look.

packages/process/src/node/process-backend-module.ts

packages/terminal/src/node/base-terminal-server.ts

packages/terminal/src/node/terminal-server.ts

packages/process/src/node/task-terminal-process.ts

packages/task/src/browser/task-service.ts

Signed-off-by: Esther Perelman <Esther.Perelman@sap.com>

EstherPerelman · 2021-05-13T06:10:55Z

@marechal-p Thank you for reviewing! I updated the code accordingly.

paul-marechal · 2021-05-20T02:19:30Z

@EstherPerelman I am fine with the changes, thanks!

Note that I am still working on refactoring processes and terminals, and I'll have a very different implementation than what you did here, although functionality will be the same. I still need more time to finish that refactoring, and I'll be unavailable the whole week that starts May 24th. This means that I'll most likely try to merge my refactoring after the next release.

All of that to ask if you are ok with merging your change now, knowing it might be obsolete sometime later after the next release? If so let's merge this PR :)

EstherPerelman · 2021-05-20T05:36:39Z

All of that to ask if you are ok with merging your change now, knowing it might be obsolete sometime later after the next release? If so let's merge this PR :)

@marechal-p I'm ok with merging now, Thank you!

EstherPerelman commented Apr 28, 2021

View reviewed changes

colin-grant-work reviewed Apr 28, 2021

View reviewed changes

packages/process/src/node/process.ts Outdated Show resolved Hide resolved

packages/process/src/node/terminal-process.ts Outdated Show resolved Hide resolved

colin-grant-work closed this Apr 28, 2021

colin-grant-work reopened this Apr 28, 2021

EstherPerelman force-pushed the Issue-2961 branch 2 times, most recently from 1dc1667 to 41bba3a Compare April 29, 2021 15:36

EstherPerelman commented Apr 29, 2021

View reviewed changes

packages/task/src/browser/task-service.ts Show resolved Hide resolved

EstherPerelman force-pushed the Issue-2961 branch 4 times, most recently from b088789 to 912fa52 Compare May 6, 2021 11:17

colin-grant-work reviewed May 6, 2021

View reviewed changes

packages/terminal/src/node/base-terminal-server.ts Outdated Show resolved Hide resolved

EstherPerelman force-pushed the Issue-2961 branch from 912fa52 to a228cb1 Compare May 6, 2021 20:45

colin-grant-work reviewed May 6, 2021

View reviewed changes

EstherPerelman force-pushed the Issue-2961 branch 2 times, most recently from 11cdc55 to 1923945 Compare May 9, 2021 05:33

colin-grant-work approved these changes May 12, 2021

View reviewed changes

paul-marechal reviewed May 13, 2021

View reviewed changes

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

db77887

Signed-off-by: Esther Perelman <Esther.Perelman@sap.com>

EstherPerelman force-pushed the Issue-2961 branch from 1923945 to db77887 Compare May 13, 2021 06:08

paul-marechal approved these changes May 20, 2021

View reviewed changes

paul-marechal merged commit d563b6b into eclipse-theia:master May 20, 2021

vince-fugnitto added this to the 1.14.0 milestone May 27, 2021

vince-fugnitto mentioned this pull request Jun 8, 2021

fix #2961: a workaround to show output and title of short-lived tasks. #7776

Closed

1 task

vince-fugnitto mentioned this pull request Mar 30, 2022

Error trying to run task #4038

Closed

Conversation

EstherPerelman commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What it does

How to test

Demo:

Review checklist

Reminder for reviewers

Uh oh!

EstherPerelman Apr 28, 2021

Choose a reason for hiding this comment

Uh oh!

EstherPerelman Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EstherPerelman commented Apr 28, 2021

Uh oh!

colin-grant-work left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

colin-grant-work commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

colin-grant-work commented May 5, 2021

Uh oh!

Uh oh!

colin-grant-work left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

colin-grant-work May 6, 2021

Choose a reason for hiding this comment

Uh oh!

EstherPerelman May 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EstherPerelman commented May 11, 2021

Uh oh!

colin-grant-work left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EstherPerelman commented May 13, 2021

Uh oh!

paul-marechal commented May 20, 2021

Uh oh!

EstherPerelman commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

EstherPerelman commented Apr 28, 2021 •

edited

Loading

EstherPerelman Apr 28, 2021 •

edited

Loading

colin-grant-work left a comment •

edited

Loading

colin-grant-work commented Apr 28, 2021 •

edited

Loading

EstherPerelman May 9, 2021 •

edited

Loading

EstherPerelman commented May 20, 2021 •

edited

Loading