process: add onClose event #6595

paul-marechal · 2019-11-20T22:37:32Z

What it does

WIP

Add a onClose convenience event on processes, fired when all the
streams are supposed to be closed.

Fixes #6587

How to test

yarn run test should pass
Run tasks and make sure it stops correctly
Open terminals, reload frontend and check if you can keep using it.

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

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

akosyakov · 2019-11-21T15:20:59Z

packages/process/src/node/multi-ring-buffer.ts

@@ -44,6 +44,14 @@ export class MultiRingBufferReadableStream extends stream.Readable implements Di
        this.deq(size);
    }

+    _destroy(err: Error | undefined, callback: (err?: Error) => void): void {


@marcdumais-work someone has to review and test it, i'm not familiar with it

akosyakov · 2019-11-21T15:21:50Z

@marechal-p Please fill in How to test to specify how to test for regressions, i.e. it seems that reconnecting terminals are changed it should be verified and so on.

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

packages/process/src/node/dev-null-stream.ts

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

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

paul-marechal · 2019-11-22T14:41:56Z

I will need to wait until I get access to a Windows machine to check why part of the test suite is failing there...

akosyakov · 2019-11-22T14:59:10Z

@marechal-p Could you ask colleagues to test terminal preservation between reloads that it works like on master?

marcdumais-work · 2019-11-25T15:25:36Z

Could you ask colleagues to test terminal preservation between reloads that it works like on master?

I am testing the reload case

Add a `onClose` convenience event on processes, fired when all the streams are supposed to be closed. Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>

AndrienkoAleksandr · 2019-11-25T17:02:15Z

I a bit tested killing tasks with different kill signal, tested exit terminal, restore task output after page refresh, didn't found regression.

marcdumais-work · 2019-11-25T17:30:42Z

I am testing the reload case

I also did not find regressions, quickly testing this PR.

marcdumais-work · 2019-11-25T17:39:20Z

It would probably be good to test this on Windows, since process execution is different.

paul-marechal · 2019-11-25T18:11:38Z

Tried on Windows; tests are green, couldn't find new issues at runtime.

akosyakov

I have not tested, but @marcdumais-work and @AndrienkoAleksandr did and code looks reasonable.

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

paul-marechal added the process issues related to the process extension label Nov 20, 2019

akosyakov requested review from elaihau, AlexTugarev, AndrienkoAleksandr, marcdumais-work and vince-fugnitto November 21, 2019 09:26

paul-marechal force-pushed the mp/process-close branch from 72b83c5 to 9ab299d Compare November 21, 2019 15:13

paul-marechal marked this pull request as ready for review November 21, 2019 15:13