http: do not emit end after aborted

ronag · Trott · commit 5f80df882057 · 2019-10-13T19:04:44.000-07:00
IncomingMessage will no longer emit end after aborted. PR-URL: #27984 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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) {

Original file line number	Diff line number	Diff line change
`@@ -364,7 +364,7 @@ function socketCloseListener() {`
`364`	`364`	`res.emit('aborted');`
`365`	`365`	`}`
`366`	`366`	`req.emit('close');`
`367`		`- if (res.readable) {`
	`367`	`+ if (!res.aborted && res.readable) {`
`368`	`368`	`res.on('end', function() {`
`369`	`369`	`this.emit('close');`
`370`	`370`	`});`