http: don't emit error after destroy or multiple times

ronag · ronag · commit 6f95d70f5db3 · 2019-08-09T18:08:13.000+02:00
diff --git 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
@@ -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
@@ -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);
   }
