From 2d907537b42c93fd053d114abeee036153963249 Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Fri, 29 Mar 2024 15:22:32 -0700 Subject: [PATCH 1/8] Synchronously update internal sockets length so http.Agent pooling is used Fixes #299 --- packages/agent-base/src/index.ts | 71 ++++++++++++++++++++++++++++---- packages/agent-base/test/test.ts | 49 ++++++++++++++++++++++ 2 files changed, 112 insertions(+), 8 deletions(-) diff --git a/packages/agent-base/src/index.ts b/packages/agent-base/src/index.ts index f3758fb9..da58f528 100644 --- a/packages/agent-base/src/index.ts +++ b/packages/agent-base/src/index.ts @@ -77,26 +77,81 @@ export abstract class Agent extends http.Agent { ); } + // In order to support async signatures in `connect()` and Node's native + // connection pooling in http.Agent, the array of sockets for each origin has + // to be updated syncronously. This is so the length of the array is accurate + // when `addRequest()` is next called. We achieve this by creating a fake socket + // and adding it to this.sockets and incrementing totalSocketCount. + private incrementSockets(name: string) { + // If maxSockets and maxTotalSockets are both Infinity then there is no need + // to create a fake socket because Node.js native connection pooling will + // never be invoked. + if (this.maxSockets === Infinity && this.maxTotalSockets === Infinity) { + return null; + } + if (!this.sockets[name]) { + // All instances of `sockets` are expected TypeScript errors. The alternative is to + // add it as a private property of this class but that will break TypeScript subclassing. + // @ts-expect-error `sockets` is readonly in `@types/node` but we need to write to it + this.sockets[name] = []; + } + const fakeSocket = new net.Socket({ writable: false }); + // @ts-expect-error + this.sockets[name].push(fakeSocket); + // @ts-expect-error `totalSocketCount` isn't defined in `@types/node` + this.totalSocketCount++; + return fakeSocket; + } + + private decrementSockets(name: string, socket: null | net.Socket) { + if (!this.sockets[name] || socket === null) { + return; + } + // @ts-expect-error + const index = this.sockets[name].indexOf(socket); + if (index !== -1) { + // @ts-expect-error + this.sockets[name].splice(index, 1); + // @ts-expect-error + this.totalSocketCount--; + // @ts-expect-error + if (this.sockets[name].length === 0) { + // @ts-expect-error + delete this.sockets[name]; + } + } + } + createSocket( req: http.ClientRequest, options: AgentConnectOpts, cb: (err: Error | null, s?: Duplex) => void ) { + // @ts-expect-error `getName()` isn't defined in `@types/node` + const name = this.getName(options); + const fakeSocket = this.incrementSockets(name); const connectOpts = { ...options, secureEndpoint: this.isSecureEndpoint(options), }; Promise.resolve() .then(() => this.connect(req, connectOpts)) - .then((socket) => { - if (socket instanceof http.Agent) { - // @ts-expect-error `addRequest()` isn't defined in `@types/node` - return socket.addRequest(req, connectOpts); + .then( + (socket) => { + this.decrementSockets(name, fakeSocket); + if (socket instanceof http.Agent) { + // @ts-expect-error `addRequest()` isn't defined in `@types/node` + return socket.addRequest(req, connectOpts); + } + this[INTERNAL].currentSocket = socket; + // @ts-expect-error `createSocket()` isn't defined in `@types/node` + super.createSocket(req, options, cb); + }, + (err) => { + this.decrementSockets(name, fakeSocket); + cb(err); } - this[INTERNAL].currentSocket = socket; - // @ts-expect-error `createSocket()` isn't defined in `@types/node` - super.createSocket(req, options, cb); - }, cb); + ); } createConnection(): Duplex { diff --git a/packages/agent-base/test/test.ts b/packages/agent-base/test/test.ts index 6f49a84d..d4a631a9 100644 --- a/packages/agent-base/test/test.ts +++ b/packages/agent-base/test/test.ts @@ -310,6 +310,55 @@ describe('Agent (TypeScript)', () => { }); }); + it('should support `keepAlive: true` with `maxSockets`', async () => { + let reqCount = 0; + let connectCount = 0; + + class MyAgent extends Agent { + async connect(_req: http.ClientRequest, opts: AgentConnectOpts) { + connectCount++; + assert(opts.secureEndpoint === false); + await sleep(10); + return net.connect(opts); + } + } + const agent = new MyAgent({ keepAlive: true, maxSockets: 1 }); + + const server = http.createServer(async (req, res) => { + expect(req.headers.connection).toEqual('keep-alive'); + reqCount++; + await sleep(10); + res.end(); + }); + const addr = await listen(server); + + try { + const resPromise = req(new URL('/foo', addr), { agent }); + const res2Promise = req(new URL('/another', addr), { agent }); + + const res = await resPromise; + expect(reqCount).toEqual(1); + expect(connectCount).toEqual(1); + expect(res.headers.connection).toEqual('keep-alive'); + + res.resume(); + const s1 = res.socket; + await once(s1, 'free'); + + const res2 = await res2Promise; + expect(reqCount).toEqual(2); + expect(connectCount).toEqual(1); + expect(res2.headers.connection).toEqual('keep-alive'); + assert(res2.socket === s1); + + res2.resume(); + await once(res2.socket, 'free'); + } finally { + agent.destroy(); + server.close(); + } + }); + describe('"https" module', () => { it('should work for basic HTTPS requests', async () => { let gotReq = false; From 50dcbe93aaae8380e4c7294adcda9469567c3f4b Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Fri, 29 Mar 2024 15:37:41 -0700 Subject: [PATCH 2/8] Fix some comments --- packages/agent-base/src/index.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/agent-base/src/index.ts b/packages/agent-base/src/index.ts index da58f528..8498753c 100644 --- a/packages/agent-base/src/index.ts +++ b/packages/agent-base/src/index.ts @@ -78,12 +78,12 @@ export abstract class Agent extends http.Agent { } // In order to support async signatures in `connect()` and Node's native - // connection pooling in http.Agent, the array of sockets for each origin has - // to be updated syncronously. This is so the length of the array is accurate + // connection pooling in `http.Agent`, the array of sockets for each origin has + // to be updated synchronously. This is so the length of the array is accurate // when `addRequest()` is next called. We achieve this by creating a fake socket - // and adding it to this.sockets and incrementing totalSocketCount. + // and adding it to `sockets[origin]` and incrementing `totalSocketCount`. private incrementSockets(name: string) { - // If maxSockets and maxTotalSockets are both Infinity then there is no need + // If `maxSockets` and `maxTotalSockets` are both Infinity then there is no need // to create a fake socket because Node.js native connection pooling will // never be invoked. if (this.maxSockets === Infinity && this.maxTotalSockets === Infinity) { From 634b88b8a35615d2f024bf2239d293a9a7b07407 Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Fri, 29 Mar 2024 15:46:31 -0700 Subject: [PATCH 3/8] Use fewer ts-expect-error comments --- packages/agent-base/src/index.ts | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/packages/agent-base/src/index.ts b/packages/agent-base/src/index.ts index 8498753c..b15d6cae 100644 --- a/packages/agent-base/src/index.ts +++ b/packages/agent-base/src/index.ts @@ -89,15 +89,14 @@ export abstract class Agent extends http.Agent { if (this.maxSockets === Infinity && this.maxTotalSockets === Infinity) { return null; } + // All instances of `sockets` are expected TypeScript errors. The alternative is to + // add it as a private property of this class but that will break TypeScript subclassing. if (!this.sockets[name]) { - // All instances of `sockets` are expected TypeScript errors. The alternative is to - // add it as a private property of this class but that will break TypeScript subclassing. - // @ts-expect-error `sockets` is readonly in `@types/node` but we need to write to it + // @ts-expect-error `sockets` is readonly in `@types/node` this.sockets[name] = []; } const fakeSocket = new net.Socket({ writable: false }); - // @ts-expect-error - this.sockets[name].push(fakeSocket); + (this.sockets[name] as net.Socket[]).push(fakeSocket); // @ts-expect-error `totalSocketCount` isn't defined in `@types/node` this.totalSocketCount++; return fakeSocket; @@ -107,16 +106,14 @@ export abstract class Agent extends http.Agent { if (!this.sockets[name] || socket === null) { return; } - // @ts-expect-error - const index = this.sockets[name].indexOf(socket); + const sockets = this.sockets[name] as net.Socket[]; + const index = sockets.indexOf(socket); if (index !== -1) { - // @ts-expect-error - this.sockets[name].splice(index, 1); - // @ts-expect-error + sockets.splice(index, 1); + // @ts-expect-error `totalSocketCount` isn't defined in `@types/node` this.totalSocketCount--; - // @ts-expect-error - if (this.sockets[name].length === 0) { - // @ts-expect-error + if (sockets.length === 0) { + // @ts-expect-error `sockets` is readonly in `@types/node` delete this.sockets[name]; } } From cdad2e19b712fb88025cc020f96dc54f94613ea8 Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Fri, 29 Mar 2024 16:11:17 -0700 Subject: [PATCH 4/8] Implement getName method to get both http/https names --- packages/agent-base/src/index.ts | 36 +++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/packages/agent-base/src/index.ts b/packages/agent-base/src/index.ts index b15d6cae..a55e47ee 100644 --- a/packages/agent-base/src/index.ts +++ b/packages/agent-base/src/index.ts @@ -1,6 +1,7 @@ import * as net from 'net'; import * as tls from 'tls'; import * as http from 'http'; +import { Agent as HttpsAgent } from 'https'; import type { Duplex } from 'stream'; export * from './helpers'; @@ -78,19 +79,21 @@ export abstract class Agent extends http.Agent { } // In order to support async signatures in `connect()` and Node's native - // connection pooling in `http.Agent`, the array of sockets for each origin has - // to be updated synchronously. This is so the length of the array is accurate - // when `addRequest()` is next called. We achieve this by creating a fake socket - // and adding it to `sockets[origin]` and incrementing `totalSocketCount`. + // connection pooling in `http.Agent`, the array of sockets for each origin + // has to be updated synchronously. This is so the length of the array is + // accurate when `addRequest()` is next called. We achieve this by creating a + // fake socket and adding it to `sockets[origin]` and incrementing + // `totalSocketCount`. private incrementSockets(name: string) { - // If `maxSockets` and `maxTotalSockets` are both Infinity then there is no need - // to create a fake socket because Node.js native connection pooling will - // never be invoked. + // If `maxSockets` and `maxTotalSockets` are both Infinity then there is no + // need to create a fake socket because Node.js native connection pooling + // will never be invoked. if (this.maxSockets === Infinity && this.maxTotalSockets === Infinity) { return null; } - // All instances of `sockets` are expected TypeScript errors. The alternative is to - // add it as a private property of this class but that will break TypeScript subclassing. + // All instances of `sockets` are expected TypeScript errors. The + // alternative is to add it as a private property of this class but that + // will break TypeScript subclassing. if (!this.sockets[name]) { // @ts-expect-error `sockets` is readonly in `@types/node` this.sockets[name] = []; @@ -119,18 +122,27 @@ export abstract class Agent extends http.Agent { } } + // In order to properly update the socket pool, we need to call `getName()` on + // the core `https.Agent` if it is a secureEndpoint. + private getName({ secureEndpoint, ...options }: AgentConnectOpts) { + return secureEndpoint + ? // @ts-expect-error `getName()` isn't defined in `@types/node` + HttpsAgent.prototype.getName.call(this, options) + : // @ts-expect-error `getName()` isn't defined in `@types/node` + super.getName(options); + } + createSocket( req: http.ClientRequest, options: AgentConnectOpts, cb: (err: Error | null, s?: Duplex) => void ) { - // @ts-expect-error `getName()` isn't defined in `@types/node` - const name = this.getName(options); - const fakeSocket = this.incrementSockets(name); const connectOpts = { ...options, secureEndpoint: this.isSecureEndpoint(options), }; + const name = this.getName(connectOpts); + const fakeSocket = this.incrementSockets(name); Promise.resolve() .then(() => this.connect(req, connectOpts)) .then( From d8714c525308b46b95e5669284574688a2c5dc8b Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Fri, 29 Mar 2024 16:17:43 -0700 Subject: [PATCH 5/8] Fix linting --- packages/agent-base/src/index.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/agent-base/src/index.ts b/packages/agent-base/src/index.ts index a55e47ee..7a6cb2c8 100644 --- a/packages/agent-base/src/index.ts +++ b/packages/agent-base/src/index.ts @@ -125,11 +125,12 @@ export abstract class Agent extends http.Agent { // In order to properly update the socket pool, we need to call `getName()` on // the core `https.Agent` if it is a secureEndpoint. private getName({ secureEndpoint, ...options }: AgentConnectOpts) { - return secureEndpoint - ? // @ts-expect-error `getName()` isn't defined in `@types/node` - HttpsAgent.prototype.getName.call(this, options) - : // @ts-expect-error `getName()` isn't defined in `@types/node` - super.getName(options); + if (secureEndpoint) { + // @ts-expect-error `getName()` isn't defined in `@types/node` + return HttpsAgent.prototype.getName.call(this, options); + } + // @ts-expect-error `getName()` isn't defined in `@types/node` + return super.getName(options); } createSocket( From 6520a4a40ac8e252d2ef95df14486277f9ea6439 Mon Sep 17 00:00:00 2001 From: Nathan Rajlich Date: Fri, 29 Mar 2024 16:21:51 -0700 Subject: [PATCH 6/8] Create seven-colts-flash.md --- .changeset/seven-colts-flash.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/seven-colts-flash.md diff --git a/.changeset/seven-colts-flash.md b/.changeset/seven-colts-flash.md new file mode 100644 index 00000000..4c37bc2b --- /dev/null +++ b/.changeset/seven-colts-flash.md @@ -0,0 +1,5 @@ +--- +"agent-base": patch +--- + +Synchronously update internal sockets length so `http.Agent` pooling is used From e10a55561d06baec355c030e609fe678c43e7981 Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Fri, 29 Mar 2024 16:43:37 -0700 Subject: [PATCH 7/8] Make getName() public and add tests for it --- packages/agent-base/src/index.ts | 6 +- packages/agent-base/test/test.ts | 98 ++++++++++++++++++-------------- 2 files changed, 59 insertions(+), 45 deletions(-) diff --git a/packages/agent-base/src/index.ts b/packages/agent-base/src/index.ts index 7a6cb2c8..fcf2b57c 100644 --- a/packages/agent-base/src/index.ts +++ b/packages/agent-base/src/index.ts @@ -124,7 +124,11 @@ export abstract class Agent extends http.Agent { // In order to properly update the socket pool, we need to call `getName()` on // the core `https.Agent` if it is a secureEndpoint. - private getName({ secureEndpoint, ...options }: AgentConnectOpts) { + getName(options: AgentConnectOpts): string { + const secureEndpoint = + typeof options.secureEndpoint === 'boolean' + ? options.secureEndpoint + : this.isSecureEndpoint(options); if (secureEndpoint) { // @ts-expect-error `getName()` isn't defined in `@types/node` return HttpsAgent.prototype.getName.call(this, options); diff --git a/packages/agent-base/test/test.ts b/packages/agent-base/test/test.ts index d4a631a9..e3566370 100644 --- a/packages/agent-base/test/test.ts +++ b/packages/agent-base/test/test.ts @@ -79,6 +79,7 @@ describe('Agent (TypeScript)', () => { ) { gotCallback = true; assert(opts.secureEndpoint === false); + assert.equal(this.getName(opts), `127.0.0.1:${port}:`); return net.connect(opts); } } @@ -308,55 +309,60 @@ describe('Agent (TypeScript)', () => { server2.close(); } }); - }); - it('should support `keepAlive: true` with `maxSockets`', async () => { - let reqCount = 0; - let connectCount = 0; + it('should support `keepAlive: true` with `maxSockets`', async () => { + let reqCount = 0; + let connectCount = 0; - class MyAgent extends Agent { - async connect(_req: http.ClientRequest, opts: AgentConnectOpts) { - connectCount++; - assert(opts.secureEndpoint === false); - await sleep(10); - return net.connect(opts); + class MyAgent extends Agent { + async connect( + _req: http.ClientRequest, + opts: AgentConnectOpts + ) { + connectCount++; + assert(opts.secureEndpoint === false); + await sleep(10); + return net.connect(opts); + } } - } - const agent = new MyAgent({ keepAlive: true, maxSockets: 1 }); + const agent = new MyAgent({ keepAlive: true, maxSockets: 1 }); + + const server = http.createServer(async (req, res) => { + expect(req.headers.connection).toEqual('keep-alive'); + reqCount++; + await sleep(10); + res.end(); + }); + const addr = await listen(server); - const server = http.createServer(async (req, res) => { - expect(req.headers.connection).toEqual('keep-alive'); - reqCount++; - await sleep(10); - res.end(); + try { + const resPromise = req(new URL('/foo', addr), { agent }); + const res2Promise = req(new URL('/another', addr), { + agent, + }); + + const res = await resPromise; + expect(reqCount).toEqual(1); + expect(connectCount).toEqual(1); + expect(res.headers.connection).toEqual('keep-alive'); + + res.resume(); + const s1 = res.socket; + await once(s1, 'free'); + + const res2 = await res2Promise; + expect(reqCount).toEqual(2); + expect(connectCount).toEqual(1); + expect(res2.headers.connection).toEqual('keep-alive'); + assert(res2.socket === s1); + + res2.resume(); + await once(res2.socket, 'free'); + } finally { + agent.destroy(); + server.close(); + } }); - const addr = await listen(server); - - try { - const resPromise = req(new URL('/foo', addr), { agent }); - const res2Promise = req(new URL('/another', addr), { agent }); - - const res = await resPromise; - expect(reqCount).toEqual(1); - expect(connectCount).toEqual(1); - expect(res.headers.connection).toEqual('keep-alive'); - - res.resume(); - const s1 = res.socket; - await once(s1, 'free'); - - const res2 = await res2Promise; - expect(reqCount).toEqual(2); - expect(connectCount).toEqual(1); - expect(res2.headers.connection).toEqual('keep-alive'); - assert(res2.socket === s1); - - res2.resume(); - await once(res2.socket, 'free'); - } finally { - agent.destroy(); - server.close(); - } }); describe('"https" module', () => { @@ -371,6 +377,10 @@ describe('Agent (TypeScript)', () => { ): net.Socket { gotCallback = true; assert(opts.secureEndpoint === true); + assert.equal( + this.getName(opts), + `127.0.0.1:${port}::::::::false:::::::::::::` + ); return tls.connect(opts); } } From 4650f933d24aef7f32246a4b0c03f36f7fa4b54d Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Fri, 29 Mar 2024 16:48:33 -0700 Subject: [PATCH 8/8] Add test for keepAlive+maxSockets when using https --- packages/agent-base/test/test.ts | 58 ++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/packages/agent-base/test/test.ts b/packages/agent-base/test/test.ts index e3566370..deeed875 100644 --- a/packages/agent-base/test/test.ts +++ b/packages/agent-base/test/test.ts @@ -568,5 +568,63 @@ describe('Agent (TypeScript)', () => { server.close(); } }); + + it('should support `keepAlive: true` with `maxSockets`', async () => { + let reqCount = 0; + let connectCount = 0; + + class MyAgent extends Agent { + async connect( + _req: http.ClientRequest, + opts: AgentConnectOpts + ) { + connectCount++; + assert(opts.secureEndpoint === true); + await sleep(10); + return tls.connect(opts); + } + } + const agent = new MyAgent({ keepAlive: true, maxSockets: 1 }); + + const server = https.createServer(sslOptions, async (req, res) => { + expect(req.headers.connection).toEqual('keep-alive'); + reqCount++; + await sleep(10); + res.end(); + }); + const addr = await listen(server); + + try { + const resPromise = req(new URL('/foo', addr), { + agent, + rejectUnauthorized: false, + }); + const res2Promise = req(new URL('/another', addr), { + agent, + rejectUnauthorized: false, + }); + + const res = await resPromise; + expect(reqCount).toEqual(1); + expect(connectCount).toEqual(1); + expect(res.headers.connection).toEqual('keep-alive'); + + res.resume(); + const s1 = res.socket; + await once(s1, 'free'); + + const res2 = await res2Promise; + expect(reqCount).toEqual(2); + expect(connectCount).toEqual(1); + expect(res2.headers.connection).toEqual('keep-alive'); + assert(res2.socket === s1); + + res2.resume(); + await once(res2.socket, 'free'); + } finally { + agent.destroy(); + server.close(); + } + }); }); });