http: add support for abortsignal to http.request

benjamingr · codebytere · commit 8390f8a86ba9 · 2020-11-22T13:15:37.000-08:00
PR-URL: #36048 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
diff --git a/doc/api/http.md b/doc/api/http.md
@@ -2336,6 +2336,9 @@ This can be overridden for servers and client requests by passing the
 <!-- YAML
 added: v0.3.6
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/36048
+    description: It is possible to abort a request with an AbortSignal.
   - version:
      - v13.8.0
      - v12.15.0
@@ -2403,6 +2406,8 @@ changes:
      or `port` is specified, those specify a TCP Socket).
   * `timeout` {number}: A number specifying the socket timeout in milliseconds.
     This will set the timeout before the socket is connected.
+  * `signal` {AbortSignal}: An AbortSignal that may be used to abort an ongoing
+    request.
 * `callback` {Function}
 * Returns: {http.ClientRequest}
 
@@ -2596,6 +2601,10 @@ events will be emitted in the following order:
 Setting the `timeout` option or using the `setTimeout()` function will
 not abort the request or do anything besides add a `'timeout'` event.
 
+Passing an `AbortSignal` and then calling `abort` on the corresponding
+`AbortController` will behave the same way as calling `.destroy()` on the
+request itself.
+
 ## `http.validateHeaderName(name)`
 <!-- YAML
 added: v14.3.0
diff --git a/lib/.eslintrc.yaml b/lib/.eslintrc.yaml
@@ -19,7 +19,7 @@ rules:
     - selector: "ThrowStatement > CallExpression[callee.name=/Error$/]"
       message: "Use new keyword when throwing an Error."
     # Config specific to lib
-    - selector: "NewExpression[callee.name=/Error$/]:not([callee.name=/^(AssertionError|NghttpError)$/])"
+    - selector: "NewExpression[callee.name=/Error$/]:not([callee.name=/^(AssertionError|NghttpError|AbortError)$/])"
       message: "Use an error exported by the internal/errors module."
     - selector: "CallExpression[callee.object.name='Error'][callee.property.name='captureStackTrace']"
       message: "Please use `require('internal/errors').hideStackFrames()` instead."
diff --git a/lib/_http_client.js b/lib/_http_client.js
@@ -51,15 +51,18 @@ const { Buffer } = require('buffer');
 const { defaultTriggerAsyncIdScope } = require('internal/async_hooks');
 const { URL, urlToOptions, searchParamsSymbol } = require('internal/url');
 const { kOutHeaders, kNeedDrain } = require('internal/http');
-const { connResetException, codes } = require('internal/errors');
+const { AbortError, connResetException, codes } = require('internal/errors');
 const {
   ERR_HTTP_HEADERS_SENT,
   ERR_INVALID_ARG_TYPE,
   ERR_INVALID_HTTP_TOKEN,
   ERR_INVALID_PROTOCOL,
   ERR_UNESCAPED_CHARACTERS
 } = codes;
-const { validateInteger } = require('internal/validators');
+const {
+  validateInteger,
+  validateAbortSignal,
+} = require('internal/validators');
 const { getTimerDuration } = require('internal/timers');
 const {
   DTRACE_HTTP_CLIENT_REQUEST,
@@ -169,6 +172,15 @@ function ClientRequest(input, options, cb) {
   if (options.timeout !== undefined)
     this.timeout = getTimerDuration(options.timeout, 'timeout');
 
+  const signal = options.signal;
+  if (signal) {
+    validateAbortSignal(signal, 'options.signal');
+    const listener = (e) => this.destroy(new AbortError());
+    signal.addEventListener('abort', listener);
+    this.once('close', () => {
+      signal.removeEventListener('abort', listener);
+    });
+  }
   let method = options.method;
   const methodIsString = (typeof method === 'string');
   if (method !== null && method !== undefined && !methodIsString) {
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
@@ -727,6 +727,16 @@ const fatalExceptionStackEnhancers = {
   }
 };
 
+// Node uses an AbortError that isn't exactly the same as the DOMException
+// to make usage of the error in userland and readable-stream easier.
+// It is a regular error with `.code` and `.name`.
+class AbortError extends Error {
+  constructor() {
+    super('The operation was aborted');
+    this.code = 'ABORT_ERR';
+    this.name = 'AbortError';
+  }
+}
 module.exports = {
   addCodeToName, // Exported for NghttpError
   codes,
@@ -741,6 +751,7 @@ module.exports = {
   uvException,
   uvExceptionWithHostPort,
   SystemError,
+  AbortError,
   // This is exported only to facilitate testing.
   E,
   kNoOverride,
diff --git a/test/parallel/test-http-client-abort-destroy.js b/test/parallel/test-http-client-abort-destroy.js
@@ -52,8 +52,7 @@ const assert = require('assert');
 {
   // destroy
 
-  const server = http.createServer(common.mustNotCall((req, res) => {
-  }));
+  const server = http.createServer(common.mustNotCall());
 
   server.listen(0, common.mustCall(() => {
     const options = { port: server.address().port };
@@ -69,3 +68,26 @@ const assert = require('assert');
     assert.strictEqual(req.destroyed, true);
   }));
 }
+
+
+{
+  // Destroy with AbortSignal
+
+  const server = http.createServer(common.mustNotCall());
+  const controller = new AbortController();
+
+  server.listen(0, common.mustCall(() => {
+    const options = { port: server.address().port, signal: controller.signal };
+    const req = http.get(options, common.mustNotCall());
+    req.on('error', common.mustCall((err) => {
+      assert.strictEqual(err.code, 'ABORT_ERR');
+      assert.strictEqual(err.name, 'AbortError');
+      server.close();
+    }));
+    assert.strictEqual(req.aborted, false);
+    assert.strictEqual(req.destroyed, false);
+    controller.abort();
+    assert.strictEqual(req.aborted, false);
+    assert.strictEqual(req.destroyed, true);
+  }));
+}
