Skip to content

Commit

Permalink
http: emit 'error' on aborted client response
Browse files Browse the repository at this point in the history
Client responses aka. IncomingMessage emits 'aborted'
instead of 'error' which causes confusion when
the object is used as a regular stream, i.e. if
functions working on streams are passed a
client response object they might not work properly
unless they take this into account.

Refs: nodejs/web-server-frameworks#41
Fixes: #28172
  • Loading branch information
ronag committed May 1, 2020
1 parent 9515204 commit 4bcd762
Show file tree
Hide file tree
Showing 7 changed files with 67 additions and 18 deletions.
11 changes: 8 additions & 3 deletions doc/api/http.md
Original file line number Diff line number Diff line change
Expand Up @@ -333,9 +333,8 @@ Until the data is consumed, the `'end'` event will not fire. Also, until
the data is read it will consume memory that can eventually lead to a
'process out of memory' error.

Unlike the `request` object, if the response closes prematurely, the
`response` object does not emit an `'error'` event but instead emits the
`'aborted'` event.
For backward compatibility, `res` will only emit `'error'` if there is an
`'error'` listener registered.

Node.js does not check whether Content-Length and the length of the
body which has been transmitted are equal or not.
Expand Down Expand Up @@ -2404,6 +2403,8 @@ the following events will be emitted in the following order:
* `'data'` any number of times, on the `res` object
* (connection closed here)
* `'aborted'` on the `res` object
* `'error'` on the `res` object with an error with message
`'Error: socket hang up'` and code `'ECONNRESET'`.
* `'close'`
* `'close'` on the `res` object

Expand Down Expand Up @@ -2432,6 +2433,8 @@ events will be emitted in the following order:
* `'data'` any number of times, on the `res` object
* (`req.destroy()` called here)
* `'aborted'` on the `res` object
* `'error'` on the `res` object with an error with message
`'Error: socket hang up'` and code `'ECONNRESET'`.
* `'close'`
* `'close'` on the `res` object

Expand Down Expand Up @@ -2461,6 +2464,8 @@ events will be emitted in the following order:
* (`req.abort()` called here)
* `'abort'`
* `'aborted'` on the `res` object
* `'error'` on the `res` object with an error with message
`'Error: socket hang up'` and code `'ECONNRESET'`.
* `'close'`
* `'close'` on the `res` object

Expand Down
4 changes: 4 additions & 0 deletions lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -413,9 +413,13 @@ function socketCloseListener() {
const res = req.res;
if (res) {
// Socket closed before we emitted 'end' below.
// TOOD(ronag): res.destroy(err)
if (!res.complete) {
res.aborted = true;
res.emit('aborted');
if (res.listenerCount('error') > 0) {
res.emit('error', connResetException('aborted'));
}
}
req.emit('close');
if (!res.aborted && res.readable) {
Expand Down
4 changes: 4 additions & 0 deletions test/parallel/test-http-abort-client.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

'use strict';
const common = require('../common');
const assert = require('assert');
const http = require('http');

let serverRes;
Expand All @@ -41,6 +42,9 @@ server.listen(0, common.mustCall(() => {
res.resume();
res.on('end', common.mustNotCall());
res.on('aborted', common.mustCall());
res.on('error', common.mustCall((err) => {
assert.strictEqual(err.code, 'ECONNRESET');
}));
res.on('close', common.mustCall());
res.socket.on('close', common.mustCall());
}));
Expand Down
4 changes: 4 additions & 0 deletions test/parallel/test-http-aborted.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ const assert = require('assert');
res.on('aborted', common.mustCall(() => {
assert.strictEqual(res.aborted, true);
}));
res.on('error', common.mustCall((err) => {
assert.strictEqual(err.code, 'ECONNRESET');
}));
req.abort();
}));
}));
Expand All @@ -36,6 +39,7 @@ const assert = require('assert');
const server = http.createServer(common.mustCall(function(req, res) {
req.on('aborted', common.mustCall(function() {
assert.strictEqual(this.aborted, true);
server.close();
}));
assert.strictEqual(req.aborted, false);
res.write('hello');
Expand Down
54 changes: 40 additions & 14 deletions test/parallel/test-http-client-aborted-event.js
Original file line number Diff line number Diff line change
@@ -1,20 +1,46 @@
'use strict';
const common = require('../common');
const http = require('http');
const assert = require('assert');

let serverRes;
const server = http.Server(function(req, res) {
res.write('Part of my res.');
serverRes = res;
});
{
let serverRes;
const server = http.Server(function(req, res) {
res.write('Part of my res.');
serverRes = res;
});

server.listen(0, common.mustCall(function() {
http.get({
port: this.address().port,
headers: { connection: 'keep-alive' }
}, common.mustCall(function(res) {
server.close();
serverRes.destroy();
res.on('aborted', common.mustCall());
server.listen(0, common.mustCall(function() {
http.get({
port: this.address().port,
headers: { connection: 'keep-alive' }
}, common.mustCall(function(res) {
server.close();
serverRes.destroy();
res.on('aborted', common.mustCall());
res.on('error', common.mustCall((err) => {
assert.strictEqual(err.code, 'ECONNRESET');
}));
}));
}));
}));
}

{
// Don't crash of no 'error' handler.
let serverRes;
const server = http.Server(function(req, res) {
res.write('Part of my res.');
serverRes = res;
});

server.listen(0, common.mustCall(function() {
http.get({
port: this.address().port,
headers: { connection: 'keep-alive' }
}, common.mustCall(function(res) {
server.close();
serverRes.destroy();
res.on('aborted', common.mustCall());
}));
}));
}
5 changes: 4 additions & 1 deletion test/parallel/test-http-client-spurious-aborted.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,15 +60,18 @@ function download() {
res.on('end', common.mustCall(() => {
reqCountdown.dec();
}));
res.on('error', common.mustNotCall());
} else {
res.on('aborted', common.mustCall(() => {
aborted = true;
reqCountdown.dec();
writable.end();
}));
res.on('error', common.mustCall((err) => {
assert.strictEqual(err.code, 'ECONNRESET');
}));
}

res.on('error', common.mustNotCall());
writable.on('finish', () => {
assert.strictEqual(aborted, abortRequest);
finishCountdown.dec();
Expand Down
3 changes: 3 additions & 0 deletions test/parallel/test-http-outgoing-message-capture-rejection.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ events.captureRejections = true;

req.on('response', common.mustCall((res) => {
res.on('aborted', common.mustCall());
res.on('error', common.mustCall((err) => {
assert.strictEqual(err.code, 'ECONNRESET');
}));
res.resume();
server.close();
}));
Expand Down

0 comments on commit 4bcd762

Please sign in to comment.