diff --git a/.changeset/hot-actors-roll.md b/.changeset/hot-actors-roll.md new file mode 100644 index 00000000000..61c7fa618d1 --- /dev/null +++ b/.changeset/hot-actors-roll.md @@ -0,0 +1,6 @@ +--- +"@smithy/node-http-handler": minor +"@smithy/types": minor +--- + +undeprecate socketTimeout for node:https requests 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 5f219a82db4..0e4c4a4160a 100644 --- a/packages/node-http-handler/src/node-http-handler.spec.ts +++ b/packages/node-http-handler/src/node-http-handler.spec.ts @@ -5,6 +5,7 @@ import { afterEach, beforeEach, describe, expect, test as it, vi } from "vitest" import { NodeHttpHandler } from "./node-http-handler"; import * as setConnectionTimeoutModule from "./set-connection-timeout"; +import * as setRequestTimeoutModule from "./set-request-timeout"; import * as setSocketTimeoutModule from "./set-socket-timeout"; import { timing } from "./timing"; @@ -57,6 +58,7 @@ describe("NodeHttpHandler", () => { const randomMaxSocket = Math.round(Math.random() * 50) + 1; const randomSocketAcquisitionWarningTimeout = Math.round(Math.random() * 10000) + 1; const randomConnectionTimeout = Math.round(Math.random() * 10000) + 1; + const randomSocketTimeout = Math.round(Math.random() * 10000) + 1; const randomRequestTimeout = Math.round(Math.random() * 10000) + 1; beforeEach(() => {}); @@ -140,10 +142,25 @@ describe("NodeHttpHandler", () => { }), ], ])("sets requestTimeout correctly when input is %s", async (_, option) => { + vi.spyOn(setRequestTimeoutModule, "setRequestTimeout"); + const nodeHttpHandler = new NodeHttpHandler(option); + await nodeHttpHandler.handle({} as any); + expect(vi.mocked(setRequestTimeoutModule.setRequestTimeout).mock.calls[0][2]).toBe(randomRequestTimeout); + }); + + it.each([ + ["an options hash", { socketTimeout: randomSocketTimeout }], + [ + "a provider", + async () => ({ + socketTimeout: randomSocketTimeout, + }), + ], + ])("sets socketTimeout correctly when input is %s", async (_, option) => { vi.spyOn(setSocketTimeoutModule, "setSocketTimeout"); const nodeHttpHandler = new NodeHttpHandler(option); await nodeHttpHandler.handle({} as any); - expect(vi.mocked(setSocketTimeoutModule.setSocketTimeout).mock.calls[0][2]).toBe(randomRequestTimeout); + expect(vi.mocked(setSocketTimeoutModule.setSocketTimeout).mock.calls[0][2]).toBe(randomSocketTimeout); }); it.each([ diff --git a/packages/node-http-handler/src/node-http-handler.ts b/packages/node-http-handler/src/node-http-handler.ts index adad3b87bae..255049dd91d 100644 --- a/packages/node-http-handler/src/node-http-handler.ts +++ b/packages/node-http-handler/src/node-http-handler.ts @@ -10,6 +10,7 @@ import { Agent as hsAgent, request as hsRequest } from "https"; import { NODEJS_TIMEOUT_ERROR_CODES } from "./constants"; import { getTransformedHeaders } from "./get-transformed-headers"; import { setConnectionTimeout } from "./set-connection-timeout"; +import { setRequestTimeout } from "./set-request-timeout"; import { setSocketKeepAlive } from "./set-socket-keep-alive"; import { setSocketTimeout } from "./set-socket-timeout"; import { timing } from "./timing"; @@ -124,15 +125,24 @@ or increase socketAcquisitionWarningTimeout=(millis) in the NodeHttpHandler conf } private resolveDefaultConfig(options?: NodeHttpHandlerOptions | void): ResolvedNodeHttpHandlerConfig { - const { requestTimeout, connectionTimeout, socketTimeout, socketAcquisitionWarningTimeout, httpAgent, httpsAgent } = - options || {}; + const { + requestTimeout, + connectionTimeout, + socketTimeout, + socketAcquisitionWarningTimeout, + httpAgent, + httpsAgent, + throwOnRequestTimeout, + } = options || {}; const keepAlive = true; const maxSockets = 50; return { connectionTimeout, - requestTimeout: requestTimeout ?? socketTimeout, + requestTimeout, + socketTimeout, socketAcquisitionWarningTimeout, + throwOnRequestTimeout, httpAgent: (() => { if (httpAgent instanceof hAgent || typeof (httpAgent as hAgent)?.destroy === "function") { return httpAgent as hAgent; @@ -288,7 +298,16 @@ or increase socketAcquisitionWarningTimeout=(millis) in the NodeHttpHandler conf // are longer than a few seconds. This avoids slowing down faster operations. const effectiveRequestTimeout = requestTimeout ?? this.config.requestTimeout; timeouts.push(setConnectionTimeout(req, reject, this.config.connectionTimeout)); - timeouts.push(setSocketTimeout(req, reject, effectiveRequestTimeout)); + timeouts.push( + setRequestTimeout( + req, + reject, + effectiveRequestTimeout, + this.config.throwOnRequestTimeout, + this.config.logger ?? console + ) + ); + timeouts.push(setSocketTimeout(req, reject, this.config.socketTimeout)); // Workaround for bug report in Node.js https://github.com/nodejs/node/issues/47137 const httpAgent = nodeHttpsOptions.agent; diff --git a/packages/node-http-handler/src/set-connection-timeout.spec.ts b/packages/node-http-handler/src/set-connection-timeout.spec.ts index 6d226fc5a78..517ba29d008 100644 --- a/packages/node-http-handler/src/set-connection-timeout.spec.ts +++ b/packages/node-http-handler/src/set-connection-timeout.spec.ts @@ -78,9 +78,14 @@ describe("setConnectionTimeout", () => { expect(clientRequest.destroy).toHaveBeenCalledTimes(1); expect(reject).toHaveBeenCalledTimes(1); expect(reject).toHaveBeenCalledWith( - Object.assign(new Error(`Socket timed out without establishing a connection within ${timeoutInMs} ms`), { - name: "TimeoutError", - }) + Object.assign( + new Error( + `@smithy/node-http-handler - the request socket did not establish a connection with the server within the configured timeout of ${timeoutInMs} ms.` + ), + { + name: "TimeoutError", + } + ) ); }); diff --git a/packages/node-http-handler/src/set-connection-timeout.ts b/packages/node-http-handler/src/set-connection-timeout.ts index 05211a0155e..974c9f332e7 100644 --- a/packages/node-http-handler/src/set-connection-timeout.ts +++ b/packages/node-http-handler/src/set-connection-timeout.ts @@ -18,9 +18,14 @@ export const setConnectionTimeout = ( const timeoutId = timing.setTimeout(() => { request.destroy(); reject( - Object.assign(new Error(`Socket timed out without establishing a connection within ${timeoutInMs} ms`), { - name: "TimeoutError", - }) + Object.assign( + new Error( + `@smithy/node-http-handler - the request socket did not establish a connection with the server within the configured timeout of ${timeoutInMs} ms.` + ), + { + name: "TimeoutError", + } + ) ); }, timeoutInMs - offset); diff --git a/packages/node-http-handler/src/set-request-timeout.spec.ts b/packages/node-http-handler/src/set-request-timeout.spec.ts new file mode 100644 index 00000000000..e386efcab32 --- /dev/null +++ b/packages/node-http-handler/src/set-request-timeout.spec.ts @@ -0,0 +1,58 @@ +import { beforeEach, describe, expect, test as it, vi } from "vitest"; + +import { setRequestTimeout } from "./set-request-timeout"; + +describe("setRequestTimeout", () => { + const reject = vi.fn(); + const clientRequest: any = { + destroy: vi.fn(), + }; + + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("returns -1 if no timeout is given", () => { + { + const id = setRequestTimeout(clientRequest, reject, 0); + expect(id).toEqual(-1); + } + { + const id = setRequestTimeout(clientRequest, reject, undefined); + expect(id).toEqual(-1); + } + }); + + describe("when timeout is provided", () => { + it("rejects after the timeout", async () => { + setRequestTimeout(clientRequest, reject, 1, true); + await new Promise((r) => setTimeout(r, 2)); + expect(reject).toHaveBeenCalledWith( + Object.assign( + new Error( + `@smithy/node-http-handler - [ERROR] a request has exceeded the configured ${1} ms requestTimeout.` + ), + { + name: "TimeoutError", + code: "ETIMEDOUT", + } + ) + ); + expect(clientRequest.destroy).toHaveBeenCalled(); + }); + + it("logs a warning", async () => { + const logger = { + ...console, + warn: vi.fn(), + }; + setRequestTimeout(clientRequest, reject, 1, false, logger); + await new Promise((r) => setTimeout(r, 2)); + expect(logger.warn).toHaveBeenCalledWith( + `@smithy/node-http-handler - [WARN] a request has exceeded the configured ${1} ms requestTimeout.` + + ` Init client requestHandler with throwOnRequestTimeout=true to turn this into an error.` + ); + expect(clientRequest.destroy).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/packages/node-http-handler/src/set-request-timeout.ts b/packages/node-http-handler/src/set-request-timeout.ts new file mode 100644 index 00000000000..f197b6e23e2 --- /dev/null +++ b/packages/node-http-handler/src/set-request-timeout.ts @@ -0,0 +1,35 @@ +import type { Logger } from "@smithy/types"; +import type { ClientRequest } from "http"; + +import { timing } from "./timing"; + +/** + * @internal + */ +export const setRequestTimeout = ( + req: ClientRequest, + reject: (err: Error) => void, + timeoutInMs = 0, + throwOnRequestTimeout?: boolean, + logger?: Logger +) => { + if (timeoutInMs) { + return timing.setTimeout(() => { + let msg = `@smithy/node-http-handler - [${ + throwOnRequestTimeout ? "ERROR" : "WARN" + }] a request has exceeded the configured ${timeoutInMs} ms requestTimeout.`; + if (throwOnRequestTimeout) { + const error = Object.assign(new Error(msg), { + name: "TimeoutError", + code: "ETIMEDOUT", + }); + req.destroy(error); + reject(error); + } else { + msg += ` Init client requestHandler with throwOnRequestTimeout=true to turn this into an error.`; + logger?.warn?.(msg); + } + }, timeoutInMs); + } + return -1; +}; 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 960531f07aa..a1aa61e2e10 100644 --- a/packages/node-http-handler/src/set-socket-timeout.spec.ts +++ b/packages/node-http-handler/src/set-socket-timeout.spec.ts @@ -90,7 +90,12 @@ describe("setSocketTimeout", () => { clientRequest.setTimeout.mock.calls[0][1](); expect(reject).toHaveBeenCalledTimes(1); expect(reject).toHaveBeenCalledWith( - Object.assign(new Error(`Connection timed out after ${timeoutInMs} ms`), { name: "TimeoutError" }) + Object.assign( + new Error( + `@smithy/node-http-handler - the request socket timed out after ${timeoutInMs} ms of inactivity (configured by client requestHandler).` + ), + { name: "TimeoutError" } + ) ); }); }); diff --git a/packages/node-http-handler/src/set-socket-timeout.ts b/packages/node-http-handler/src/set-socket-timeout.ts index 02e0ef104df..c05516eb95f 100644 --- a/packages/node-http-handler/src/set-socket-timeout.ts +++ b/packages/node-http-handler/src/set-socket-timeout.ts @@ -1,6 +1,5 @@ import type { ClientRequest } from "http"; -import { DEFAULT_REQUEST_TIMEOUT } from "./node-http-handler"; import { timing } from "./timing"; const DEFER_EVENT_LISTENER_TIME = 3000; @@ -8,13 +7,20 @@ const DEFER_EVENT_LISTENER_TIME = 3000; export const setSocketTimeout = ( request: ClientRequest, reject: (err: Error) => void, - timeoutInMs = DEFAULT_REQUEST_TIMEOUT + timeoutInMs = 0 ): NodeJS.Timeout | number => { const registerTimeout = (offset: number) => { const timeout = timeoutInMs - offset; const onTimeout = () => { request.destroy(); - reject(Object.assign(new Error(`Connection timed out after ${timeoutInMs} ms`), { name: "TimeoutError" })); + reject( + Object.assign( + new Error( + `@smithy/node-http-handler - the request socket timed out after ${timeoutInMs} ms of inactivity (configured by client requestHandler).` + ), + { name: "TimeoutError" } + ) + ); }; if (request.socket) { diff --git a/packages/types/src/http/httpHandlerInitialization.ts b/packages/types/src/http/httpHandlerInitialization.ts index 0c7400f4575..2807e73c851 100644 --- a/packages/types/src/http/httpHandlerInitialization.ts +++ b/packages/types/src/http/httpHandlerInitialization.ts @@ -26,35 +26,43 @@ export interface NodeHttpHandlerOptions { /** * The maximum time in milliseconds that the connection phase of a request * may take before the connection attempt is abandoned. - * * Defaults to 0, which disables the timeout. */ connectionTimeout?: number; /** - * The number of milliseconds a request can take before automatically being terminated. + * The maximum number of milliseconds request & response should take. * Defaults to 0, which disables the timeout. + * + * If exceeded, a warning will be emitted unless throwOnRequestTimeout=true, + * in which case a TimeoutError will be thrown. */ requestTimeout?: number; /** - * Delay before the NodeHttpHandler checks for socket exhaustion, - * and emits a warning if the active sockets and enqueued request count is greater than - * 2x the maxSockets count. - * - * Defaults to connectionTimeout + requestTimeout or 3000ms if those are not set. + * Because requestTimeout was for a long time incorrectly being set as a socket idle timeout, + * users must also opt-in for request timeout thrown errors. + * Without this setting, a breach of the request timeout will be logged as a warning. */ - socketAcquisitionWarningTimeout?: number; + throwOnRequestTimeout?: boolean; /** - * This field is deprecated, and requestTimeout should be used instead. * The maximum time in milliseconds that a socket may remain idle before it - * is closed. + * is closed. Defaults to 0, which means no maximum. * - * @deprecated Use {@link requestTimeout} + * This does not affect the server, which may still close the connection due to an idle socket. */ socketTimeout?: number; + /** + * Delay before the NodeHttpHandler checks for socket exhaustion, + * and emits a warning if the active sockets and enqueued request count is greater than + * 2x the maxSockets count. + * + * Defaults to connectionTimeout + requestTimeout or 3000ms if those are not set. + */ + socketAcquisitionWarningTimeout?: number; + /** * You can pass http.Agent or its constructor options. */