http: fix double AbortSignal registration

Fix an issue where AbortSignals are registered twice PR-URL: #37730 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs · Mar 24, 2021 · e9c161c · e9c161c
1 parent 9f61cbd
commit e9c161c
Show file tree

Hide file tree

Showing 5 changed files with 143 additions and 17 deletions.
diff --git a/lib/_http_client.js b/lib/_http_client.js
@@ -304,14 +304,20 @@ function ClientRequest(input, options, cb) {
                       options.headers);
   }
 
+  let optsWithoutSignal = options;
+  if (optsWithoutSignal.signal) {
+    optsWithoutSignal = ObjectAssign({}, options);
+    delete optsWithoutSignal.signal;
+  }
+
   // initiate connection
   if (this.agent) {
-    this.agent.addRequest(this, options);
+    this.agent.addRequest(this, optsWithoutSignal);
   } else {
     // No agent, default to Connection:close.
     this._last = true;
     this.shouldKeepAlive = false;
-    if (typeof options.createConnection === 'function') {
+    if (typeof optsWithoutSignal.createConnection === 'function') {
       const oncreate = once((err, socket) => {
         if (err) {
           process.nextTick(() => this.emit('error', err));
@@ -321,16 +327,17 @@ function ClientRequest(input, options, cb) {
       });
 
       try {
-        const newSocket = options.createConnection(options, oncreate);
+        const newSocket = optsWithoutSignal.createConnection(optsWithoutSignal,
+                                                             oncreate);
         if (newSocket) {
           oncreate(null, newSocket);
         }
       } catch (err) {
         oncreate(err);
       }
     } else {
-      debug('CLIENT use net.createConnection', options);
-      this.onSocket(net.createConnection(options));
+      debug('CLIENT use net.createConnection', optsWithoutSignal);
+      this.onSocket(net.createConnection(optsWithoutSignal));
     }
   }
 }

diff --git a/test/parallel/test-http-agent-abort-controller.js b/test/parallel/test-http-agent-abort-controller.js
@@ -0,0 +1,73 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const http = require('http');
+const Agent = http.Agent;
+const { getEventListeners, once } = require('events');
+const agent = new Agent();
+const server = http.createServer();
+
+server.listen(0, common.mustCall(async () => {
+  const port = server.address().port;
+  const host = 'localhost';
+  const options = {
+    port: port,
+    host: host,
+    _agentKey: agent.getName({ port, host })
+  };
+
+  async function postCreateConnection() {
+    const ac = new AbortController();
+    const { signal } = ac;
+    const connection = agent.createConnection({ ...options, signal });
+    assert.strictEqual(getEventListeners(signal, 'abort').length, 1);
+    ac.abort();
+    const [err] = await once(connection, 'error');
+    assert.strictEqual(err?.name, 'AbortError');
+  }
+
+  async function preCreateConnection() {
+    const ac = new AbortController();
+    const { signal } = ac;
+    ac.abort();
+    const connection = agent.createConnection({ ...options, signal });
+    const [err] = await once(connection, 'error');
+    assert.strictEqual(err?.name, 'AbortError');
+  }
+
+  async function agentAsParam() {
+    const ac = new AbortController();
+    const { signal } = ac;
+    const request = http.get({
+      port: server.address().port,
+      path: '/hello',
+      agent: agent,
+      signal,
+    });
+    assert.strictEqual(getEventListeners(signal, 'abort').length, 1);
+    ac.abort();
+    const [err] = await once(request, 'error');
+    assert.strictEqual(err?.name, 'AbortError');
+  }
+
+  async function agentAsParamPreAbort() {
+    const ac = new AbortController();
+    const { signal } = ac;
+    ac.abort();
+    const request = http.get({
+      port: server.address().port,
+      path: '/hello',
+      agent: agent,
+      signal,
+    });
+    assert.strictEqual(getEventListeners(signal, 'abort').length, 0);
+    const [err] = await once(request, 'error');
+    assert.strictEqual(err?.name, 'AbortError');
+  }
+
+  await postCreateConnection();
+  await preCreateConnection();
+  await agentAsParam();
+  await agentAsParamPreAbort();
+  server.close();
+}));
diff --git a/test/parallel/test-http-client-abort-destroy.js b/test/parallel/test-http-client-abort-destroy.js
@@ -2,6 +2,7 @@
 const common = require('../common');
 const http = require('http');
 const assert = require('assert');
+const { getEventListeners } = require('events');
 
 {
   // abort
@@ -71,23 +72,68 @@ const assert = require('assert');
 
 
 {
-  // Destroy with AbortSignal
+  // Destroy post-abort sync with AbortSignal
 
   const server = http.createServer(common.mustNotCall());
   const controller = new AbortController();
-
+  const { signal } = controller;
   server.listen(0, common.mustCall(() => {
-    const options = { port: server.address().port, signal: controller.signal };
+    const options = { port: server.address().port, 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(getEventListeners(signal, 'abort').length, 1);
     assert.strictEqual(req.aborted, false);
     assert.strictEqual(req.destroyed, false);
     controller.abort();
     assert.strictEqual(req.aborted, false);
     assert.strictEqual(req.destroyed, true);
   }));
 }
+
+{
+  // Use post-abort async AbortSignal
+  const server = http.createServer(common.mustNotCall());
+  const controller = new AbortController();
+  const { signal } = controller;
+  server.listen(0, common.mustCall(() => {
+    const options = { port: server.address().port, 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');
+    }));
+
+    req.on('close', common.mustCall(() => {
+      assert.strictEqual(req.aborted, false);
+      assert.strictEqual(req.destroyed, true);
+      server.close();
+    }));
+
+    assert.strictEqual(getEventListeners(signal, 'abort').length, 1);
+    process.nextTick(() => controller.abort());
+  }));
+}
+
+{
+  // Use pre-aborted AbortSignal
+  const server = http.createServer(common.mustNotCall());
+  const controller = new AbortController();
+  const { signal } = controller;
+  server.listen(0, common.mustCall(() => {
+    controller.abort();
+    const options = { port: server.address().port, signal };
+    const req = http.get(options, common.mustNotCall());
+    assert.strictEqual(getEventListeners(signal, 'abort').length, 0);
+    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, true);
+  }));
+}
diff --git a/test/parallel/test-http2-client-destroy.js b/test/parallel/test-http2-client-destroy.js
@@ -8,9 +8,8 @@ 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');
-
+const { getEventListeners } = require('events');
 {
   const server = h2.createServer();
   server.listen(0, common.mustCall(() => {
@@ -180,11 +179,11 @@ const Countdown = require('../common/countdown');
     client.on('close', common.mustCall());
 
     const { signal } = controller;
-    assert.strictEqual(signal[kEvents].get('abort'), undefined);
+    assert.strictEqual(getEventListeners(signal, 'abort').length, 0);
 
     client.on('error', common.mustCall(() => {
       // After underlying stream dies, signal listener detached
-      assert.strictEqual(signal[kEvents].get('abort'), undefined);
+      assert.strictEqual(getEventListeners(signal, 'abort').length, 0);
     }));
 
     const req = client.request({}, { signal });
@@ -198,7 +197,7 @@ const Countdown = require('../common/countdown');
     assert.strictEqual(req.aborted, false);
     assert.strictEqual(req.destroyed, false);
     // Signal listener attached
-    assert.strictEqual(signal[kEvents].get('abort').size, 1);
+    assert.strictEqual(getEventListeners(signal, 'abort').length, 1);
 
     controller.abort();
 
@@ -219,16 +218,16 @@ const Countdown = require('../common/countdown');
     const { signal } = controller;
     controller.abort();
 
-    assert.strictEqual(signal[kEvents].get('abort'), undefined);
+    assert.strictEqual(getEventListeners(signal, 'abort').length, 0);
 
     client.on('error', common.mustCall(() => {
       // After underlying stream dies, signal listener detached
-      assert.strictEqual(signal[kEvents].get('abort'), undefined);
+      assert.strictEqual(getEventListeners(signal, 'abort').length, 0);
     }));
 
     const req = client.request({}, { signal });
     // Signal already aborted, so no event listener attached.
-    assert.strictEqual(signal[kEvents].get('abort'), undefined);
+    assert.strictEqual(getEventListeners(signal, 'abort').length, 0);
 
     assert.strictEqual(req.aborted, false);
     // Destroyed on same tick as request made

diff --git a/test/parallel/test-https-abortcontroller.js b/test/parallel/test-https-abortcontroller.js
@@ -7,7 +7,7 @@ if (!common.hasCrypto)
 const fixtures = require('../common/fixtures');
 const https = require('https');
 const assert = require('assert');
-const { once } = require('events');
+const { once, getEventListeners } = require('events');
 
 const options = {
   key: fixtures.readKey('agent1-key.pem'),
@@ -29,6 +29,7 @@ const options = {
       rejectUnauthorized: false,
       signal: ac.signal,
     });
+    assert.strictEqual(getEventListeners(ac.signal, 'abort').length, 1);
     process.nextTick(() => ac.abort());
     const [ err ] = await once(req, 'error');
     assert.strictEqual(err.name, 'AbortError');