stream: prevent pipeline() callback after sync throw - #65165
Conversation
|
Review requested:
|
When a stage in the middle of pipeline() throws synchronously (e.g. an invalid return value), the stages already wired have already incremented finishCount. When those stages later complete, their finish callbacks invoke the completion callback with no error — double-reporting the failure that the caller already received as an exception. Track synchronous throws while wiring, destroy the stages wired so far, and skip the completion callback in that case. Fixes: nodejs#65127 Signed-off-by: pacocartones <pacocartones@users.noreply.github.com>
88532ab to
4c49ce4
Compare
MILLERMARRU
left a comment
There was a problem hiding this comment.
Traced this against finishImpl and I think the mechanism is correct. The important part isn't just the syncThrow flag suppressing the callback, it's that the catch block does the destroy/dispose/abort work immediately at throw time instead of waiting for whatever stray completion event would otherwise trigger finishImpl's existing cleanup. That matters for a stage that's still actively flowing when a later stage throws, ac.abort() here is what actually tells an in-progress async generator/function stage to stop, rather than just leaving it to finish on its own.
Compared this against #65128, which fixes the same issue with a much smaller diff (a wired flag gating just the callback), and confirmed with a quick repro that the smaller version doesn't clean up an already-wired stage that hasn't naturally completed yet, since it never calls ac.abort() or destroys proactively, it just relies on the natural finish() path that #65127's own repro happens to trigger quickly. Left the details on that PR since it's a real gap this one doesn't have.
Re-read the wiring loop indented into the try block against the original to make sure nothing changed in the reformatting, looks like a straight re-indent, no logic difference.
Worth reconciling with #65128 before merge since they're both open against the same issue.
Fixes: #65127
Summary
When
pipeline()throws synchronously while wiring streams together — forexample, when a middle stage returns an invalid value (
ERR_INVALID_RETURN_VALUE)— the stages already wired have already incremented
finishCount. When thosestages complete afterwards, their finish callbacks invoke the completion
callback with no error, double-reporting the failure that the caller
already received as a synchronous exception.
This change tracks synchronous throws while wiring, destroys the stages wired
so far, and skips the completion callback in that case.
Repro (issue case A)
Before:
threw: ERR_INVALID_RETURN_VALUEandcallback: NO ERROR(double report).After:
threw: ERR_INVALID_RETURN_VALUEonly; the callback is never invoked.Verification
test/parallel/test-stream-pipeline.js:common.mustNotCall()gets invoked with success),abort paths verified unchanged against the pristine implementation.
Checklist