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