Skip to content

Commit

Permalink
http: emit 'error' on aborted server request
Browse files Browse the repository at this point in the history 
Server requests 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
server request object they might not work properly
unless they take this into account.

Refs: nodejs/web-server-frameworks#41

PR-URL: #33172
Fixes: #28172
Refs: #28677
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
  • Loading branch information
@ronag
ronag committed May 10, 2020
1 parent cbf2fa6 commit 8a6fab0
Show file tree
Hide file tree
Showing 8 changed files with 121 additions and 35 deletions.
11 changes: 8 additions & 3 deletions doc/api/http.md
View file
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 @@ -2417,6 +2416,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: aborted'` and code `'ECONNRESET'`.
* `'close'`
* `'close'` on the `res` object

Expand Down Expand Up @@ -2445,6 +2446,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: aborted'` and code `'ECONNRESET'`.
* `'close'`
* `'close'` on the `res` object

Expand Down Expand Up @@ -2474,6 +2477,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: aborted'` and code `'ECONNRESET'`.
* `'close'`
* `'close'` on the `res` object

Expand Down
4 changes: 4 additions & 0 deletions lib/_http_client.js
View file
Expand Up @@ -417,9 +417,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
10 changes: 9 additions & 1 deletion lib/_http_server.js
View file
Expand Up @@ -56,12 +56,16 @@ const {
getOrSetAsyncId
} = require('internal/async_hooks');
const { IncomingMessage } = require('_http_incoming');
const {
connResetException,
codes
} = require('internal/errors');
const {
ERR_HTTP_HEADERS_SENT,
ERR_HTTP_INVALID_STATUS_CODE,
ERR_INVALID_ARG_TYPE,
ERR_INVALID_CHAR
} = require('internal/errors').codes;
} = codes;
const { validateInteger } = require('internal/validators');
const Buffer = require('buffer').Buffer;
const {
Expand Down Expand Up @@ -536,9 +540,13 @@ function socketOnClose(socket, state) {
function abortIncoming(incoming) {
while (incoming.length) {
const req = incoming.shift();
// TODO(ronag): req.destroy(err)
req.aborted = true;
req.destroyed = true;
req.emit('aborted');
if (req.listenerCount('error') > 0) {
req.emit('error', connResetException('aborted'));
}
req.emit('close');
}
// Abort socket._httpMessage ?
Expand Down
3 changes: 3 additions & 0 deletions test/parallel/test-http-abort-client.js
View file
Expand Up @@ -41,6 +41,9 @@ server.listen(0, common.mustCall(() => {
res.resume();
res.on('end', common.mustNotCall());
res.on('aborted', common.mustCall());
res.on('error', common.expectsError({
code: 'ECONNRESET'
}));
res.on('close', common.mustCall());
res.socket.on('close', common.mustCall());
}));
Expand Down
67 changes: 51 additions & 16 deletions test/parallel/test-http-aborted.js
View file
Expand Up @@ -4,23 +4,58 @@ const common = require('../common');
const http = require('http');
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();
{
const server = http.createServer(common.mustCall(function(req, res) {
req.on('aborted', common.mustCall(function() {
assert.strictEqual(this.aborted, true);
}));
req.on('error', common.mustCall(function(err) {
assert.strictEqual(err.code, 'ECONNRESET');
assert.strictEqual(err.message, 'aborted');
server.close();
}));
assert.strictEqual(req.aborted, false);
res.write('hello');
}));

server.listen(0, common.mustCall(() => {
const req = http.get({
port: server.address().port,
headers: { connection: 'keep-alive' }
}, common.mustCall((res) => {
res.on('aborted', common.mustCall(() => {
assert.strictEqual(res.aborted, true);
}));
res.on('error', common.expectsError({
code: 'ECONNRESET',
message: 'aborted'
}));
req.abort();
}));
}));
}

{
// Don't crash if no 'error' handler on server request.

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');
}));
assert.strictEqual(req.aborted, false);
res.write('hello');
}));

server.listen(0, common.mustCall(() => {
const req = http.get({
port: server.address().port,
headers: { connection: 'keep-alive' }
}, common.mustCall((res) => {
res.on('aborted', common.mustCall(() => {
assert.strictEqual(res.aborted, true);
server.listen(0, common.mustCall(() => {
const req = http.get({
port: server.address().port,
headers: { connection: 'keep-alive' }
}, common.mustCall((res) => {
res.on('aborted', common.mustCall(() => {
assert.strictEqual(res.aborted, true);
}));
req.abort();
}));
req.abort();
}));
}));
}
53 changes: 39 additions & 14 deletions test/parallel/test-http-client-aborted-event.js
View file
Expand Up @@ -2,19 +2,44 @@
const common = require('../common');
const http = require('http');

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.expectsError({
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
View file
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.expectsError({
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
View file
Expand Up @@ -33,6 +33,9 @@ events.captureRejections = true;

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

0 comments on commit 8a6fab0

Please sign in to comment.