From 8a6fab02adab2de05f6e864847f96b0924be0840 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Thu, 30 Apr 2020 20:29:35 +0200 Subject: [PATCH] http: emit 'error' on aborted server request 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: https://github.com/nodejs/web-server-frameworks/issues/41 PR-URL: https://github.com/nodejs/node/pull/33172 Fixes: https://github.com/nodejs/node/issues/28172 Refs: https://github.com/nodejs/node/pull/28677 Reviewed-By: Matteo Collina Reviewed-By: Rich Trott Reviewed-By: Anna Henningsen --- doc/api/http.md | 11 ++- lib/_http_client.js | 4 ++ lib/_http_server.js | 10 ++- test/parallel/test-http-abort-client.js | 3 + test/parallel/test-http-aborted.js | 67 ++++++++++++++----- .../test-http-client-aborted-event.js | 53 +++++++++++---- .../test-http-client-spurious-aborted.js | 5 +- ...http-outgoing-message-capture-rejection.js | 3 + 8 files changed, 121 insertions(+), 35 deletions(-) diff --git a/doc/api/http.md b/doc/api/http.md index a6f73ee94f9cfd..e924a2dc312812 100644 --- a/doc/api/http.md +++ b/doc/api/http.md @@ -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. @@ -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 @@ -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 @@ -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 diff --git a/lib/_http_client.js b/lib/_http_client.js index 7b00417e8a253a..8f46ef367b0e8a 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -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) { diff --git a/lib/_http_server.js b/lib/_http_server.js index 9dfeef746636c5..2c7c5284f604a0 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -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 { @@ -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 ? diff --git a/test/parallel/test-http-abort-client.js b/test/parallel/test-http-abort-client.js index 608d4dc7607853..8a4666df1c28c3 100644 --- a/test/parallel/test-http-abort-client.js +++ b/test/parallel/test-http-abort-client.js @@ -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()); })); diff --git a/test/parallel/test-http-aborted.js b/test/parallel/test-http-aborted.js index c3d7e4641f4501..e22e7ca4cc330f 100644 --- a/test/parallel/test-http-aborted.js +++ b/test/parallel/test-http-aborted.js @@ -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(); })); -})); +} diff --git a/test/parallel/test-http-client-aborted-event.js b/test/parallel/test-http-client-aborted-event.js index 1e7feb7d5860f6..b1401187705acb 100644 --- a/test/parallel/test-http-client-aborted-event.js +++ b/test/parallel/test-http-client-aborted-event.js @@ -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()); + })); + })); +} diff --git a/test/parallel/test-http-client-spurious-aborted.js b/test/parallel/test-http-client-spurious-aborted.js index 0cb2f471c2b792..850462dadc76f8 100644 --- a/test/parallel/test-http-client-spurious-aborted.js +++ b/test/parallel/test-http-client-spurious-aborted.js @@ -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(); diff --git a/test/parallel/test-http-outgoing-message-capture-rejection.js b/test/parallel/test-http-outgoing-message-capture-rejection.js index 5f667ea17ea156..9fe9bdb21331d8 100644 --- a/test/parallel/test-http-outgoing-message-capture-rejection.js +++ b/test/parallel/test-http-outgoing-message-capture-rejection.js @@ -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(); }));