fix(node-http-handler): undeprecate socket timeout (#1745)

* fix(node-http-handler): undeprecate socket timeout

* set consistent error name with other timeouts

Author: kuhe

.changeset/hot-actors-roll.md

@@ -0,0 +1,6 @@
+---
+"@smithy/node-http-handler": minor
+"@smithy/types": minor
+---
+
+undeprecate socketTimeout for node:https requests

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([

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;

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",
+          }
+        )
       );
     });
 

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);
 

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();
+    });
+  });
+});

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;
+};

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" }
+      )
     );
   });
 });

packages/node-http-handler/src/set-socket-timeout.ts

@@ -1,20 +1,26 @@
 import type { ClientRequest } from "http";
 
-import { DEFAULT_REQUEST_TIMEOUT } from "./node-http-handler";
 import { timing } from "./timing";
 
 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) {

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.
    */