From ba7f38b1f376778f279b22a9a44f56c1079fe3b4 Mon Sep 17 00:00:00 2001
From: Madara Uchiha <me@madara.ninja>
Date: Tue, 10 Nov 2020 19:06:41 +0200
Subject: [PATCH 1/9] http2: add support for AbortSignal to
 http2Session.request

- Add support
- Add test
- Docs once PR is up
---
 lib/internal/http2/core.js                 | 11 ++++++++
 test/parallel/test-http2-client-destroy.js | 29 ++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index 2ea8eacd0c10a2..512b2fa6fadb7c 100644
--- a/lib/internal/http2/core.js
+++ b/lib/internal/http2/core.js
@@ -118,6 +118,7 @@ const {
   validateNumber,
   validateString,
   validateUint32,
+  validateAbortSignal,
 } = require('internal/validators');
 const fsPromisesInternal = require('internal/fs/promises');
 const { utcDate } = require('internal/http');
@@ -1721,6 +1722,16 @@ class ClientHttp2Session extends Http2Session {
     if (options.waitForTrailers)
       stream[kState].flags |= STREAM_FLAGS_HAS_TRAILERS;
 
+    const signal = options.signal;
+    if (options.signal) {
+      validateAbortSignal(signal, 'options.signal');
+      const listener = () => stream.destroy();
+      signal.addEventListener('abort', listener);
+      stream.once('close', () => {
+        signal.removeEventListener('abort', listener);
+      })
+    }
+
     const onConnect = FunctionPrototypeBind(requestOnConnect,
                                             stream, headersList, options);
     if (this.connecting) {
diff --git a/test/parallel/test-http2-client-destroy.js b/test/parallel/test-http2-client-destroy.js
index c91e88079db6bb..178eaf3d1e5ac7 100644
--- a/test/parallel/test-http2-client-destroy.js
+++ b/test/parallel/test-http2-client-destroy.js
@@ -167,3 +167,32 @@ const Countdown = require('../common/countdown');
     req.on('close', common.mustCall(() => server.close()));
   }));
 }
+
+// Destroy with AbortSignal
+{
+  const server = h2.createServer();
+  const controller = new AbortController();
+
+  server.on('stream', common.mustNotCall());
+  server.listen(0, common.mustCall(() => {
+    const client = h2.connect(`http://localhost:${server.address().port}`);
+    client.on('close', common.mustCall());
+    const socket = client[kSocket];
+    socket.on('close', common.mustCall(() => {
+      assert(socket.destroyed);
+    }));
+
+    const req = client.request({}, { signal: controller.signal });
+    client.on('error', common.mustCall((err) => {
+      assert.strictEqual(err.code, 'ECONNREFUSED');
+      server.close();
+    }));
+
+    assert.strictEqual(req.aborted, false);
+    assert.strictEqual(req.destroyed, false);
+    controller.abort();
+    assert.strictEqual(req.aborted, false);
+    assert.strictEqual(req.destroyed, true);
+    req.on('close', common.mustCall(() => server.close()));
+  }));
+}
\ No newline at end of file

From aec2c2b682068b5d1fc19e827fb2d55048173c47 Mon Sep 17 00:00:00 2001
From: Madara Uchiha <me@madara.ninja>
Date: Tue, 10 Nov 2020 19:14:04 +0200
Subject: [PATCH 2/9] http2: Document AbortSignalSupport

---
 doc/api/http2.md | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/doc/api/http2.md b/doc/api/http2.md
index 9fc447b657b368..6abccafefa48ad 100644
--- a/doc/api/http2.md
+++ b/doc/api/http2.md
@@ -2,6 +2,10 @@
 <!-- YAML
 added: v8.4.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/36070
