From 06a8208c477633050e9bad368a5533065c34d402 Mon Sep 17 00:00:00 2001
From: Robert Nagy <ronagy@icloud.com>
Date: Sun, 14 Jul 2019 21:16:52 +0200
Subject: [PATCH 1/3] http: clientRequest.abort is destroy

---
 doc/api/deprecations.md                       | 17 +++++++++++++++
 doc/api/http.md                               |  4 ++++
 lib/_http_client.js                           | 21 +++++++++++++------
 test/parallel/test-http-client-close-event.js |  4 +++-
 test/parallel/test-http-client-set-timeout.js |  4 +++-
 5 files changed, 42 insertions(+), 8 deletions(-)

diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md
index 0d97cd38df0673..db44cb3ee08214 100644
--- a/doc/api/deprecations.md
+++ b/doc/api/deprecations.md
@@ -2500,6 +2500,21 @@ Type: Runtime
 Passing a callback to [`worker.terminate()`][] is deprecated. Use the returned
 `Promise` instead, or a listener to the worker’s `'exit'` event.
 
+<a id="DEP0XXX"></a>
+### DEP0XXX: ClientRequest.abort() is destroy
+<!-- YAML
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/28683
+    description: Documentation-only deprecation.
+-->
+
+Type: Documentation-only
+
+`[ClientRequest.destroy()`][] should be the same as
+`[ClientRequest.abort()`][]. Make ClientRequest more streamlike by deprecating
+ abort().
+
 [`--http-parser=legacy`]: cli.html#cli_http_parser_library
 [`--pending-deprecation`]: cli.html#cli_pending_deprecation
 [`--throw-deprecation`]: cli.html#cli_throw_deprecation
@@ -2508,6 +2523,8 @@ Passing a callback to [`worker.terminate()`][] is deprecated. Use the returned
 [`Buffer.from(buffer)`]: buffer.html#buffer_class_method_buffer_from_buffer
 [`Buffer.isBuffer()`]: buffer.html#buffer_class_method_buffer_isbuffer_obj
 [`Cipher`]: crypto.html#crypto_class_cipher
+[`ClientRequest.abort()`]: #http.html#http_request_abort
+[`ClientRequest.destroy()`]: #stream.html#stream_readable_destroy_error
 [`Decipher`]: crypto.html#crypto_class_decipher
 [`EventEmitter.listenerCount(emitter, eventName)`]: events.html#events_eventemitter_listenercount_emitter_eventname
 [`REPLServer.clearBufferedCommand()`]: repl.html#repl_replserver_clearbufferedcommand
