Repository navigation
Pipeline aborts all HTTP requests #32105
Description
Activity
I'll take a look asap
Reacted by Szymon Marczak, Clément Billoré, Victor Vlasenko, Maël Nison, Alex Yang, Peter Merkert and Claus KlingbergAre you able to create a self contained test case? I tried the following on
masterbut was unable to make it fail:const server = http.createServer((req, res) => { setTimeout(() => { req.write('ASDASD'); req.end(); }, 1e3); }); server.listen(0, () => { const req = http.request({ port: server.address().port }); const body = new PassThrough(); pipeline( body, req, common.mustCall((err) => { assert(!err); server.close(); }) ); body.end(); });
You've missed
request.once('abort', common.mustNotCall);
No, you misunderstood me. Let me explain...
In #31940 you introduced this change:
https://github.com/nodejs/node/pull/31940/files#diff-eefad5e8051f1634964a3847b691ff84R36
It calls
destroyer(stream, error), which callsrequest.abort()even though it should not.even though it should not
Why do you think it should not?
pipelineis meant to sent data from one stream to another. It shouldn't destroy streams that have been successfully ended. I mean it should callrequest.abort()only if an error occurs.pipeline is meant to sent data from one stream to another. It shouldn't destroy streams that have been successfully ended. I mean it should call request.abort() only if an error occurs.
I disagree with this. The current semantics of
pipelineis to destroy all streams on completion. This is because not all streams properly calldestroyon completion, and the idea of pipeline is to normalize this behavior.I might be wrong here, and it might also be a bad idea. Why do you think it shouldn't be destroyed?
And if we don't call destroy how do we properly handle streams that do no properly destroy themselves? Changing this could break it in the other direction where we end up with leaking resources.
A quick fix to resolve the break is not have a special case for
requestand simply don't destroy it like the rest. But I don't think that is a longterm solution. Do we have a more fundamental problem?@szmarczak What is the fundamental breaking change here? That the request is aborted/destroyed or that
'abort'is emitted?That the request is aborted.
That the request is aborted.
If it has completed. Why is this a problem? Aborting a completed request is basically a noop expect for the
'abort'event?Is the problem here that only the writable side has completed when the pipeline callback is invoked and the readable side is still to be consumed?
I disagree with this.
Well, then you should've made an exception for HTTP requests as this change makes #30869 and #31054 moot which is a major breaking change.
If it has completed.
No, it hasn't. It has only sent the request, but it hasn't received the response yet.
Is the problem here that only the writable side has completed when the pipeline callback is invoked and the readable side is still to be consumed?
Exactly! :)
12 remaining items
- added a commit that references this issue
on Mar 9, 2020 - added a commit that references this issue
on Mar 11, 2020 Is there any chance to get this before 13.11.0 (#32185), in a real patch version ? I'm surprised that a major bug patch like this takes days to be published while the node ecosystem is really affected by this problem.
@darksabrefr v13.x is not an LTS release and we explicitly have the uneven releases to detect issues like these early so that they do not get backported to the LTS release lines or only with fixes.
It is not as easy as just pushing a button to publish a new release. It requires a significant amount of time to do so. The project itself is all open source and people must find the time to do so. If you would like to contribute and help out, that would be great!
That aside: we have certain rules when commits may land and when not. It was almost immediately fixed after reporting it but merging requires to review the code and to wait 48 hours so that multiple collaborators get the chance to have a look at the change. Publishing a minor or a patch version will also take the same amount of time.
We have lots of tests in place and try hard to guarantee stability for all changes. This is just very difficult with a complex state machine as streams. I am very grateful for the fast response from @ronag @MylesBorins and everyone else involved to make sure that a fixed version is released as soon as possible.
Reacted by Szymon Marczak and Pedro Augusto de Paula Barbosa@darksabrefr some extra context that might be helpful. There was another regression introduced in 13.9.0 that broke building from source tarballs. We did not get a fix in time for 13.10.0 as the releasers were not aware of the issue before pushing for the release. This in turn escalated an emegency 13.10.1 which went out last Wednesday. This issue was opened the following day, and we didn't yet have a clear solution. The earliest we could get a solution in would have been Friday, and we do our best to not do releases on Fridays, as to avoid weekend fire drills.
I started working on a new 11.x release on Monday, having something basic prepared and running in CI that evening. For the current release line, as mentioned above by @BridgeAR, we include any commits that are not Semver-Minor that land cleanly on the release line. We of course work really hard to ensure that if we break thing we fix them, as you can see based on the above release going out 24 hours after the original 13.10.0, but we do not offer the same guaranteed support that we do for LTS. It is for this reason that I generally advise folks to only use LTS is production.
While working on a fix for the streams bug a number of missing patches were identified that made it harder to ensure the same behavior on master and 13.x, specifically that the fix actually fixed things and could land. There we also more regressions that were identified.
Multiple people have been putting in time since last Thursday to get this out, so I hope you can understand the situation is a bit complicated
Reacted by Clément BilloréReacted by Claus KlingbergAlso worth mentioning that we might be having a further delay due to an infrastructure issue in our build cluster
edit: this is now resolved
Reacted by Clément Billoré and WillI want to thank @MylesBorins for the clarifications and @ronag and @szmarczak for the fast interventions.
I just mitigate the advice to use only LTS in production. Like other people, I contribute to node, in an invisible way, by using and testing non-LTS versions days after days, on various (but not critical) real-life projects in various situations, and describing issues, here or in other related repos. @BridgeAR: code and commits are not the only maneer to contribute to a project ;-)
My only reproach is the lack of communication versus this breaking bug patch release (and others related bugs that @MylesBorins mentioned too). There is no one to blame concerning the bug itself on this non-LTS channel. But a confirmed breaking bug related to an identified commit should not be reverted and immediately deployed (or asap)? I understand that's not always as simple as reverting a single commit, but that's a base idea to not let the latest stable version broken for days. A too unstable latest can lead to not have sufficient real-life tests on it, people using more LTS versions, causing backported things to LTS under-tested and potentially bugged too.
- added a commit that references this issue
on Jul 27, 2026
This bug breaks hundreds of packages, including Got, Yarn and Renovate.
What steps will reproduce the bug?
How often does it reproduce? Is there a required condition?
Always.
What is the expected behavior?
What do you see instead?
Pipeline errored: false +The request has been abortedAdditional information
This is the culprit: https://github.com/nodejs/node/pull/31940/files#diff-eefad5e8051f1634964a3847b691ff84R36
->
https://github.com/nxtedition/node/blob/5a55b1df97a1f93c5fb81bf758050d81524ae834/lib/internal/streams/destroy.js#L166
/cc @ronag