http: fix double AbortSignal registration

Linkgoron · ruyadorno · commit e9c161ce129f · 2021-03-23T21:15:06.000-04:00
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>
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');

-Original file line number
+Diff line change
 options.headers);
+}
 +letoptsWithoutSignal=options;
 +if(optsWithoutSignal.signal){
 +optsWithoutSignal=ObjectAssign({},options);
 +deleteoptsWithoutSignal.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(typeofoptions.createConnection==='function'){
 +if(typeofoptsWithoutSignal.createConnection==='function'){
 constoncreate=once((err,socket)=>{
 if(err){
 process.nextTick(()=>this.emit('error',err));
 });
 try{
 -constnewSocket=options.createConnection(options,oncreate);
 +constnewSocket=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));
+}
+}
+}
-Original file line number
+Diff line change
@@ @@ -0,0 +1,73 @@ @@
 +'use strict';
 +constcommon=require('../common');
 +constassert=require('assert');
 +consthttp=require('http');
 +constAgent=http.Agent;
 +const{ getEventListeners, once }=require('events');
 +constagent=newAgent();
 +constserver=http.createServer();
++
 +server.listen(0,common.mustCall(async()=>{
 +constport=server.address().port;
 +consthost='localhost';
 +constoptions={
 +port: port,
 +host: host,
 +_agentKey: agent.getName({ port, host })
 +};
++
 +asyncfunctionpostCreateConnection(){
 +constac=newAbortController();
 +const{ signal }=ac;
 +constconnection=agent.createConnection({ ...options, signal });
 +assert.strictEqual(getEventListeners(signal,'abort').length,1);
 +ac.abort();
 +const[err]=awaitonce(connection,'error');
 +assert.strictEqual(err?.name,'AbortError');
 +}
++
 +asyncfunctionpreCreateConnection(){
 +constac=newAbortController();
 +const{ signal }=ac;
 +ac.abort();
 +constconnection=agent.createConnection({ ...options, signal });
 +const[err]=awaitonce(connection,'error');
 +assert.strictEqual(err?.name,'AbortError');
 +}
++
 +asyncfunctionagentAsParam(){
 +constac=newAbortController();
 +const{ signal }=ac;
 +constrequest=http.get({
 +port: server.address().port,
 +path: '/hello',
 +agent: agent,
 + signal,
 +});
 +assert.strictEqual(getEventListeners(signal,'abort').length,1);
 +ac.abort();
 +const[err]=awaitonce(request,'error');
 +assert.strictEqual(err?.name,'AbortError');
 +}
++
 +asyncfunctionagentAsParamPreAbort(){
 +constac=newAbortController();
 +const{ signal }=ac;
 +ac.abort();
 +constrequest=http.get({
 +port: server.address().port,
 +path: '/hello',
 +agent: agent,
 + signal,
 +});
 +assert.strictEqual(getEventListeners(signal,'abort').length,0);
 +const[err]=awaitonce(request,'error');
 +assert.strictEqual(err?.name,'AbortError');
 +}
++
 +awaitpostCreateConnection();
 +awaitpreCreateConnection();
 +awaitagentAsParam();
 +awaitagentAsParamPreAbort();
 +server.close();
 +}));
-Original file line number
+Diff line change
 constcommon=require('../common');
 consthttp=require('http');
 constassert=require('assert');
 +const{ getEventListeners }=require('events');
+{
 // abort
+{
 -// Destroy with AbortSignal
 +// Destroy post-abort sync with AbortSignal
 constserver=http.createServer(common.mustNotCall());
 constcontroller=newAbortController();
+-
 +const{ signal }=controller;
 server.listen(0,common.mustCall(()=>{
 -constoptions={port: server.address().port,signal: controller.signal};
 +constoptions={port: server.address().port, signal };
 constreq=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
 +constserver=http.createServer(common.mustNotCall());
 +constcontroller=newAbortController();
 +const{ signal }=controller;
 +server.listen(0,common.mustCall(()=>{
 +constoptions={port: server.address().port, signal };
 +constreq=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
 +constserver=http.createServer(common.mustNotCall());
 +constcontroller=newAbortController();
 +const{ signal }=controller;
 +server.listen(0,common.mustCall(()=>{
 +controller.abort();
 +constoptions={port: server.address().port, signal };
 +constreq=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);
 +}));
 +}