diff --git a/doc/api/http.md b/doc/api/http.md
index 4923b6a61d840f..89d41201cd068f 100644
--- a/doc/api/http.md
+++ b/doc/api/http.md
@@ -546,8 +546,11 @@ srv.listen(1337, '127.0.0.1', () => {
 ### request.abort()
 <!-- YAML
 added: v0.3.8
+deprecated: REPLACEME
 -->
 
+> Stability: 0 - Deprecated: Use [`request.destroy()`][] instead.
+
 Marks the request as aborting. Calling this will cause remaining data
 in the response to be dropped and the socket to be destroyed.
 
@@ -2215,6 +2218,7 @@ not abort the request or do anything besides add a `'timeout'` event.
 [`net.createConnection()`]: net.html#net_net_createconnection_options_connectlistener
 [`new URL()`]: url.html#url_constructor_new_url_input_base
 [`removeHeader(name)`]: #http_request_removeheader_name
+[`request.destroy()`]: #stream.html#stream_readable_destroy_error
 [`request.end()`]: #http_request_end_data_encoding_callback
 [`request.flushHeaders()`]: #http_request_flushheaders
 [`request.getHeader()`]: #http_request_getheader_name
diff --git a/lib/_http_client.js b/lib/_http_client.js
index 14d9e59f63634f..7a80ce88dff394 100644
--- a/lib/_http_client.js
+++ b/lib/_http_client.js
@@ -311,11 +311,13 @@ ClientRequest.prototype._implicitHeader = function _implicitHeader() {
                     this[outHeadersKey]);
 };
 
-ClientRequest.prototype.abort = function abort() {
-  if (!this.aborted) {
-    process.nextTick(emitAbortNT.bind(this));
+ClientRequest.prototype.destroy = function destroy(error) {
+  if (this.aborted) {
+    return;
   }
+
   this.aborted = true;
+  process.nextTick(emitAbortNT.bind(this));
 
   // If we're aborting, we don't care about any more response data.
   if (this.res) {
@@ -326,10 +328,14 @@ ClientRequest.prototype.abort = function abort() {
   // the request queue through handling in onSocket.
   if (this.socket) {
     // in-progress
-    this.socket.destroy();
+    this.socket.destroy(error);
+  } else {
+    this._destroyError = error;
   }
 };
-
+ClientRequest.prototype.abort = function abort() {
+  this.destroy();
+};
 
 function emitAbortNT() {
   this.emit('abort');
@@ -708,8 +714,11 @@ function onSocketNT(req, socket) {
   if (req.aborted) {
     // If we were aborted while waiting for a socket, skip the whole thing.
     if (!req.agent) {
-      socket.destroy();
+      socket.destroy(req._destroyError);
     } else {
+      if (req._destroyError) {
+        req.emit('error', req._destroyError);
+      }
       socket.emit('free');
     }
   } else {
diff --git a/test/parallel/test-http-client-close-event.js b/test/parallel/test-http-client-close-event.js
index 7573931ac48ef6..4bd0832e4cfb6f 100644
--- a/test/parallel/test-http-client-close-event.js
+++ b/test/parallel/test-http-client-close-event.js
@@ -26,5 +26,7 @@ server.listen(0, common.mustCall(() => {
     server.close();
   }));
 
-  req.destroy();
+  const err = new Error('socket hang up');
+  err.code = 'ECONNRESET';
+  req.destroy(err);
 }));
diff --git a/test/parallel/test-http-client-set-timeout.js b/test/parallel/test-http-client-set-timeout.js
index 15aae7023bf3f1..9d267de6ac8dca 100644
--- a/test/parallel/test-http-client-set-timeout.js
+++ b/test/parallel/test-http-client-set-timeout.js
@@ -43,6 +43,8 @@ server.listen(0, mustCall(() => {
 
   req.on('timeout', mustCall(() => {
     strictEqual(req.socket.listenerCount('timeout'), 0);
-    req.destroy();
+    const err = new Error('socket hang up');
+    err.code = 'ECONNRESET';
+    req.destroy(err);
   }));
 }));

From dfcaf1848cd4fc104cb809d5595907e2e4b00e58 Mon Sep 17 00:00:00 2001
From: Robert Nagy <ronagy@icloud.com>
Date: Fri, 7 Jun 2019 14:20:43 +0200
Subject: [PATCH 2/3] http: fix test where aborted should not be emitted

---
 doc/api/http.md                               |  5 ++--
 lib/_http_client.js                           | 16 +++++++++----
 test/parallel/test-http-client-aborted.js     | 23 +++++++++++++++++++
 ...test-http-client-no-error-after-aborted.js | 21 +++++++++++++++++
 .../test-http-client-timeout-on-connect.js    |  3 +--
 .../test-http-writable-true-after-close.js    |  2 +-
 6 files changed, 60 insertions(+), 10 deletions(-)
 create mode 100644 test/parallel/test-http-client-aborted.js
 create mode 100644 test/parallel/test-http-client-no-error-after-aborted.js

diff --git a/doc/api/http.md b/doc/api/http.md
index 89d41201cd068f..172b63c23ea210 100644
--- a/doc/api/http.md
+++ b/doc/api/http.md
@@ -552,7 +552,8 @@ deprecated: REPLACEME
 > Stability: 0 - Deprecated: Use [`request.destroy()`][] instead.
 
 Marks the request as aborting. Calling this will cause remaining data
-in the response to be dropped and the socket to be destroyed.
+in the response to be dropped and the socket to be destroyed. After
+calling this method no further errors will be emitted.
 
 ### request.aborted
 <!-- YAML
@@ -2170,8 +2171,6 @@ will be emitted in the following order:
 * `'socket'`
 * (`req.abort()` called here)
 * `'abort'`
-* `'error'` with an error with message `'Error: socket hang up'` and code
-  `'ECONNRESET'`
 * `'close'`
 
 If `req.abort()` is called after the response is received, the following events
diff --git a/lib/_http_client.js b/lib/_http_client.js
index 7a80ce88dff394..2a0eed37de9ba5 100644
--- a/lib/_http_client.js
+++ b/lib/_http_client.js
@@ -376,7 +376,9 @@ function socketCloseListener() {
       // receive a response. The error needs to
       // fire on the request.
       req.socket._hadError = true;
-      req.emit('error', connResetException('socket hang up'));
+      if (!req.aborted) {
+        req.emit('error', connResetException('socket hang up'));
+      }
     }
     req.emit('close');
   }
@@ -402,7 +404,9 @@ function socketErrorListener(err) {
     // For Safety. Some additional errors might fire later on
     // and we need to make sure we don't double-fire the error event.
     req.socket._hadError = true;
-    req.emit('error', err);
+    if (!req.aborted) {
+      req.emit('error', err);
+    }
   }
 
   // Handle any pending data
@@ -436,7 +440,9 @@ function socketOnEnd() {
     // If we don't have a response then we know that the socket
     // ended prematurely and we need to emit an error on the request.
     req.socket._hadError = true;
-    req.emit('error', connResetException('socket hang up'));
+    if (!req.aborted) {
+      req.emit('error', connResetException('socket hang up'));
+    }
   }
   if (parser) {
     parser.finish();
@@ -459,7 +465,9 @@ function socketOnData(d) {
     freeParser(parser, req, socket);
     socket.destroy();
     req.socket._hadError = true;
-    req.emit('error', ret);
+    if (!req.aborted) {
+      req.emit('error', ret);
+    }
   } else if (parser.incoming && parser.incoming.upgrade) {
     // Upgrade (if status code 101) or CONNECT
     var bytesParsed = ret;
diff --git a/test/parallel/test-http-client-aborted.js b/test/parallel/test-http-client-aborted.js
new file mode 100644
index 00000000000000..58e8d589c79dc0
--- /dev/null
+++ b/test/parallel/test-http-client-aborted.js
@@ -0,0 +1,23 @@
+'use strict';
+
+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();
+  }));
+  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) => {
+    req.abort();
+  }));
+}));
diff --git a/test/parallel/test-http-client-no-error-after-aborted.js b/test/parallel/test-http-client-no-error-after-aborted.js
new file mode 100644
index 00000000000000..21f72008a93441
--- /dev/null
+++ b/test/parallel/test-http-client-no-error-after-aborted.js
@@ -0,0 +1,21 @@
+'use strict';
+
+const common = require('../common');
+const http = require('http');
+
+const server = http.createServer(common.mustCall((req, res) => {
+  res.write('hello');
+}));
+
+server.listen(0, common.mustCall(() => {
+  const req = http.get({
+    port: server.address().port
+  }, common.mustCall((res) => {
+    req.on('error', common.mustNotCall());
+    req.abort();
+    req.socket.destroy(new Error());
+    req.on('close', common.mustCall(() => {
+      server.close();
+    }));
+  }));
+}));
diff --git a/test/parallel/test-http-client-timeout-on-connect.js b/test/parallel/test-http-client-timeout-on-connect.js
index 3a0098229d9af8..d2c6975e2578c0 100644
--- a/test/parallel/test-http-client-timeout-on-connect.js
+++ b/test/parallel/test-http-client-timeout-on-connect.js
@@ -23,8 +23,7 @@ server.listen(0, common.localhostIPv4, common.mustCall(() => {
     }));
   }));
   req.on('timeout', common.mustCall(() => req.abort()));
