http: ClientRequest.abort is destroy

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

doc/api/http.md

@@ -546,10 +546,14 @@ 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.
+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
@@ -2161,24 +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'` with an error with message `'Error: socket hang up'` and code
-  `'ECONNRESET'`
+* `'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
@@ -2215,6 +2219,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

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);
@@ -311,25 +312,43 @@ 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) {
     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) {
     // in-progress
-    this.socket.destroy();
+    this.socket.destroy(error);
+  } else {
+    this._destroyError = error;
   }
 };
+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');
@@ -365,13 +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;
-      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');
   }
 
@@ -393,10 +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;
-    req.emit('error', err);
+    emitError(req, err);
   }
 
   // Handle any pending data
@@ -426,11 +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;
-    req.emit('error', connResetException('socket hang up'));
+    emitError(req, connResetException('socket hang up'));
   }
   if (parser) {
     parser.finish();
@@ -452,8 +464,7 @@ function socketOnData(d) {
     debug('parse error', ret);
     freeParser(parser, req, socket);
     socket.destroy();
-    req.socket._hadError = true;
-    req.emit('error', ret);
+    emitError(req, ret);
   } else if (parser.incoming && parser.incoming.upgrade) {
     // Upgrade (if status code 101) or CONNECT
     var bytesParsed = ret;
@@ -708,10 +719,15 @@ 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) {
+        emitError(req, req._destroyError);
+      }
+      req.emit('close');
       socket.emit('free');
     }
+    req._destroyError = null;
   } else {
     tickOnSocket(req, socket);
   }

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();
+  }));
+}));

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);
 }));

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();
+    }));
+  }));
+}));

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);
   }));
 }));

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();
   }));
 }));

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();
     }));