stream.pipeline(httpResponse, decompressStream) emits error three times

[szmarczak]

• Version: v13.5.0
• Platform: Linux solus 5.4.1-137.current #1 SMP PREEMPT Sat Dec 7 15:17:22 UTC 2019 x86_64 GNU/Linux
• Subsystem: stream, http

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

http.get('http://example.com', response => {
	const stream = new PassThrough();

	stream.on('error', error => {
		console.log('stream error', error);
	});

	pipeline(
		response,
		stream,
		error => {
			console.log('pipeline error', error);
		}
	);

	stream.destroy(new Error('oh no'));
}).on('error', error => {
	console.log('request error', error);
});

If you run the example above, the error will be emitted three times:

• stream error (oh no)
• pipeline error (oh no)
• request error (oh no)

I did not expect the ClientRequest instance to throw.
It's because response.destroy(error) calls socket.destroy(error) directly.
ClientRequest catches the error and throws it again.

But why does it call response.destroy(error)?
Well, it happens because of 6480882 (previously it just called response.destroy()).

If you run the example in Node.js 12, you will get these results:

• stream error (Cannot call write after a stream was destroyed)
• pipeline error (Cannot call write after a stream was destroyed)
• stream error (oh no)

I just want to say that the new behavior is correct, but ClientRequest should not throw if using pipeline with response.

[lpinca]

See 6480882.

[lpinca]

cc: @mcollina

[mcollina]

Let's revert 6480882 and try again - I'm swamped, it would be great if somebody else took care of it.

[szmarczak]

There's no need to revert that. I would just replace:

node/lib/internal/streams/pipeline.js

Lines 46 to 54 in 6480882

	if (typeof stream.destroy === 'function') {
	if (stream.req && stream._writableState === undefined) {
	// This is a ClientRequest
	// TODO(mcollina): backward compatible fix to avoid crashing.
	// Possibly remove in a later semver-major change.
	stream.req.on('error', noop);
	}
	return stream.destroy(err);
	}

with

    if (isRequest(stream.req)) return stream.req.abort();
    if (typeof stream.destroy === 'function') return stream.destroy(err);

The same is already done if you provide ClientRequest directly
(perhaps you just missed that, no worries):

node/lib/internal/streams/pipeline.js

Line 45 in 6480882

if (isRequest(stream)) return stream.abort();

[BridgeAR]

Ping @ronag

[ronag]

Yes, res.destroy(err) propagating the error to req might be a bit weird, though I'm actually not sure what is "correct" here. I'm working on trying to clarify these cases.

For now I think @szmarczak suggestion makes sense. @szmarczak would you like to create a PR? I'll investigate later whether that would have any impact on what 6480882 was trying to fix.

[szmarczak]

Sure, will do today.