-  req.on('error', common.mustCall((err) => {
-    assert.strictEqual(err.message, 'socket hang up');
+  req.on('abort', common.mustCall(() => {
     server.close();
   }));
 }));
diff --git a/test/parallel/test-http-writable-true-after-close.js b/test/parallel/test-http-writable-true-after-close.js
index c0db7c34492762..1174eec2ec1ac7 100644
--- a/test/parallel/test-http-writable-true-after-close.js
+++ b/test/parallel/test-http-writable-true-after-close.js
@@ -34,7 +34,7 @@ const server = createServer(common.mustCall((req, res) => {
     }));
   }).listen(0, () => {
     external = get(`http://127.0.0.1:${server.address().port}`);
-    external.on('error', common.mustCall(() => {
+    external.on('abort', common.mustCall(() => {
       server.close();
       internal.close();
     }));

From 6f95d70f5db364a31301306688d1e33a2522d500 Mon Sep 17 00:00:00 2001
From: Robert Nagy <ronagy@icloud.com>
Date: Tue, 16 Jul 2019 11:15:48 +0200
Subject: [PATCH 3/3] http: don't emit error after destroy or multiple times

---
 doc/api/deprecations.md |  4 ++--
 doc/api/http.md         | 16 +++++++------
 lib/_http_client.js     | 51 ++++++++++++++++++++---------------------
 3 files changed, 36 insertions(+), 35 deletions(-)

diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md
index db44cb3ee08214..3319755eefbe70 100644
--- a/doc/api/deprecations.md
+++ b/doc/api/deprecations.md
@@ -2511,8 +2511,8 @@ changes:
 
 Type: Documentation-only
 
-`[ClientRequest.destroy()`][] should be the same as
-`[ClientRequest.abort()`][]. Make ClientRequest more streamlike by deprecating
+[`ClientRequest.destroy()`][] should be the same as
+[`ClientRequest.abort()`][]. Make ClientRequest more streamlike by deprecating
  abort().
 
 [`--http-parser=legacy`]: cli.html#cli_http_parser_library
diff --git a/doc/api/http.md b/doc/api/http.md
index 172b63c23ea210..f3f2348e91bf0d 100644
--- a/doc/api/http.md
+++ b/doc/api/http.md
@@ -553,7 +553,7 @@ deprecated: REPLACEME
 
 Marks the request as aborting. Calling this will cause remaining data
 in the response to be dropped and the socket to be destroyed. After
-calling this method no further errors will be emitted.
+calling this method, no further errors will be emitted.
 
 ### request.aborted
 <!-- YAML
@@ -2165,22 +2165,24 @@ In the case of a connection error, the following events will be emitted:
 * `'error'`
 * `'close'`
 
-If `req.abort()` is called before the connection succeeds, the following events
-will be emitted in the following order:
+If `req.destroy()` is called before the connection succeeds, the following
+events will be emitted in the following order:
 
 * `'socket'`
-* (`req.abort()` called here)
+* (`req.destroy(err)` called here)
 * `'abort'`
+* `'error'` if `err` was provided.
 * `'close'`
 
-If `req.abort()` is called after the response is received, the following events
-will be emitted in the following order:
+If `req.destroy()` is called 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
-* (`req.abort()` called here)
+* (`req.destroy(err)` called here)
 * `'abort'`
+* `'error'` if `err` was provided.
 * `'aborted'` on the `res` object
 * `'close'`
 * `'end'` on the `res` object
diff --git a/lib/_http_client.js b/lib/_http_client.js
index 2a0eed37de9ba5..c3bf83435a0582 100644
--- a/lib/_http_client.js
+++ b/lib/_http_client.js
@@ -190,6 +190,7 @@ function ClientRequest(input, options, cb) {
   }
 
   this._ended = false;
+  this._errorEmitted = false;
   this.res = null;
   this.aborted = false;
   this.timeoutCb = null;
@@ -265,7 +266,7 @@ function ClientRequest(input, options, cb) {
       return;
     called = true;
     if (err) {
-      process.nextTick(() => this.emit('error', err));
+      process.nextTick(emitError, this, err);
       return;
     }
     this.onSocket(socket);
@@ -324,6 +325,11 @@ ClientRequest.prototype.destroy = function destroy(error) {
     this.res._dump();
   }
 
+  if (!error) {
+    // No more errors after destroy has been called without error.
+    this._errorEmitted = true;
+  }
+
   // In the event that we don't have a socket, we will pop out of
   // the request queue through handling in onSocket.
   if (this.socket) {
@@ -337,6 +343,13 @@ ClientRequest.prototype.abort = function abort() {
   this.destroy();
 };
 
+function emitError(req, err) {
+  if (!req._errorEmitted) {
+    req._errorEmitted = true;
+    req.emit('error', err);
+  }
+}
+
 function emitAbortNT() {
   this.emit('abort');
 }
@@ -371,15 +384,10 @@ function socketCloseListener() {
       res.emit('close');
     }
   } else {
-    if (!req.socket._hadError) {
-      // This socket error fired before we started to
-      // receive a response. The error needs to
-      // fire on the request.
-      req.socket._hadError = true;
-      if (!req.aborted) {
-        req.emit('error', connResetException('socket hang up'));
-      }
-    }
+    // This socket error fired before we started to
+    // receive a response. The error needs to
+    // fire on the request.
+    emitError(req, connResetException('socket hang up'));
     req.emit('close');
   }
 
@@ -401,12 +409,7 @@ function socketErrorListener(err) {
   debug('SOCKET ERROR:', err.message, err.stack);
 
   if (req) {
-    // For Safety. Some additional errors might fire later on
-    // and we need to make sure we don't double-fire the error event.
-    req.socket._hadError = true;
-    if (!req.aborted) {
-      req.emit('error', err);
-    }
+    emitError(req, err);
   }
 
   // Handle any pending data
@@ -436,13 +439,10 @@ function socketOnEnd() {
   const req = this._httpMessage;
   const parser = this.parser;
 
-  if (!req.res && !req.socket._hadError) {
+  if (!req.res) {
     // If we don't have a response then we know that the socket
     // ended prematurely and we need to emit an error on the request.
-    req.socket._hadError = true;
-    if (!req.aborted) {
-      req.emit('error', connResetException('socket hang up'));
-    }
+    emitError(req, connResetException('socket hang up'));
   }
   if (parser) {
     parser.finish();
@@ -464,10 +464,7 @@ function socketOnData(d) {
     debug('parse error', ret);
     freeParser(parser, req, socket);
     socket.destroy();
-    req.socket._hadError = true;
-    if (!req.aborted) {
-      req.emit('error', ret);
-    }
+    emitError(req, ret);
   } else if (parser.incoming && parser.incoming.upgrade) {
     // Upgrade (if status code 101) or CONNECT
     var bytesParsed = ret;
@@ -725,10 +722,12 @@ function onSocketNT(req, socket) {
       socket.destroy(req._destroyError);
     } else {
       if (req._destroyError) {
-        req.emit('error', req._destroyError);
+        emitError(req, req._destroyError);
       }
+      req.emit('close');
       socket.emit('free');
     }
+    req._destroyError = null;
   } else {
     tickOnSocket(req, socket);
   }