diff --git a/.changeset/fast-turkeys-bow.md b/.changeset/fast-turkeys-bow.md new file mode 100644 index 00000000000..8d1565ddd75 --- /dev/null +++ b/.changeset/fast-turkeys-bow.md @@ -0,0 +1,5 @@ +--- +"@smithy/node-http-handler": patch +--- + +consolidate checkSocketUsage timers to reduce overhead diff --git a/packages/node-http-handler/src/node-http-handler.spec.ts b/packages/node-http-handler/src/node-http-handler.spec.ts index 5a7943511d6..98256e55735 100644 --- a/packages/node-http-handler/src/node-http-handler.spec.ts +++ b/packages/node-http-handler/src/node-http-handler.spec.ts @@ -107,10 +107,11 @@ describe("NodeHttpHandler", () => { }), ], ])("sets socketAcquisitionWarningTimeout correctly when input is %s", async (_, option) => { - vi.spyOn(timing, "setTimeout"); + vi.spyOn(global, "setInterval"); const nodeHttpHandler = new NodeHttpHandler(option); await nodeHttpHandler.handle({} as any); - expect(vi.mocked(timing.setTimeout).mock.calls[0][1]).toBe(randomSocketAcquisitionWarningTimeout); + expect(vi.mocked(setInterval).mock.calls[0][1]).toBe(randomSocketAcquisitionWarningTimeout); + nodeHttpHandler.destroy(); }); it.each([ @@ -268,6 +269,7 @@ describe("NodeHttpHandler", () => { describe("per-request requestTimeout", () => { it("should use per-request timeout over handler config timeout", async () => { + vi.spyOn(timing, "setTimeout"); const testTimeout = async (handlerTimeout: number, requestTimeout?: number) => { const handler = new NodeHttpHandler({ requestTimeout: handlerTimeout }); await handler.handle( @@ -410,6 +412,42 @@ describe("NodeHttpHandler", () => { }); }); + describe("socket warning interval", () => { + afterEach(() => { + vi.clearAllMocks(); + }); + + it("does not create an interval when maxSockets is Infinity", async () => { + vi.spyOn(global, "setInterval"); + const handler = new NodeHttpHandler({ + httpAgent: new http.Agent({ maxSockets: Infinity }), + httpsAgent: new https.Agent({ maxSockets: Infinity }), + }); + await handler.handle({ protocol: "http:", headers: {}, method: "GET", hostname: "localhost", path: "/" } as any); + expect(setInterval).not.toHaveBeenCalled(); + handler.destroy(); + }); + + it("creates only one interval across multiple handle() calls", async () => { + vi.spyOn(global, "setInterval"); + const handler = new NodeHttpHandler({ httpAgent: { maxSockets: 25 } }); + const req = { protocol: "http:", headers: {}, method: "GET", hostname: "localhost", path: "/" } as any; + await handler.handle(req); + await handler.handle(req); + await handler.handle(req); + expect(setInterval).toHaveBeenCalledTimes(1); + handler.destroy(); + }); + + it("cleans up the interval on destroy()", async () => { + vi.spyOn(global, "clearInterval"); + const handler = new NodeHttpHandler({ httpAgent: { maxSockets: 25 } }); + await handler.handle({ protocol: "http:", headers: {}, method: "GET", hostname: "localhost", path: "/" } as any); + handler.destroy(); + expect(clearInterval).toHaveBeenCalledTimes(1); + }); + }); + describe("checkSocketUsage", () => { beforeEach(() => { vi.spyOn(console, "warn").mockImplementation(vi.fn() as any); diff --git a/packages/node-http-handler/src/node-http-handler.ts b/packages/node-http-handler/src/node-http-handler.ts index f5554925119..caf0bca011b 100644 --- a/packages/node-http-handler/src/node-http-handler.ts +++ b/packages/node-http-handler/src/node-http-handler.ts @@ -38,6 +38,16 @@ export class NodeHttpHandler implements HttpHandler { private socketWarningTimestamp = 0; private externalAgent = false; + /** + * @internal + * Single interval for socket usage warnings, shared across all requests. + * Lazily created on first handle() call. + * - undefined: not yet initialized + * - null: initialized, but not needed (maxSockets is Infinity) + * - NodeJS.Timeout: active interval + */ + private socketWarningInterval?: NodeJS.Timeout | null; + // Node http handler is hard-coded to http/1.1: https://github.com/nodejs/node/blob/ff5664b83b89c55e4ab5d5f60068fb457f1f5872/lib/_http_server.js#L286 public readonly metadata = { handlerProtocol: "http/1.1" }; @@ -88,13 +98,11 @@ export class NodeHttpHandler implements HttpHandler { /** * Running at maximum socket usage can be intentional and normal. - * That is why this warning emits at a delay which can be seen - * at the call site's setTimeout wrapper. The warning will be cancelled - * if the request finishes in a reasonable amount of time regardless - * of socket saturation. + * That is why this check runs on a shared interval rather than + * immediately, giving transient spikes time to resolve. * - * Additionally, when the warning is emitted, there is an interval - * lockout. + * Additionally, when the warning is emitted, there is a 15-second + * lockout to avoid log spam. */ if (socketsInUse >= maxSockets && requestsEnqueued >= 2 * maxSockets) { logger?.warn?.( @@ -162,7 +170,54 @@ or increase socketAcquisitionWarningTimeout=(millis) in the NodeHttpHandler conf }; } + /** + * Lazily starts a single shared interval for socket usage warnings. + * Replaces the per-request timer that was previously created in handle(). + * Skipped entirely when maxSockets === Infinity on both agents. + */ + private ensureSocketWarningInterval(config: ResolvedNodeHttpHandlerConfig): void { + if (this.socketWarningInterval !== undefined) { + return; + } + + const httpMax = config.httpAgent.maxSockets; + const httpsMax = config.httpsAgent.maxSockets; + + if ( + (httpMax === Infinity || httpMax === undefined) && + (httpsMax === Infinity || httpsMax === undefined) + ) { + this.socketWarningInterval = null; + return; + } + + const warningTimeout = + config.socketAcquisitionWarningTimeout ?? + (config.requestTimeout ?? 2000) + (config.connectionTimeout ?? 1000); + + this.socketWarningInterval = setInterval(() => { + this.socketWarningTimestamp = NodeHttpHandler.checkSocketUsage( + config.httpsAgent, + this.socketWarningTimestamp, + config.logger + ); + this.socketWarningTimestamp = NodeHttpHandler.checkSocketUsage( + config.httpAgent, + this.socketWarningTimestamp, + config.logger + ); + }, warningTimeout); + + if (this.socketWarningInterval.unref) { + this.socketWarningInterval.unref(); + } + } + destroy(): void { + if (this.socketWarningInterval) { + clearInterval(this.socketWarningInterval); + this.socketWarningInterval = undefined; + } this.config?.httpAgent?.destroy(); this.config?.httpsAgent?.destroy(); } @@ -175,6 +230,8 @@ or increase socketAcquisitionWarningTimeout=(millis) in the NodeHttpHandler conf this.config = await this.configProvider; } + this.ensureSocketWarningInterval(this.config); + return new Promise((_resolve, _reject) => { const config = this.config!; @@ -221,21 +278,6 @@ or increase socketAcquisitionWarningTimeout=(millis) in the NodeHttpHandler conf }); } - // If the request is taking a long time, check socket usage and potentially warn. - // This warning will be cancelled if the request resolves. - timeouts.push( - timing.setTimeout( - () => { - this.socketWarningTimestamp = NodeHttpHandler.checkSocketUsage( - agent, - this.socketWarningTimestamp, - config.logger - ); - }, - config.socketAcquisitionWarningTimeout ?? (config.requestTimeout ?? 2000) + (config.connectionTimeout ?? 1000) - ) - ); - const queryString = buildQueryString(request.query || {}); let auth = undefined; if (request.username != null || request.password != null) { @@ -342,6 +384,10 @@ or increase socketAcquisitionWarningTimeout=(millis) in the NodeHttpHandler conf updateHttpClientConfig(key: keyof NodeHttpHandlerOptions, value: NodeHttpHandlerOptions[typeof key]): void { this.config = undefined; + if (this.socketWarningInterval) { + clearInterval(this.socketWarningInterval); + } + this.socketWarningInterval = undefined; this.configProvider = this.configProvider.then((config) => { return { ...config,