Skip to content

Commit

Permalink
stream: pipeline should use req.abort() to destroy response
Browse files Browse the repository at this point in the history
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 6480882.

Prefer res.req.abort() over res.destroy() until this situation is
clarified.

Fixes: #31029
Refs: 6480882
  • Loading branch information
ronag committed Dec 21, 2019
1 parent fc553fd commit 6dd8500
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 13 deletions.
15 changes: 3 additions & 12 deletions lib/internal/streams/pipeline.js
Expand Up @@ -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) {
Expand All @@ -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);
}
Expand Down
36 changes: 35 additions & 1 deletion 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');
Expand Down Expand Up @@ -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());
});
}

0 comments on commit 6dd8500

Please sign in to comment.