+    description: It is possible to abort a request with an AbortSignal.
+
   - version: v15.0.0
     pr-url: https://github.com/nodejs/node/pull/34664
     description: Requests with the `host` header (with or without
@@ -846,6 +850,8 @@ added: v8.4.0
     and `256` (inclusive).
   * `waitForTrailers` {boolean} When `true`, the `Http2Stream` will emit the
     `'wantTrailers'` event after the final `DATA` frame has been sent.
+  * `signal` {AbortSignal} An AbortSignal that may be used to abort an ongoing
+    request.
 
 * Returns: {ClientHttp2Stream}
 
@@ -882,6 +888,10 @@ close when the final `DATA` frame is transmitted. User code must call either
 `http2stream.sendTrailers()` or `http2stream.close()` to close the
 `Http2Stream`.
 
+When `options.signal` is set with an `AbortSignal` and then calling `abort` on
+the corresponding `AbortController` will behave the same way as calling
+`.destroy()` on the request itself.
+
 The `:method` and `:path` pseudo-headers are not specified within `headers`,
 they respectively default to:
 

From a88f87bc1e903bb8020e77b5656e8aaef0a53e46 Mon Sep 17 00:00:00 2001
From: Madara Uchiha <me@madara.ninja>
Date: Tue, 10 Nov 2020 19:17:55 +0200
Subject: [PATCH 3/9] http2: Fix lint errors

---
 lib/internal/http2/core.js                 | 2 +-
 test/parallel/test-http2-client-destroy.js | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index 512b2fa6fadb7c..e6bd5cf05cee17 100644
--- a/lib/internal/http2/core.js
+++ b/lib/internal/http2/core.js
@@ -1729,7 +1729,7 @@ class ClientHttp2Session extends Http2Session {
       signal.addEventListener('abort', listener);
       stream.once('close', () => {
         signal.removeEventListener('abort', listener);
-      })
+      });
     }
 
     const onConnect = FunctionPrototypeBind(requestOnConnect,
diff --git a/test/parallel/test-http2-client-destroy.js b/test/parallel/test-http2-client-destroy.js
index 178eaf3d1e5ac7..eb75096037b5fb 100644
--- a/test/parallel/test-http2-client-destroy.js
+++ b/test/parallel/test-http2-client-destroy.js
@@ -195,4 +195,4 @@ const Countdown = require('../common/countdown');
     assert.strictEqual(req.destroyed, true);
     req.on('close', common.mustCall(() => server.close()));
   }));
-}
\ No newline at end of file
+}

From 90ab360227cc1effaa63c664148a5eb650827d56 Mon Sep 17 00:00:00 2001
From: Madara Uchiha <MadaraUchiha@users.noreply.github.com>
Date: Tue, 10 Nov 2020 20:09:40 +0200
Subject: [PATCH 4/9] http2: Remove redundant empty line in docs

- As per code review comment https://github.com/nodejs/node/pull/36070#discussion_r520740695

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
---
 doc/api/http2.md | 1 -
 1 file changed, 1 deletion(-)

diff --git a/doc/api/http2.md b/doc/api/http2.md
index 6abccafefa48ad..948ac1ce05cad7 100644
--- a/doc/api/http2.md
+++ b/doc/api/http2.md
@@ -5,7 +5,6 @@ changes:
   - version: REPLACEME
     pr-url: https://github.com/nodejs/node/pull/36070
     description: It is possible to abort a request with an AbortSignal.
