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

Re-attach stdout and stderr from new processes after retries #8087

Merged

Conversation

rubennorte
Copy link
Contributor

Summary

A worker pool exposes a unified stdout and stderr resulting from the combination of all the outputs from the workers it contains. The problem is that the workers only expose stdout/stderr from the first child they spawn and never re-attach the output of any subsequent child they might spawn after an initial one dies (for retries).

This changes both worker implementations (child process and node threads) to return a merged stream, resulting of the combination of all their possible children, so that output isn't lost.

Test plan

Added unit tests for both

@rubennorte rubennorte changed the title Reattach stdout and stderr from new processes after retries Re-attach stdout and stderr from new processes after retries Mar 8, 2019
Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

Nice tests

@@ -154,11 +174,11 @@ export default class ExperimentalWorker implements WorkerInterface {
return this._options.workerId;
}

getStdout() {
return this._worker.stdout;
getStdout(): NodeJS.ReadableStream | null {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the type defined in the interface, no? Does TS requires you to add this explicitly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, otherwise I'm getting this error:

[ts] Return type of public method from exported class has or is using name 'MergedStream' from external module "/Users/rubennorte/gitrepos/github/jest/node_modules/@types/merge-stream/index" but cannot be named.

@@ -45,10 +46,14 @@ export default class ChildProcessWorker implements WorkerInterface {
private _onProcessEnd!: OnEnd;
private _request: ChildMessage | null;
private _retries!: number;
private _stderr: ReturnType<typeof mergeStream> | null;
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

This is awesome!

@SimenB
Copy link
Member

SimenB commented Mar 8, 2019

The test from #8079 is failing, in case you didn't notice 🙂

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants