From 6dd8500e688ce7e41fa8ca266587ff17422e28d4 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 21 Dec 2019 12:08:36 +0100 Subject: [PATCH 1/4] stream: pipeline should use req.abort() to destroy response destroy(err) on http response will propagate the error to the request causing 'error' to be unexpectedly emitted. Furthermore, response.destroy() unlike request.abort() does not _dump buffered data. Fixes a breaking change introduced in https://github.com/nodejs/node/commit/648088289d619bfb149fe90316ce0127083c4c99. Prefer res.req.abort() over res.destroy() until this situation is clarified. Fixes: https://github.com/nodejs/node/issues/31029 Refs: https://github.com/nodejs/node/commit/648088289d619bfb149fe90316ce0127083c4c99 --- lib/internal/streams/pipeline.js | 15 +++-------- test/parallel/test-stream-pipeline.js | 36 ++++++++++++++++++++++++++- 2 files changed, 38 insertions(+), 13 deletions(-) diff --git a/lib/internal/streams/pipeline.js b/lib/internal/streams/pipeline.js index ed5556e5d0a600..92a91c30171af1 100644 --- a/lib/internal/streams/pipeline.js +++ b/lib/internal/streams/pipeline.js @@ -17,7 +17,7 @@ const { } = require('internal/errors').codes; function isRequest(stream) { - return stream.setHeader && typeof stream.abort === 'function'; + return stream && stream.setHeader && typeof stream.abort === 'function'; } function destroyer(stream, reading, writing, callback) { @@ -43,22 +43,13 @@ function destroyer(stream, reading, writing, callback) { // request.destroy just do .end - .abort is what we want if (isRequest(stream)) return stream.abort(); - 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); - } + if (isRequest(stream.req)) return stream.req.abort(); + if (typeof stream.destroy === 'function') return stream.destroy(err); callback(err || new ERR_STREAM_DESTROYED('pipe')); }; } -function noop() {} - function pipe(from, to) { return from.pipe(to); } diff --git a/test/parallel/test-stream-pipeline.js b/test/parallel/test-stream-pipeline.js index 4a41f053bd0a85..76a6171bb1bd5a 100644 --- a/test/parallel/test-stream-pipeline.js +++ b/test/parallel/test-stream-pipeline.js @@ -1,7 +1,14 @@ 'use strict'; const common = require('../common'); -const { Stream, Writable, Readable, Transform, pipeline } = require('stream'); +const { + Stream, + Writable, + Readable, + Transform, + pipeline, + PassThrough +} = require('stream'); const assert = require('assert'); const http = require('http'); const { promisify } = require('util'); @@ -483,3 +490,30 @@ const { promisify } = require('util'); { code: 'ERR_INVALID_CALLBACK' } ); } + +{ + const server = http.Server(function(req, res) { + res.write('asd'); + }); + server.listen(0, function() { + http.request({ + port: this.address().port, + path: '/', + method: 'GET' + }, (res) => { + const stream = new PassThrough(); + + stream.on('error', common.mustCall()); + + pipeline( + res, + stream, + common.mustCall((err) => { + server.close(); + }) + ); + + stream.destroy(new Error('oh no')); + }).end().on('error', common.mustNotCall()); + }); +} From b7a0f753f3361071bee8ca5156ad0bb8d7ec27a0 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 21 Dec 2019 12:47:32 +0100 Subject: [PATCH 2/4] fixup: nit --- test/parallel/test-stream-pipeline.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/test/parallel/test-stream-pipeline.js b/test/parallel/test-stream-pipeline.js index 76a6171bb1bd5a..dd43f093751d02 100644 --- a/test/parallel/test-stream-pipeline.js +++ b/test/parallel/test-stream-pipeline.js @@ -496,11 +496,7 @@ const { promisify } = require('util'); res.write('asd'); }); server.listen(0, function() { - http.request({ - port: this.address().port, - path: '/', - method: 'GET' - }, (res) => { + http.get({ port: this.address().port }, (res) => { const stream = new PassThrough(); stream.on('error', common.mustCall()); @@ -514,6 +510,6 @@ const { promisify } = require('util'); ); stream.destroy(new Error('oh no')); - }).end().on('error', common.mustNotCall()); + }).on('error', common.mustNotCall()); }); } From 039b944483c7b0137412fa84e1d0fe7529b02e89 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Wed, 25 Dec 2019 11:32:11 +0100 Subject: [PATCH 3/4] fixup --- test/parallel/test-stream-pipeline.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/parallel/test-stream-pipeline.js b/test/parallel/test-stream-pipeline.js index dd43f093751d02..a7e86b39cd67f6 100644 --- a/test/parallel/test-stream-pipeline.js +++ b/test/parallel/test-stream-pipeline.js @@ -505,6 +505,9 @@ const { promisify } = require('util'); res, stream, common.mustCall((err) => { + assert.ok(err); + // TODO + // assert.strictEqual(err.message, 'oh no'); server.close(); }) ); From 14c816d4fcc8a91bb638131fd946a872b027248b Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Wed, 25 Dec 2019 12:56:59 +0100 Subject: [PATCH 4/4] Update test/parallel/test-stream-pipeline.js Co-Authored-By: Ruben Bridgewater --- test/parallel/test-stream-pipeline.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-stream-pipeline.js b/test/parallel/test-stream-pipeline.js index a7e86b39cd67f6..f6ee97ba43d053 100644 --- a/test/parallel/test-stream-pipeline.js +++ b/test/parallel/test-stream-pipeline.js @@ -506,7 +506,7 @@ const { promisify } = require('util'); stream, common.mustCall((err) => { assert.ok(err); - // TODO + // TODO(ronag): // assert.strictEqual(err.message, 'oh no'); server.close(); })