fix(node-http-handler): skip body write delay with externally owned http Agent (#1757)

* fix(node-http-handler): skip body write delay with externally owned http Agent

* fix(core/schema): date utils parsing

Author: kuhe

.changeset/gorgeous-buttons-relax.md

@@ -0,0 +1,5 @@
+---
+"@smithy/core": patch
+---
+
+fix schema date utils date parsing

.changeset/ninety-comics-kneel.md

@@ -0,0 +1,5 @@
+---
+"@smithy/node-http-handler": patch
+---
+
+skip body write delay when http Agent is externally owned

packages/core/src/submodules/serde/schema-serde-lib/schema-date-utils.ts

@@ -83,12 +83,18 @@ export const _parseRfc3339DateTimeWithOffset = (value: unknown): Date | undefine
   range(minutes, 0, 59);
   range(seconds, 0, 60); // leap second
 
-  const date = new Date();
-  date.setUTCFullYear(Number(yearStr), Number(monthStr) - 1, Number(dayStr));
-  date.setUTCHours(Number(hours));
-  date.setUTCMinutes(Number(minutes));
-  date.setUTCSeconds(Number(seconds));
-  date.setUTCMilliseconds(Number(ms) ? Math.round(parseFloat(`0.${ms}`) * 1000) : 0);
+  const date = new Date(
+    Date.UTC(
+      Number(yearStr),
+      Number(monthStr) - 1,
+      Number(dayStr),
+      Number(hours),
+      Number(minutes),
+      Number(seconds),
+      Number(ms) ? Math.round(parseFloat(`0.${ms}`) * 1000) : 0
+    )
+  );
+  date.setUTCFullYear(Number(yearStr));
 
   if (offsetStr.toUpperCase() != "Z") {
     const [, sign, offsetH, offsetM] = /([+-])(\d\d):(\d\d)/.exec(offsetStr) || [void 0, "+", 0, 0];
@@ -147,18 +153,21 @@ export const _parseRfc7231DateTime = (value: unknown): Date | undefined => {
   }
 
   if (year && second) {
-    const date = new Date();
-    date.setUTCFullYear(Number(year));
-    date.setUTCMonth(months.indexOf(month));
+    const timestamp = Date.UTC(
+      Number(year),
+      months.indexOf(month),
+      Number(day),
+      Number(hour),
+      Number(minute),
+      Number(second),
+      fraction ? Math.round(parseFloat(`0.${fraction}`) * 1000) : 0
+    );
     range(day, 1, 31);
-    date.setUTCDate(Number(day));
     range(hour, 0, 23);
-    date.setUTCHours(Number(hour));
     range(minute, 0, 59);
-    date.setUTCMinutes(Number(minute));
-    range(second, 0, 60); // leap second.
-    date.setUTCSeconds(Number(second));
-    date.setUTCMilliseconds(fraction ? Math.round(parseFloat(`0.${fraction}`) * 1000) : 0);
+    range(second, 0, 60); // leap second
+    const date = new Date(timestamp);
+    date.setUTCFullYear(Number(year));
     return date;
   }
 

packages/node-http-handler/src/node-http-handler.spec.ts

@@ -1,6 +1,6 @@
 import { HttpRequest } from "@smithy/protocol-http";
-import http from "http";
-import https from "https";
+import http, { request as hRequest } from "http";
+import https, { request as hsRequest } from "https";
 import { afterEach, beforeEach, describe, expect, test as it, vi } from "vitest";
 
 import { NodeHttpHandler } from "./node-http-handler";
@@ -50,9 +50,6 @@ vi.mock("https", async () => {
   };
 });
 
-import { request as hRequest } from "http";
-import { request as hsRequest } from "https";
-
 describe("NodeHttpHandler", () => {
   describe("constructor and #handle", () => {
     const randomMaxSocket = Math.round(Math.random() * 50) + 1;
@@ -262,32 +259,69 @@ describe("NodeHttpHandler", () => {
         expect(vi.mocked(hRequest as any).mock.calls[0][0]?.host).toEqual("host");
       });
 
-      it("creates a new http(s) Agent if the request has expect: 100-continue header", async () => {
-        const nodeHttpHandler = new NodeHttpHandler({});
-        {
-          const httpRequest = {
-            protocol: "http:",
-            hostname: "[host]",
-            path: "/some/path",
-            headers: {
-              expect: "100-continue",
+      describe("expect 100-continue", () => {
+        it("creates a new http(s) Agent if the request has expect: 100-continue header and agents are NodeHttpHandler-owned", async () => {
+          const nodeHttpHandler = new NodeHttpHandler({
+            httpAgent: {
+              maxSockets: 25,
             },
-          };
-          await nodeHttpHandler.handle(httpRequest as any);
-          expect(vi.mocked(hRequest as any).mock.calls[0][0]?.agent).not.toBe(
-            (nodeHttpHandler as any).config.httpAgent
-          );
-        }
-        {
-          const httpRequest = {
-            protocol: "http:",
-            hostname: "[host]",
-            path: "/some/path",
-            headers: {},
-          };
-          await nodeHttpHandler.handle(httpRequest as any);
-          expect(vi.mocked(hRequest as any).mock.calls[1][0]?.agent).toBe((nodeHttpHandler as any).config.httpAgent);
-        }
+            httpsAgent: {
+              maxSockets: 25,
+            },
+          });
+          {
+            const httpRequest = {
+              protocol: "http:",
+              hostname: "[host]",
+              path: "/some/path",
+              headers: {
+                expect: "100-continue",
+              },
+            };
+            await nodeHttpHandler.handle(httpRequest as any);
+            expect(vi.mocked(hRequest as any).mock.calls[0][0]?.agent).not.toBe(
+              (nodeHttpHandler as any).config.httpAgent
+            );
+          }
+          {
+            const httpRequest = {
+              protocol: "http:",
+              hostname: "[host]",
+              path: "/some/path",
+              headers: {},
+            };
+            await nodeHttpHandler.handle(httpRequest as any);
+            expect(vi.mocked(hRequest as any).mock.calls[1][0]?.agent).toBe((nodeHttpHandler as any).config.httpAgent);
+          }
+        });
+
+        it("does not create a new Agent if configured Agent is caller-owned (e.g. proxy), but instead skips the writeBody delay", async () => {
+          const nodeHttpHandler = new NodeHttpHandler({
+            httpAgent: new http.Agent(),
+          });
+          {
+            const httpRequest = {
+              protocol: "http:",
+              hostname: "[host]",
+              path: "/some/path",
+              headers: {
+                expect: "100-continue",
+              },
+            };
+            await nodeHttpHandler.handle(httpRequest as any);
+            expect(vi.mocked(hRequest as any).mock.calls[0][0]?.agent).toBe((nodeHttpHandler as any).config.httpAgent);
+          }
+          {
+            const httpRequest = {
+              protocol: "http:",
+              hostname: "[host]",
+              path: "/some/path",
+              headers: {},
+            };
+            await nodeHttpHandler.handle(httpRequest as any);
+            expect(vi.mocked(hRequest as any).mock.calls[1][0]?.agent).toBe((nodeHttpHandler as any).config.httpAgent);
+          }
+        });
       });
     });
   });

packages/node-http-handler/src/node-http-handler.ts

@@ -1,8 +1,7 @@
 import type { HttpHandler, HttpRequest } from "@smithy/protocol-http";
 import { HttpResponse } from "@smithy/protocol-http";
 import { buildQueryString } from "@smithy/querystring-builder";
-import type { Logger, NodeHttpHandlerOptions } from "@smithy/types";
-import type { HttpHandlerOptions, Provider } from "@smithy/types";
+import type { HttpHandlerOptions, Logger, NodeHttpHandlerOptions, Provider } from "@smithy/types";
 import { Agent as hAgent, request as hRequest } from "http";
 import type { RequestOptions } from "https";
 import { Agent as hsAgent, request as hsRequest } from "https";
@@ -37,6 +36,7 @@ export class NodeHttpHandler implements HttpHandler<NodeHttpHandlerOptions> {
   private config?: ResolvedNodeHttpHandlerConfig;
   private configProvider: Promise<ResolvedNodeHttpHandlerConfig>;
   private socketWarningTimestamp = 0;
+  private externalAgent = false;
 
   // 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" };
@@ -145,12 +145,14 @@ or increase socketAcquisitionWarningTimeout=(millis) in the NodeHttpHandler conf
       throwOnRequestTimeout,
       httpAgent: (() => {
         if (httpAgent instanceof hAgent || typeof (httpAgent as hAgent)?.destroy === "function") {
+          this.externalAgent = true;
           return httpAgent as hAgent;
         }
         return new hAgent({ keepAlive, maxSockets, ...httpAgent });
       })(),
       httpsAgent: (() => {
         if (httpsAgent instanceof hsAgent || typeof (httpsAgent as hsAgent)?.destroy === "function") {
+          this.externalAgent = true;
           return httpsAgent as hsAgent;
         }
         return new hsAgent({ keepAlive, maxSockets, ...httpsAgent });
@@ -206,7 +208,7 @@ or increase socketAcquisitionWarningTimeout=(millis) in the NodeHttpHandler conf
       const expectContinue = (headers.Expect ?? headers.expect) === "100-continue";
 
       let agent = isSSL ? config.httpsAgent : config.httpAgent;
-      if (expectContinue) {
+      if (expectContinue && !this.externalAgent) {
         // Because awaiting 100-continue desynchronizes the request and request body transmission,
         // such requests must be offloaded to a separate Agent instance.
         // Additional logic will exist on the client using this handler to determine whether to add the header at all.
@@ -328,10 +330,12 @@ or increase socketAcquisitionWarningTimeout=(millis) in the NodeHttpHandler conf
         );
       }
 
-      writeRequestBodyPromise = writeRequestBody(req, request, effectiveRequestTimeout).catch((e) => {
-        timeouts.forEach(timing.clearTimeout);
-        return _reject(e);
-      });
+      writeRequestBodyPromise = writeRequestBody(req, request, effectiveRequestTimeout, this.externalAgent).catch(
+        (e) => {
+          timeouts.forEach(timing.clearTimeout);
+          return _reject(e);
+        }
+      );
     });
   }
 

packages/node-http-handler/src/write-request-body.spec.ts

@@ -18,16 +18,38 @@ describe(writeRequestBody.name, () => {
       protocol: "https:",
       path: "/",
     };
-    let done: (value?: unknown) => void;
-    const promise = new Promise((r) => (done = r));
     setTimeout(async () => {
       httpRequest.emit("continue", {});
-      done();
-    }, 25);
-    await writeRequestBody(httpRequest, request);
+    }, 200);
+    const promise = writeRequestBody(httpRequest, request);
+    expect(httpRequest.end).not.toHaveBeenCalled();
+    await promise;
+    expect(httpRequest.end).toHaveBeenCalled();
+  });
+
+  it("should not wait for the continue event if request has expect=100-continue but agent is external", async () => {
+    const httpRequest = Object.assign(new EventEmitter(), {
+      end: vi.fn(),
+    }) as any;
+    const request = {
+      headers: {
+        expect: "100-continue",
+      },
+      body: Buffer.from("abcd"),
+      method: "GET",
+      hostname: "",
+      protocol: "https:",
+      path: "/",
+    };
+    const id = setTimeout(async () => {
+      httpRequest.emit("continue", {});
+    }, 200);
+    const promise = writeRequestBody(httpRequest, request, 6000, true);
     expect(httpRequest.end).toHaveBeenCalled();
     await promise;
+    clearTimeout(id);
   });
+
   it(
     "should not send the body if the request is expect=100-continue" +
       "but a response is received before the continue event",
@@ -47,16 +69,12 @@ describe(writeRequestBody.name, () => {
         protocol: "https:",
         path: "/",
       };
-      let done: (value?: unknown) => void;
-      const promise = new Promise((r) => (done = r));
       setTimeout(() => {
         httpRequest.emit("response", {});
-        done();
       }, 25);
       await writeRequestBody(httpRequest, request);
       expect(request.body.pipe).not.toHaveBeenCalled();
       expect(httpRequest.end).not.toHaveBeenCalled();
-      await promise;
     }
   );
 

packages/node-http-handler/src/write-request-body.ts

@@ -13,19 +13,21 @@ const MIN_WAIT_TIME = 6_000;
  * @param httpRequest - opened Node.js request.
  * @param request - container with the request body.
  * @param maxContinueTimeoutMs - time to wait for the continue event.
+ * @param externalAgent - whether agent is owned by caller code.
  */
 export async function writeRequestBody(
   httpRequest: ClientRequest | ClientHttp2Stream,
   request: HttpRequest,
-  maxContinueTimeoutMs = MIN_WAIT_TIME
+  maxContinueTimeoutMs = MIN_WAIT_TIME,
+  externalAgent = false
 ): Promise<void> {
   const headers = request.headers ?? {};
   const expect = headers.Expect || headers.expect;
 
   let timeoutId = -1;
   let sendBody = true;
 
-  if (expect === "100-continue") {
+  if (!externalAgent && expect === "100-continue") {
     sendBody = await Promise.race<boolean>([
       new Promise((resolve) => {
         // If this resolves first (wins the race), it means that at least MIN_WAIT_TIME ms