http: fix double AbortSignal registration

Fix an issue where AbortSignals are registered twice

lib/_http_client.js

@@ -288,14 +288,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(() => emitError(this, err));
@@ -305,16 +311,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));
  }
  }
 }

test/parallel/test-http-agent-abort-controller.js

@@ -0,0 +1,94 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const http = require('http');
+const Agent = require('_http_agent').Agent;
+const { getEventListeners } = require('events');
+const agent = new Agent();
+const server = http.createServer();
+
+server.listen(0, common.mustCall(() => {
+ const port = server.address().port;
+ const host = 'localhost';
+ const options = {
+ port: port,
+ host: host,
+ rejectUnauthorized: false,
+ _agentKey: agent.getName({ port, host })
+ };
+
+ function postCreateConnection() {
+ return new Promise((res) => {
+ const ac = new AbortController();
+ const { signal } = ac;
+ const connection = agent.createConnection({ ...options, signal });
+ connection.on('error', common.mustCall((err) => {
+ assert(err);
+ assert.strictEqual(err.name, 'AbortError');
+ res();
+ }));
+ assert.strictEqual(getEventListeners(signal, 'abort').length, 1);
+ ac.abort();
+ });
+ }
+
+ function preCreateConnection() {
+ return new Promise((res) => {
+ const ac = new AbortController();
+ const { signal } = ac;
+ ac.abort();
+ const connection = agent.createConnection({ ...options, signal });
+ connection.on('error', common.mustCall((err) => {
+ assert(err);
+ assert.strictEqual(err.name, 'AbortError');
+ res();
+ }));
+ });
+ }
+
+ function agentAsParam() {
+ return new Promise((res) => {
+ const ac = new AbortController();
+ const { signal } = ac;
+ const request = http.get({
+ port: server.address().port,
+ path: '/hello',
+ agent: agent,
+ signal,
+ });
+ request.on('error', common.mustCall((err) => {
+ assert(err);
+ assert.strictEqual(err.name, 'AbortError');
+ res();
+ }));
+ assert.strictEqual(getEventListeners(signal, 'abort').length, 1);
+ ac.abort();
+ });
+ }
+
+ function agentAsParamPreAbort() {
+ return new Promise((res) => {
+ const ac = new AbortController();
+ const { signal } = ac;
+ ac.abort();
+ const request = http.get({
+ port: server.address().port,
+ path: '/hello',
+ agent: agent,
+ signal,
+ });
+ request.on('error', common.mustCall((err) => {
+ assert(err);
+ assert.strictEqual(err.name, 'AbortError');
+ res();
+ }));
+ assert.strictEqual(getEventListeners(signal, 'abort').length, 0);
+ });
+ }
+
+ postCreateConnection()
+ .then(common.mustCall(preCreateConnection))
+ .then(common.mustCall(agentAsParam))
+ .then(common.mustCall(agentAsParamPreAbort))
+ .then(common.mustCall(() => server.close()));
+}));

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
@@ -75,19 +76,39 @@ const assert = require('assert');
 
  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 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());
+ 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);
+ }));
+}

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
@@ -241,3 +240,82 @@ const Countdown = require('../common/countdown');
  req.on('close', common.mustCall(() => server.close()));
  }));
 }
+
+// Destroy ClientHttpSession with AbortSignal
+{
+ const server = h2.createServer();
+ const controller = new AbortController();
+
+ server.on('stream', common.mustNotCall());
+ server.listen(0, common.mustCall(() => {
+ const { signal } = controller;
+ const client = h2.connect(`http://localhost:${server.address().port}`, {
+ signal,
+ });
+ client.on('close', common.mustCall());
+ assert.strictEqual(getEventListeners(signal, 'abort').length, 1);
+
+ client.on('error', common.mustCall(common.mustCall((err) => {
+ assert.strictEqual(err.code, 'ABORT_ERR');
+ assert.strictEqual(err.name, 'AbortError');
+ })));
+
+ const req = client.request({}, {});
+ assert.strictEqual(getEventListeners(signal, 'abort').length, 1);
+
+ req.on('error', common.mustCall((err) => {
+ assert.strictEqual(err.code, 'ERR_HTTP2_STREAM_CANCEL');
+ assert.strictEqual(err.name, 'Error');
+ assert.strictEqual(req.aborted, false);
+ assert.strictEqual(req.destroyed, true);
+ }));
+ req.on('close', common.mustCall(() => server.close()));
+
+ assert.strictEqual(req.aborted, false);
+ assert.strictEqual(req.destroyed, false);
+ // Signal listener attached
+ assert.strictEqual(getEventListeners(signal, 'abort').length, 1);
+
+ controller.abort();
+ }));
+}
+
+// Destroy ClientHttpSession with AbortSignal
+{
+ const server = h2.createSecureServer();
+ const controller = new AbortController();
+
+ server.on('stream', common.mustNotCall());
+ server.listen(0, common.mustCall(() => {
+ const { signal } = controller;
+ const client = h2.connect(`https://localhost:${server.address().port}`, {
+ signal,
+ });
+ client.on('close', common.mustCall());
+ console.log(getEventListeners(signal, 'abort').length);
+ assert.strictEqual(getEventListeners(signal, 'abort').length, 1);
+
+ client.on('error', common.mustCall(common.mustCall((err) => {
+ assert.strictEqual(err.code, 'ABORT_ERR');
+ assert.strictEqual(err.name, 'AbortError');
+ })));
+
+ const req = client.request({}, {});
+ assert.strictEqual(getEventListeners(signal, 'abort').length, 1);
+
+ req.on('error', common.mustCall((err) => {
+ assert.strictEqual(err.code, 'ERR_HTTP2_STREAM_CANCEL');
+ assert.strictEqual(err.name, 'Error');
+ assert.strictEqual(req.aborted, false);
+ assert.strictEqual(req.destroyed, true);
+ }));
+ req.on('close', common.mustCall(() => server.close()));
+
+ assert.strictEqual(req.aborted, false);
+ assert.strictEqual(req.destroyed, false);
+ // Signal listener attached
+ assert.strictEqual(getEventListeners(signal, 'abort').length, 1);
+
+ controller.abort();
+ }));
+}

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