Skip to content

Commit e4e6e4a

Browse files
kuhekuhe
committed
fix(node-http-handler): skip body write delay with externally owned http Agent
1 parent 6da0ab3 commit e4e6e4a

File tree

5 files changed

+111
-48
lines changed

5 files changed

+111
-48
lines changed

‎.changeset/ninety-comics-kneel.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@smithy/node-http-handler": patch
3+
---
4+
5+
skip body write delay when http Agent is externally owned

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

Lines changed: 64 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { HttpRequest } from "@smithy/protocol-http";
2-
import http from "http";
3-
import https from "https";
2+
import http, { request as hRequest } from "http";
3+
import https, { request as hsRequest } from "https";
44
import { afterEach, beforeEach, describe, expect, test as it, vi } from "vitest";
55

66
import { NodeHttpHandler } from "./node-http-handler";
@@ -50,9 +50,6 @@ vi.mock("https", async () => {
5050
};
5151
});
5252

53-
import { request as hRequest } from "http";
54-
import { request as hsRequest } from "https";
55-
5653
describe("NodeHttpHandler", () => {
5754
describe("constructor and #handle", () => {
5855
const randomMaxSocket = Math.round(Math.random() * 50) + 1;
@@ -262,32 +259,69 @@ describe("NodeHttpHandler", () => {
262259
expect(vi.mocked(hRequest as any).mock.calls[0][0]?.host).toEqual("host");
263260
});
264261

265-
it("creates a new http(s) Agent if the request has expect: 100-continue header", async () => {
266-
const nodeHttpHandler = new NodeHttpHandler({});
267-
{
268-
const httpRequest = {
269-
protocol: "http:",
270-
hostname: "[host]",
271-
path: "/some/path",
272-
headers: {
273-
expect: "100-continue",
262+
describe("expect 100-continue", () => {
263+
it("creates a new http(s) Agent if the request has expect: 100-continue header and agents are NodeHttpHandler-owned", async () => {
264+
const nodeHttpHandler = new NodeHttpHandler({
265+
httpAgent: {
266+
maxSockets: 25,
274267
},
275-
};
276-
await nodeHttpHandler.handle(httpRequest as any);
277-
expect(vi.mocked(hRequest as any).mock.calls[0][0]?.agent).not.toBe(
278-
(nodeHttpHandler as any).config.httpAgent
279-
);
280-
}
281-
{
282-
const httpRequest = {
283-
protocol: "http:",
284-
hostname: "[host]",
285-
path: "/some/path",
286-
headers: {},
287-
};
288-
await nodeHttpHandler.handle(httpRequest as any);
289-
expect(vi.mocked(hRequest as any).mock.calls[1][0]?.agent).toBe((nodeHttpHandler as any).config.httpAgent);
290-
}
268+
httpsAgent: {
269+
maxSockets: 25,
270+
},
271+
});
272+
{
273+
const httpRequest = {
274+
protocol: "http:",
275+
hostname: "[host]",
276+
path: "/some/path",
277+
headers: {
278+
expect: "100-continue",
279+
},
280+
};
281+
await nodeHttpHandler.handle(httpRequest as any);
282+
expect(vi.mocked(hRequest as any).mock.calls[0][0]?.agent).not.toBe(
283+
(nodeHttpHandler as any).config.httpAgent
284+
);
285+
}
286+
{
287+
const httpRequest = {
288+
protocol: "http:",
289+
hostname: "[host]",
290+
path: "/some/path",
291+
headers: {},
292+
};
293+
await nodeHttpHandler.handle(httpRequest as any);
294+
expect(vi.mocked(hRequest as any).mock.calls[1][0]?.agent).toBe((nodeHttpHandler as any).config.httpAgent);
295+
}
296+
});
297+
298+
it("does not create a new Agent if configured Agent is caller-owned (e.g. proxy), but instead skips the writeBody delay", async () => {
299+
const nodeHttpHandler = new NodeHttpHandler({
300+
httpAgent: new http.Agent(),
301+
});
302+
{
303+
const httpRequest = {
304+
protocol: "http:",
305+
hostname: "[host]",
306+
path: "/some/path",
307+
headers: {
308+
expect: "100-continue",
309+
},
310+
};
311+
await nodeHttpHandler.handle(httpRequest as any);
312+
expect(vi.mocked(hRequest as any).mock.calls[0][0]?.agent).toBe((nodeHttpHandler as any).config.httpAgent);
313+
}
314+
{
315+
const httpRequest = {
316+
protocol: "http:",
317+
hostname: "[host]",
318+
path: "/some/path",
319+
headers: {},
320+
};
321+
await nodeHttpHandler.handle(httpRequest as any);
322+
expect(vi.mocked(hRequest as any).mock.calls[1][0]?.agent).toBe((nodeHttpHandler as any).config.httpAgent);
323+
}
324+
});
291325
});
292326
});
293327
});

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

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,7 @@
11
import type { HttpHandler, HttpRequest } from "@smithy/protocol-http";
22
import { HttpResponse } from "@smithy/protocol-http";
33
import { buildQueryString } from "@smithy/querystring-builder";
4-
import type { Logger, NodeHttpHandlerOptions } from "@smithy/types";
5-
import type { HttpHandlerOptions, Provider } from "@smithy/types";
4+
import type { HttpHandlerOptions, Logger, NodeHttpHandlerOptions, Provider } from "@smithy/types";
65
import { Agent as hAgent, request as hRequest } from "http";
76
import type { RequestOptions } from "https";
87
import { Agent as hsAgent, request as hsRequest } from "https";
@@ -37,6 +36,7 @@ export class NodeHttpHandler implements HttpHandler<NodeHttpHandlerOptions> {
3736
private config?: ResolvedNodeHttpHandlerConfig;
3837
private configProvider: Promise<ResolvedNodeHttpHandlerConfig>;
3938
private socketWarningTimestamp = 0;
39+
private externalAgent = false;
4040

4141
// Node http handler is hard-coded to http/1.1: https://github.com/nodejs/node/blob/ff5664b83b89c55e4ab5d5f60068fb457f1f5872/lib/_http_server.js#L286
4242
public readonly metadata = { handlerProtocol: "http/1.1" };
@@ -145,12 +145,14 @@ or increase socketAcquisitionWarningTimeout=(millis) in the NodeHttpHandler conf
145145
throwOnRequestTimeout,
146146
httpAgent: (() => {
147147
if (httpAgent instanceof hAgent || typeof (httpAgent as hAgent)?.destroy === "function") {
148+
this.externalAgent = true;
148149
return httpAgent as hAgent;
149150
}
150151
return new hAgent({ keepAlive, maxSockets, ...httpAgent });
151152
})(),
152153
httpsAgent: (() => {
153154
if (httpsAgent instanceof hsAgent || typeof (httpsAgent as hsAgent)?.destroy === "function") {
155+
this.externalAgent = true;
154156
return httpsAgent as hsAgent;
155157
}
156158
return new hsAgent({ keepAlive, maxSockets, ...httpsAgent });
@@ -206,7 +208,7 @@ or increase socketAcquisitionWarningTimeout=(millis) in the NodeHttpHandler conf
206208
const expectContinue = (headers.Expect ?? headers.expect) === "100-continue";
207209

208210
let agent = isSSL ? config.httpsAgent : config.httpAgent;
209-
if (expectContinue) {
211+
if (expectContinue && !this.externalAgent) {
210212
// Because awaiting 100-continue desynchronizes the request and request body transmission,
211213
// such requests must be offloaded to a separate Agent instance.
212214
// 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
328330
);
329331
}
330332

331-
writeRequestBodyPromise = writeRequestBody(req, request, effectiveRequestTimeout).catch((e) => {
332-
timeouts.forEach(timing.clearTimeout);
333-
return _reject(e);
334-
});
333+
writeRequestBodyPromise = writeRequestBody(req, request, effectiveRequestTimeout, this.externalAgent).catch(
334+
(e) => {
335+
timeouts.forEach(timing.clearTimeout);
336+
return _reject(e);
337+
}
338+
);
335339
});
336340
}
337341

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

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -18,16 +18,38 @@ describe(writeRequestBody.name, () => {
1818
protocol: "https:",
1919
path: "/",
2020
};
21-
let done: (value?: unknown) => void;
22-
const promise = new Promise((r) => (done = r));
2321
setTimeout(async () => {
2422
httpRequest.emit("continue", {});
25-
done();
26-
}, 25);
27-
await writeRequestBody(httpRequest, request);
23+
}, 200);
24+
const promise = writeRequestBody(httpRequest, request);
25+
expect(httpRequest.end).not.toHaveBeenCalled();
26+
await promise;
27+
expect(httpRequest.end).toHaveBeenCalled();
28+
});
29+
30+
it("should not wait for the continue event if request has expect=100-continue but agent is external", async () => {
31+
const httpRequest = Object.assign(new EventEmitter(), {
32+
end: vi.fn(),
33+
}) as any;
34+
const request = {
35+
headers: {
36+
expect: "100-continue",
37+
},
38+
body: Buffer.from("abcd"),
39+
method: "GET",
40+
hostname: "",
41+
protocol: "https:",
42+
path: "/",
43+
};
44+
const id = setTimeout(async () => {
45+
httpRequest.emit("continue", {});
46+
}, 200);
47+
const promise = writeRequestBody(httpRequest, request, 6000, true);
2848
expect(httpRequest.end).toHaveBeenCalled();
2949
await promise;
50+
clearTimeout(id);
3051
});
52+
3153
it(
3254
"should not send the body if the request is expect=100-continue" +
3355
"but a response is received before the continue event",
@@ -47,16 +69,12 @@ describe(writeRequestBody.name, () => {
4769
protocol: "https:",
4870
path: "/",
4971
};
50-
let done: (value?: unknown) => void;
51-
const promise = new Promise((r) => (done = r));
5272
setTimeout(() => {
5373
httpRequest.emit("response", {});
54-
done();
5574
}, 25);
5675
await writeRequestBody(httpRequest, request);
5776
expect(request.body.pipe).not.toHaveBeenCalled();
5877
expect(httpRequest.end).not.toHaveBeenCalled();
59-
await promise;
6078
}
6179
);
6280

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,19 +13,21 @@ const MIN_WAIT_TIME = 6_000;
1313
* @param httpRequest - opened Node.js request.
1414
* @param request - container with the request body.
1515
* @param maxContinueTimeoutMs - time to wait for the continue event.
16+
* @param externalAgent - whether agent is owned by caller code.
1617
*/
1718
export async function writeRequestBody(
1819
httpRequest: ClientRequest | ClientHttp2Stream,
1920
request: HttpRequest,
20-
maxContinueTimeoutMs = MIN_WAIT_TIME
21+
maxContinueTimeoutMs = MIN_WAIT_TIME,
22+
externalAgent = false
2123
): Promise<void> {
2224
const headers = request.headers ?? {};
2325
const expect = headers.Expect || headers.expect;
2426

2527
let timeoutId = -1;
2628
let sendBody = true;
2729

28-
if (expect === "100-continue") {
30+
if (!externalAgent && expect === "100-continue") {
2931
sendBody = await Promise.race<boolean>([
3032
new Promise((resolve) => {
3133
// If this resolves first (wins the race), it means that at least MIN_WAIT_TIME ms

0 commit comments

Comments
 (0)