From 5f80df882057522c38c30b99f4bf25d82c68f662 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Thu, 30 May 2019 20:24:12 +0200 Subject: [PATCH] http: do not emit end after aborted IncomingMessage will no longer emit end after aborted. PR-URL: https://github.com/nodejs/node/pull/27984 Reviewed-By: Luigi Pinca Reviewed-By: Matteo Collina Reviewed-By: James M Snell --- doc/api/http.md | 20 ++++++++++++++++++- lib/_http_client.js | 2 +- test/parallel/test-http-abort-client.js | 2 +- .../test-http-client-spurious-aborted.js | 18 +++++++++++------ test/parallel/test-http-header-overflow.js | 9 ++++++++- .../parallel/test-http-response-no-headers.js | 1 + .../test-http-response-status-message.js | 3 ++- 7 files changed, 44 insertions(+), 11 deletions(-) diff --git a/doc/api/http.md b/doc/api/http.md index 2f1b37064b864e..1870de25941cb8 100644 --- a/doc/api/http.md +++ b/doc/api/http.md @@ -2252,6 +2252,25 @@ In the case of a connection error, the following events will be emitted: * `'error'` * `'close'` +In the case of a premature connection close before the response is received, +the following events will be emitted in the following order: + +* `'socket'` +* `'error'` with an error with message `'Error: socket hang up'` and code + `'ECONNRESET'` +* `'close'` + +In the case of a premature connection close after the response is received, +the following events will be emitted in the following order: + +* `'socket'` +* `'response'` + * `'data'` any number of times, on the `res` object +* (connection closed here) +* `'aborted'` on the `res` object +* `'close'` +* `'close'` on the `res` object + If `req.abort()` is called before the connection succeeds, the following events will be emitted in the following order: @@ -2272,7 +2291,6 @@ will be emitted in the following order: * `'abort'` * `'aborted'` on the `res` object * `'close'` -* `'end'` on the `res` object * `'close'` on the `res` object Setting the `timeout` option or using the `setTimeout()` function will diff --git a/lib/_http_client.js b/lib/_http_client.js index 957d2d64033b9e..d55bc850bcb9e5 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -364,7 +364,7 @@ function socketCloseListener() { res.emit('aborted'); } req.emit('close'); - if (res.readable) { + if (!res.aborted && res.readable) { res.on('end', function() { this.emit('close'); }); diff --git a/test/parallel/test-http-abort-client.js b/test/parallel/test-http-abort-client.js index 80492b9d22734d..608d4dc7607853 100644 --- a/test/parallel/test-http-abort-client.js +++ b/test/parallel/test-http-abort-client.js @@ -39,7 +39,7 @@ server.listen(0, common.mustCall(() => { serverRes.destroy(); res.resume(); - res.on('end', common.mustCall()); + res.on('end', common.mustNotCall()); res.on('aborted', common.mustCall()); res.on('close', common.mustCall()); res.socket.on('close', common.mustCall()); diff --git a/test/parallel/test-http-client-spurious-aborted.js b/test/parallel/test-http-client-spurious-aborted.js index 296b4bd256c929..0cb2f471c2b792 100644 --- a/test/parallel/test-http-client-spurious-aborted.js +++ b/test/parallel/test-http-client-spurious-aborted.js @@ -56,12 +56,18 @@ function download() { if (res.complete) res.readable = true; callback(); }; - res.on('end', common.mustCall(() => { - reqCountdown.dec(); - })); - res.on('aborted', () => { - aborted = true; - }); + if (!abortRequest) { + res.on('end', common.mustCall(() => { + reqCountdown.dec(); + })); + } else { + res.on('aborted', common.mustCall(() => { + aborted = true; + reqCountdown.dec(); + writable.end(); + })); + } + res.on('error', common.mustNotCall()); writable.on('finish', () => { assert.strictEqual(aborted, abortRequest); diff --git a/test/parallel/test-http-header-overflow.js b/test/parallel/test-http-header-overflow.js index 83a2d469103bae..1d39a7fd8b48a1 100644 --- a/test/parallel/test-http-header-overflow.js +++ b/test/parallel/test-http-header-overflow.js @@ -1,9 +1,13 @@ +// Flags: --expose-internals + 'use strict'; const { expectsError, mustCall } = require('../common'); const assert = require('assert'); const { createServer, maxHeaderSize } = require('http'); const { createConnection } = require('net'); +const { getOptionValue } = require('internal/options'); + const CRLF = '\r\n'; const DUMMY_HEADER_NAME = 'Cookie: '; const DUMMY_HEADER_VALUE = 'a'.repeat( @@ -17,11 +21,14 @@ const PAYLOAD = PAYLOAD_GET + CRLF + const server = createServer(); server.on('connection', mustCall((socket) => { + // Legacy parser gives sligthly different response. + // This discripancy is not fixed on purpose. + const legacy = getOptionValue('--http-parser') === 'legacy'; socket.on('error', expectsError({ type: Error, message: 'Parse Error: Header overflow', code: 'HPE_HEADER_OVERFLOW', - bytesParsed: maxHeaderSize + PAYLOAD_GET.length, + bytesParsed: maxHeaderSize + PAYLOAD_GET.length - (legacy ? -1 : 0), rawPacket: Buffer.from(PAYLOAD) })); })); diff --git a/test/parallel/test-http-response-no-headers.js b/test/parallel/test-http-response-no-headers.js index 87b0f8e67faf7c..22705936e08254 100644 --- a/test/parallel/test-http-response-no-headers.js +++ b/test/parallel/test-http-response-no-headers.js @@ -51,6 +51,7 @@ function test(httpVersion, callback) { body += data; }); + res.on('aborted', common.mustNotCall()); res.on('end', common.mustCall(function() { assert.strictEqual(body, expected[httpVersion]); server.close(); diff --git a/test/parallel/test-http-response-status-message.js b/test/parallel/test-http-response-status-message.js index 0f9bce8fa87d67..c4256d9e5a0c49 100644 --- a/test/parallel/test-http-response-status-message.js +++ b/test/parallel/test-http-response-status-message.js @@ -20,7 +20,7 @@ // USE OR OTHER DEALINGS IN THE SOFTWARE. 'use strict'; -require('../common'); +const common = require('../common'); const assert = require('assert'); const http = require('http'); const net = require('net'); @@ -71,6 +71,7 @@ function runTest(testCaseIndex) { console.log(`client: actual status message: ${response.statusMessage}`); assert.strictEqual(testCase.statusMessage, response.statusMessage); + response.on('aborted', common.mustNotCall()); response.on('end', function() { countdown.dec(); if (testCaseIndex + 1 < testCases.length) {