-
   - version: v15.0.0
     pr-url: https://github.com/nodejs/node/pull/34664
     description: Requests with the `host` header (with or without

From 2cd337ed33a3a8ab4e38556505bf0e87d30f5e61 Mon Sep 17 00:00:00 2001
From: Madara Uchiha <me@madara.ninja>
Date: Tue, 10 Nov 2020 21:41:02 +0200
Subject: [PATCH 5/9] http2: Use existing signal constant

- As per code review comment https://github.com/nodejs/node/pull/36070#discussion_r520813679
---
 lib/internal/http2/core.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index e6bd5cf05cee17..a3afaf91be5b55 100644
--- a/lib/internal/http2/core.js
+++ b/lib/internal/http2/core.js
@@ -1723,7 +1723,7 @@ class ClientHttp2Session extends Http2Session {
       stream[kState].flags |= STREAM_FLAGS_HAS_TRAILERS;
 
     const signal = options.signal;
-    if (options.signal) {
+    if (signal) {
       validateAbortSignal(signal, 'options.signal');
       const listener = () => stream.destroy();
       signal.addEventListener('abort', listener);

From 05f68a26599724a561ab28f8a94aeb6c38dcce19 Mon Sep 17 00:00:00 2001
From: Madara Uchiha <me@madara.ninja>
Date: Wed, 11 Nov 2020 00:22:01 +0200
Subject: [PATCH 6/9] http2: Goodify test

- Remove redundant socket check
- Add assertion that AbortSignal event listener gets added then removed.
---
 lib/internal/http2/core.js                 |  2 +-
 test/parallel/test-http2-client-destroy.js | 16 ++++++++++------
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index a3afaf91be5b55..20aa14cd806c3a 100644
--- a/lib/internal/http2/core.js
+++ b/lib/internal/http2/core.js
@@ -1722,7 +1722,7 @@ class ClientHttp2Session extends Http2Session {
     if (options.waitForTrailers)
       stream[kState].flags |= STREAM_FLAGS_HAS_TRAILERS;
 
-    const signal = options.signal;
+    const { signal } = options;
     if (signal) {
       validateAbortSignal(signal, 'options.signal');
       const listener = () => stream.destroy();
diff --git a/test/parallel/test-http2-client-destroy.js b/test/parallel/test-http2-client-destroy.js
index eb75096037b5fb..122f7a6cde0153 100644
--- a/test/parallel/test-http2-client-destroy.js
+++ b/test/parallel/test-http2-client-destroy.js
@@ -8,6 +8,7 @@ if (!common.hasCrypto)
 const assert = require('assert');
 const h2 = require('http2');
 const { kSocket } = require('internal/http2/util');
+const { kEvents } = require('internal/event_target');
 const Countdown = require('../common/countdown');
 
 {
@@ -177,20 +178,23 @@ const Countdown = require('../common/countdown');
   server.listen(0, common.mustCall(() => {
     const client = h2.connect(`http://localhost:${server.address().port}`);
     client.on('close', common.mustCall());
-    const socket = client[kSocket];
-    socket.on('close', common.mustCall(() => {
-      assert(socket.destroyed);
-    }));
-
-    const req = client.request({}, { signal: controller.signal });
     client.on('error', common.mustCall((err) => {
       assert.strictEqual(err.code, 'ECONNREFUSED');
+      assert.strictEqual(signal[kEvents].get('abort'), undefined);
       server.close();
     }));
 
+    const { signal } = controller;
+    assert.strictEqual(signal[kEvents].get('abort'), undefined);
+
+    const req = client.request({}, { signal });
+
     assert.strictEqual(req.aborted, false);
     assert.strictEqual(req.destroyed, false);
+    assert.strictEqual(signal[kEvents].get('abort').size, 1);
+
     controller.abort();
+
     assert.strictEqual(req.aborted, false);
     assert.strictEqual(req.destroyed, true);
     req.on('close', common.mustCall(() => server.close()));

From dc997247fb96e81c554097ff21760ba19e0643b7 Mon Sep 17 00:00:00 2001
From: Madara Uchiha <me@madara.ninja>
Date: Thu, 19 Nov 2020 19:29:00 +0200
Subject: [PATCH 7/9] http2: Abort calls destroy with an AbortError

- Builds on #36048 and #36084
- Modify test to verify this fact
---
 lib/internal/http2/core.js                 |  5 +++--
 test/parallel/test-http2-client-destroy.js | 20 +++++++++++++-------
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index 20aa14cd806c3a..1ea22fd1c122a7 100644
--- a/lib/internal/http2/core.js
+++ b/lib/internal/http2/core.js
@@ -109,7 +109,8 @@ const {
     ERR_OUT_OF_RANGE,
     ERR_SOCKET_CLOSED
   },
-  hideStackFrames
+  hideStackFrames,
+  AbortError
 } = require('internal/errors');
 const {
   isUint32,
@@ -1725,7 +1726,7 @@ class ClientHttp2Session extends Http2Session {
     const { signal } = options;
     if (signal) {
       validateAbortSignal(signal, 'options.signal');
-      const listener = () => stream.destroy();
+      const listener = () => stream.destroy(new AbortError());
       signal.addEventListener('abort', listener);
       stream.once('close', () => {
         signal.removeEventListener('abort', listener);
diff --git a/test/parallel/test-http2-client-destroy.js b/test/parallel/test-http2-client-destroy.js
index 122f7a6cde0153..7aef686b4aca11 100644
--- a/test/parallel/test-http2-client-destroy.js
+++ b/test/parallel/test-http2-client-destroy.js
@@ -178,25 +178,31 @@ const Countdown = require('../common/countdown');
   server.listen(0, common.mustCall(() => {
     const client = h2.connect(`http://localhost:${server.address().port}`);
     client.on('close', common.mustCall());
-    client.on('error', common.mustCall((err) => {
-      assert.strictEqual(err.code, 'ECONNREFUSED');
-      assert.strictEqual(signal[kEvents].get('abort'), undefined);
-      server.close();
-    }));
-
+    
     const { signal } = controller;
     assert.strictEqual(signal[kEvents].get('abort'), undefined);
 
+    client.on('error', common.mustCall(() => {
+      // After underlying stream dies, signal listener detached 
+      assert.strictEqual(signal[kEvents].get('abort'), undefined);
+    }));
+    
     const req = client.request({}, { signal });
 
+    req.on('error', common.mustCall((err) => {
+      assert.strictEqual(err.code, 'ABORT_ERR');
+      assert.strictEqual(err.name, 'AbortError');
+    }));
+    req.on('close', common.mustCall(() => server.close()));
+    
     assert.strictEqual(req.aborted, false);
     assert.strictEqual(req.destroyed, false);
+    // Signal listener attached
     assert.strictEqual(signal[kEvents].get('abort').size, 1);
 
     controller.abort();
 
     assert.strictEqual(req.aborted, false);
     assert.strictEqual(req.destroyed, true);
-    req.on('close', common.mustCall(() => server.close()));
   }));
 }

From 3d5177ddfc3a1dc41a1d23acdef63b1a279b4733 Mon Sep 17 00:00:00 2001
From: Madara Uchiha <me@madara.ninja>
Date: Thu, 19 Nov 2020 19:43:31 +0200
Subject: [PATCH 8/9] http2: Update documentation

- Calling abort no longer behaves like .destroy()
- Fix linting errors
---
 doc/api/http2.md                           | 6 +++---
 test/parallel/test-http2-client-destroy.js | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/doc/api/http2.md b/doc/api/http2.md
index 948ac1ce05cad7..8a676d24de73f1 100644
--- a/doc/api/http2.md
+++ b/doc/api/http2.md
@@ -887,9 +887,9 @@ close when the final `DATA` frame is transmitted. User code must call either
 `http2stream.sendTrailers()` or `http2stream.close()` to close the
 `Http2Stream`.
 
-When `options.signal` is set with an `AbortSignal` and then calling `abort` on
-the corresponding `AbortController` will behave the same way as calling
-`.destroy()` on the request itself.
+When `options.signal` is set with an `AbortSignal` and then `abort` on the
+corresponding `AbortController` is called, the request will emit an `'error'`
+event with an `AbortError` error.
 
 The `:method` and `:path` pseudo-headers are not specified within `headers`,
 they respectively default to:
diff --git a/test/parallel/test-http2-client-destroy.js b/test/parallel/test-http2-client-destroy.js
index 7aef686b4aca11..cfa04e54bcdb8f 100644
--- a/test/parallel/test-http2-client-destroy.js
+++ b/test/parallel/test-http2-client-destroy.js
@@ -178,15 +178,15 @@ const Countdown = require('../common/countdown');
   server.listen(0, common.mustCall(() => {
     const client = h2.connect(`http://localhost:${server.address().port}`);
     client.on('close', common.mustCall());
-    
+
     const { signal } = controller;
     assert.strictEqual(signal[kEvents].get('abort'), undefined);
 
     client.on('error', common.mustCall(() => {
-      // After underlying stream dies, signal listener detached 
+      // After underlying stream dies, signal listener detached
       assert.strictEqual(signal[kEvents].get('abort'), undefined);
     }));
-    
+
     const req = client.request({}, { signal });
 
     req.on('error', common.mustCall((err) => {
@@ -194,7 +194,7 @@ const Countdown = require('../common/countdown');
       assert.strictEqual(err.name, 'AbortError');
     }));
     req.on('close', common.mustCall(() => server.close()));
-    
+
     assert.strictEqual(req.aborted, false);
     assert.strictEqual(req.destroyed, false);
     // Signal listener attached

From 9aca7c4e0f764cf6405868acbc8be9fd6d6cdd1a Mon Sep 17 00:00:00 2001
From: Madara Uchiha <me@madara.ninja>
Date: Thu, 19 Nov 2020 21:00:33 +0200
Subject: [PATCH 9/9] http2: Abort immediately if passed an already aborted
 signal

- Also add test to that effect
---
 lib/internal/http2/core.js                 | 14 +++++----
 test/parallel/test-http2-client-destroy.js | 35 ++++++++++++++++++++++
 2 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index 1ea22fd1c122a7..f685329574efe4 100644
--- a/lib/internal/http2/core.js
+++ b/lib/internal/http2/core.js
@@ -1726,11 +1726,15 @@ class ClientHttp2Session extends Http2Session {
     const { signal } = options;
     if (signal) {
       validateAbortSignal(signal, 'options.signal');
-      const listener = () => stream.destroy(new AbortError());
-      signal.addEventListener('abort', listener);
-      stream.once('close', () => {
-        signal.removeEventListener('abort', listener);
-      });
+      const aborter = () => stream.destroy(new AbortError());
+      if (signal.aborted) {
+        aborter();
+      } else {
+        signal.addEventListener('abort', aborter);
+        stream.once('close', () => {
+          signal.removeEventListener('abort', aborter);
+        });
+      }
     }
 
     const onConnect = FunctionPrototypeBind(requestOnConnect,
diff --git a/test/parallel/test-http2-client-destroy.js b/test/parallel/test-http2-client-destroy.js
index cfa04e54bcdb8f..f257cf6f0006fa 100644
--- a/test/parallel/test-http2-client-destroy.js
+++ b/test/parallel/test-http2-client-destroy.js
@@ -206,3 +206,38 @@ const Countdown = require('../common/countdown');
     assert.strictEqual(req.destroyed, true);
   }));
 }
+// Pass an already destroyed signal to abort immediately.
+{
+  const server = h2.createServer();
+  const controller = new AbortController();
+
+  server.on('stream', common.mustNotCall());
+  server.listen(0, common.mustCall(() => {
+    const client = h2.connect(`http://localhost:${server.address().port}`);
+    client.on('close', common.mustCall());
+
+    const { signal } = controller;
+    controller.abort();
+
+    assert.strictEqual(signal[kEvents].get('abort'), undefined);
+
+    client.on('error', common.mustCall(() => {
+      // After underlying stream dies, signal listener detached
+      assert.strictEqual(signal[kEvents].get('abort'), undefined);
+    }));
+
+    const req = client.request({}, { signal });
+    // Signal already aborted, so no event listener attached.
+    assert.strictEqual(signal[kEvents].get('abort'), undefined);
+
+    assert.strictEqual(req.aborted, false);
+    // Destroyed on same tick as request made
+    assert.strictEqual(req.destroyed, true);
+
+    req.on('error', common.mustCall((err) => {
+      assert.strictEqual(err.code, 'ABORT_ERR');
+      assert.strictEqual(err.name, 'AbortError');
+    }));
+    req.on('close', common.mustCall(() => server.close()));
+  }));
+}