From c86a02c231a9efa839238985b531e7b0cf98929e Mon Sep 17 00:00:00 2001 From: George Fu Date: Thu, 5 Sep 2024 15:07:36 -0400 Subject: [PATCH] feat(node-http-handler): defer socket event listener attachment (#1384) --- .changeset/empty-feet-shop.md | 5 ++ .../src/node-http-handler.ts | 58 ++++++++++--------- .../src/set-connection-timeout.ts | 53 +++++++++++------ .../src/set-socket-keep-alive.spec.ts | 6 +- .../src/set-socket-keep-alive.ts | 25 ++++++-- .../src/set-socket-timeout.spec.ts | 12 +++- .../src/set-socket-timeout.ts | 30 ++++++++-- 7 files changed, 128 insertions(+), 61 deletions(-) create mode 100644 .changeset/empty-feet-shop.md diff --git a/.changeset/empty-feet-shop.md b/.changeset/empty-feet-shop.md new file mode 100644 index 00000000000..8221194f659 --- /dev/null +++ b/.changeset/empty-feet-shop.md @@ -0,0 +1,5 @@ +--- +"@smithy/node-http-handler": minor +--- + +defer socket event listeners for node:http diff --git a/packages/node-http-handler/src/node-http-handler.ts b/packages/node-http-handler/src/node-http-handler.ts index cf619ab310e..e81b869e11f 100644 --- a/packages/node-http-handler/src/node-http-handler.ts +++ b/packages/node-http-handler/src/node-http-handler.ts @@ -146,19 +146,20 @@ or increase socketAcquisitionWarningTimeout=(millis) in the NodeHttpHandler conf this.config = await this.configProvider; } - let socketCheckTimeoutId: NodeJS.Timeout; - return new Promise((_resolve, _reject) => { let writeRequestBodyPromise: Promise | undefined = undefined; + + // Timeouts related to this request to clear upon completion. + const timeouts = [] as (number | NodeJS.Timeout)[]; + const resolve = async (arg: { response: HttpResponse }) => { await writeRequestBodyPromise; - // if requests are still resolving, cancel the socket usage check. - clearTimeout(socketCheckTimeoutId); + timeouts.forEach(clearTimeout); _resolve(arg); }; const reject = async (arg: unknown) => { await writeRequestBodyPromise; - clearTimeout(socketCheckTimeoutId); + timeouts.forEach(clearTimeout); _reject(arg); }; @@ -180,16 +181,18 @@ 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. - socketCheckTimeoutId = setTimeout( - () => { - this.socketWarningTimestamp = NodeHttpHandler.checkSocketUsage( - agent, - this.socketWarningTimestamp, - this.config!.logger - ); - }, - this.config.socketAcquisitionWarningTimeout ?? - (this.config.requestTimeout ?? 2000) + (this.config.connectionTimeout ?? 1000) + timeouts.push( + setTimeout( + () => { + this.socketWarningTimestamp = NodeHttpHandler.checkSocketUsage( + agent, + this.socketWarningTimestamp, + this.config!.logger + ); + }, + this.config.socketAcquisitionWarningTimeout ?? + (this.config.requestTimeout ?? 2000) + (this.config.connectionTimeout ?? 1000) + ) ); const queryString = buildQueryString(request.query || {}); @@ -237,10 +240,6 @@ or increase socketAcquisitionWarningTimeout=(millis) in the NodeHttpHandler conf } }); - // wire-up any timeout logic - setConnectionTimeout(req, reject, this.config.connectionTimeout); - setSocketTimeout(req, reject, this.config.requestTimeout); - // wire-up abort logic if (abortSignal) { const onAbort = () => { @@ -261,19 +260,26 @@ or increase socketAcquisitionWarningTimeout=(millis) in the NodeHttpHandler conf } } + // Defer registration of socket event listeners if the connection and request timeouts + // are longer than a few seconds. This avoids slowing down faster operations. + timeouts.push(setConnectionTimeout(req, reject, this.config.connectionTimeout)); + timeouts.push(setSocketTimeout(req, reject, this.config.requestTimeout)); + // Workaround for bug report in Node.js https://github.com/nodejs/node/issues/47137 const httpAgent = nodeHttpsOptions.agent; if (typeof httpAgent === "object" && "keepAlive" in httpAgent) { - setSocketKeepAlive(req, { - // @ts-expect-error keepAlive is not public on httpAgent. - keepAlive: (httpAgent as hAgent).keepAlive, - // @ts-expect-error keepAliveMsecs is not public on httpAgent. - keepAliveMsecs: (httpAgent as hAgent).keepAliveMsecs, - }); + timeouts.push( + setSocketKeepAlive(req, { + // @ts-expect-error keepAlive is not public on httpAgent. + keepAlive: (httpAgent as hAgent).keepAlive, + // @ts-expect-error keepAliveMsecs is not public on httpAgent. + keepAliveMsecs: (httpAgent as hAgent).keepAliveMsecs, + }) + ); } writeRequestBodyPromise = writeRequestBody(req, request, this.config.requestTimeout).catch((e) => { - clearTimeout(socketCheckTimeoutId); + timeouts.forEach(clearTimeout); return _reject(e); }); }); diff --git a/packages/node-http-handler/src/set-connection-timeout.ts b/packages/node-http-handler/src/set-connection-timeout.ts index 3acbeec7637..6d1a1c12972 100644 --- a/packages/node-http-handler/src/set-connection-timeout.ts +++ b/packages/node-http-handler/src/set-connection-timeout.ts @@ -1,28 +1,43 @@ import { ClientRequest } from "http"; import { Socket } from "net"; -export const setConnectionTimeout = (request: ClientRequest, reject: (err: Error) => void, timeoutInMs = 0) => { +const DEFER_EVENT_LISTENER_TIME = 1000; + +export const setConnectionTimeout = ( + request: ClientRequest, + reject: (err: Error) => void, + timeoutInMs = 0 +): NodeJS.Timeout | number => { if (!timeoutInMs) { - return; + return -1; } - // Throw a connecting timeout error unless a connection is made within time. - const timeoutId = setTimeout(() => { - request.destroy(); - reject( - Object.assign(new Error(`Socket timed out without establishing a connection within ${timeoutInMs} ms`), { - name: "TimeoutError", - }) - ); - }, timeoutInMs); + const registerTimeout = (offset: number) => { + // Throw a connecting timeout error unless a connection is made within time. + const timeoutId = setTimeout(() => { + request.destroy(); + reject( + Object.assign(new Error(`Socket timed out without establishing a connection within ${timeoutInMs} ms`), { + name: "TimeoutError", + }) + ); + }, timeoutInMs - offset); - request.on("socket", (socket: Socket) => { - if (socket.connecting) { - socket.on("connect", () => { + request.on("socket", (socket: Socket) => { + if (socket.connecting) { + socket.on("connect", () => { + clearTimeout(timeoutId); + }); + } else { clearTimeout(timeoutId); - }); - } else { - clearTimeout(timeoutId); - } - }); + } + }); + }; + + if (timeoutInMs < 2000) { + registerTimeout(0); + return 0; + } + + return setTimeout(registerTimeout.bind(null, DEFER_EVENT_LISTENER_TIME), DEFER_EVENT_LISTENER_TIME); }; diff --git a/packages/node-http-handler/src/set-socket-keep-alive.spec.ts b/packages/node-http-handler/src/set-socket-keep-alive.spec.ts index 19f44d9be43..4606c2b870e 100644 --- a/packages/node-http-handler/src/set-socket-keep-alive.spec.ts +++ b/packages/node-http-handler/src/set-socket-keep-alive.spec.ts @@ -14,7 +14,7 @@ describe("setSocketKeepAlive", () => { }); it("should set keepAlive to true", () => { - setSocketKeepAlive(request, { keepAlive: true }); + setSocketKeepAlive(request, { keepAlive: true }, 0); const setKeepAliveSpy = jest.spyOn(socket, "setKeepAlive"); request.emit("socket", socket); @@ -25,7 +25,7 @@ describe("setSocketKeepAlive", () => { it("should set keepAlive to true with custom initialDelay", () => { const initialDelay = 5 * 1000; - setSocketKeepAlive(request, { keepAlive: true, keepAliveMsecs: initialDelay }); + setSocketKeepAlive(request, { keepAlive: true, keepAliveMsecs: initialDelay }, 0); const setKeepAliveSpy = jest.spyOn(socket, "setKeepAlive"); request.emit("socket", socket); @@ -35,7 +35,7 @@ describe("setSocketKeepAlive", () => { }); it("should not set keepAlive at all when keepAlive is false", () => { - setSocketKeepAlive(request, { keepAlive: false }); + setSocketKeepAlive(request, { keepAlive: false }, 0); const setKeepAliveSpy = jest.spyOn(socket, "setKeepAlive"); request.emit("socket", socket); diff --git a/packages/node-http-handler/src/set-socket-keep-alive.ts b/packages/node-http-handler/src/set-socket-keep-alive.ts index 4ae9c079efe..31b58491d4f 100644 --- a/packages/node-http-handler/src/set-socket-keep-alive.ts +++ b/packages/node-http-handler/src/set-socket-keep-alive.ts @@ -1,16 +1,31 @@ import { ClientRequest } from "http"; +const DEFER_EVENT_LISTENER_TIME = 3000; + export interface SocketKeepAliveOptions { keepAlive: boolean; keepAliveMsecs?: number; } -export const setSocketKeepAlive = (request: ClientRequest, { keepAlive, keepAliveMsecs }: SocketKeepAliveOptions) => { +export const setSocketKeepAlive = ( + request: ClientRequest, + { keepAlive, keepAliveMsecs }: SocketKeepAliveOptions, + deferTimeMs = DEFER_EVENT_LISTENER_TIME +): NodeJS.Timeout | number => { if (keepAlive !== true) { - return; + return -1; + } + + const registerListener = () => { + request.on("socket", (socket) => { + socket.setKeepAlive(keepAlive, keepAliveMsecs || 0); + }); + }; + + if (deferTimeMs === 0) { + registerListener(); + return 0; } - request.on("socket", (socket) => { - socket.setKeepAlive(keepAlive, keepAliveMsecs || 0); - }); + return setTimeout(registerListener, deferTimeMs); }; diff --git a/packages/node-http-handler/src/set-socket-timeout.spec.ts b/packages/node-http-handler/src/set-socket-timeout.spec.ts index 8b44679af55..30fe30fd183 100644 --- a/packages/node-http-handler/src/set-socket-timeout.spec.ts +++ b/packages/node-http-handler/src/set-socket-timeout.spec.ts @@ -8,6 +8,12 @@ describe("setSocketTimeout", () => { beforeEach(() => { jest.clearAllMocks(); + jest.useFakeTimers(); + }); + + afterAll(() => { + jest.clearAllMocks(); + jest.useRealTimers(); }); it(`sets the request's timeout if provided`, () => { @@ -17,15 +23,17 @@ describe("setSocketTimeout", () => { expect(clientRequest.setTimeout).toHaveBeenLastCalledWith(100, expect.any(Function)); }); - it(`sets the request's timeout to 0 if not provided`, () => { + it(`sets the request's timeout to 0 if not provided`, async () => { setSocketTimeout(clientRequest, jest.fn()); + jest.runAllTimers(); + expect(clientRequest.setTimeout).toHaveBeenCalledTimes(1); expect(clientRequest.setTimeout).toHaveBeenLastCalledWith(0, expect.any(Function)); }); it(`destroys the request on timeout`, () => { - setSocketTimeout(clientRequest, jest.fn()); + setSocketTimeout(clientRequest, jest.fn(), 1); expect(clientRequest.destroy).not.toHaveBeenCalled(); // call setTimeout callback diff --git a/packages/node-http-handler/src/set-socket-timeout.ts b/packages/node-http-handler/src/set-socket-timeout.ts index 35a7b3c396c..78f79a4f43a 100644 --- a/packages/node-http-handler/src/set-socket-timeout.ts +++ b/packages/node-http-handler/src/set-socket-timeout.ts @@ -1,9 +1,27 @@ import { ClientRequest } from "http"; -export const setSocketTimeout = (request: ClientRequest, reject: (err: Error) => void, timeoutInMs = 0) => { - request.setTimeout(timeoutInMs, () => { - // destroy the request - request.destroy(); - reject(Object.assign(new Error(`Connection timed out after ${timeoutInMs} ms`), { name: "TimeoutError" })); - }); +const DEFER_EVENT_LISTENER_TIME = 3000; + +export const setSocketTimeout = ( + request: ClientRequest, + reject: (err: Error) => void, + timeoutInMs = 0 +): NodeJS.Timeout | number => { + const registerTimeout = (offset: number) => { + request.setTimeout(timeoutInMs - offset, () => { + // destroy the request + request.destroy(); + reject(Object.assign(new Error(`Connection timed out after ${timeoutInMs} ms`), { name: "TimeoutError" })); + }); + }; + + if (0 < timeoutInMs && timeoutInMs < 6000) { + registerTimeout(0); + return 0; + } + + return setTimeout( + registerTimeout.bind(null, timeoutInMs === 0 ? 0 : DEFER_EVENT_LISTENER_TIME), + DEFER_EVENT_LISTENER_TIME + ); };