Pipeline aborts all HTTP requests

@ronag

Version: >= 13.10.0
Platform: all
Subsystem: stream

This bug breaks hundreds of packages, including Got, Yarn and Renovate.

What steps will reproduce the bug?

const {PassThrough, pipeline} = require('stream');
const https = require('https');

const body = new PassThrough();
const request = https.request('https://example.com');

request.once('abort', () => {
	console.log('The request has been aborted');
});

pipeline(
	body,
	request,
	error => {
		console.log(`Pipeline errored: ${!!error}`);
	}
);

body.end();

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior?

Pipeline errored: false

What do you see instead?

Pipeline errored: false
+The request has been aborted

Additional 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

I'll take a look asap

Are you able to create a self contained test case? I tried the following on master but 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);

@mcollina

Ah, I think this is actually a problem with the http_client, I guess it shouldn't emit 'abort' if it has gracefully ended.

@mcollina

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 calls

request.abort() even though it should not.

even though it should not

Why do you think it should not?

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.

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 pipeline is to destroy all streams on completion. This is because not all streams properly call destroy on 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 request and 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?

@mcollina

@mcollina: I think we need your guidance here.

@szmarczak

@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! :)

Pin travis test runner at Node 13.9.0 until nodejs/node#32105 is reso…

For those returning to this issue due to breakage in 13.10.x please see the backport PR here #32111

stream: don't destroy final readable stream in pipeline

[TO REVERT] Pin Node 13.9

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

@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.

@darksabrefr

@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

Also worth mentioning that we might be having a further delay due to an infrastructure issue in our build cluster

nodejs/build#2217

edit: this is now resolved

@MylesBorins

I 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.

stream: don't destroy final readable stream in pipeline

Navigation Menu

Repository navigation

Pipeline aborts all HTTP requests #32105

What steps will reproduce the bug?

How often does it reproduce? Is there a required condition?

What is the expected behavior?

What do you see instead?

Additional information

12 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Pipeline aborts all HTTP requests #32105

Description

What steps will reproduce the bug?

How often does it reproduce? Is there a required condition?

What is the expected behavior?

What do you see instead?

Additional information

Activity

ronag commented on Mar 5, 2020

ronag commented on Mar 5, 2020

szmarczak commented on Mar 5, 2020

ronag commented on Mar 5, 2020

szmarczak commented on Mar 5, 2020

szmarczak commented on Mar 5, 2020

ronag commented on Mar 5, 2020

szmarczak commented on Mar 5, 2020

ronag commented on Mar 5, 2020

ronag commented on Mar 5, 2020

ronag commented on Mar 5, 2020

ronag commented on Mar 5, 2020

szmarczak commented on Mar 5, 2020

ronag commented on Mar 5, 2020

szmarczak commented on Mar 5, 2020

12 remaining items

ronag commented on Mar 9, 2020

darksabrefr commented on Mar 11, 2020

BridgeAR commented on Mar 11, 2020

MylesBorins commented on Mar 11, 2020

MylesBorins commented on Mar 11, 2020

darksabrefr commented on Mar 11, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions