Re-attach stdout and stderr from new processes after retries #8087
Conversation
rubennorte
changed the title
| @@ -154,11 +174,11 @@ export default class ExperimentalWorker implements WorkerInterface { | |||
| return this._options.workerId; | |||
| } | |||
|
|
|||
| getStdout() { | |||
| return this._worker.stdout; | |||
| getStdout(): NodeJS.ReadableStream | null { | |||
There was a problem hiding this comment.
This is the type defined in the interface, no? Does TS requires you to add this explicitly?
There was a problem hiding this comment.
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; | |||
There was a problem hiding this comment.
ugh, but they don't export the interface: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/b32ee2597779e6376db8c49e68d75c68e50b9287/types/merge-stream/index.d.ts#L11-L14
|
The test from #8079 is failing, in case you didn't notice 🙂 |
rubennorte
force-pushed
the
reattach-streams-of-workers-after-retries
branch
from
d9121a8
to
fc4025b
Compare
|
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. |
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