Skip to content

Commit 8ffd6b2

Browse files
kuhekuhe
authored
fix(node-http-handler): resolve config provider only once per NodeHttpHandler instance (#3545)
* fix(node-http-handler): resolve config provider only once per NodeHttpHandler instance * fix(node-http-handler): test synchronization update
1 parent c95bafd commit 8ffd6b2

File tree

3 files changed

+93
-58
lines changed

3 files changed

+93
-58
lines changed

‎.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ jspm_packages
2020
.DS_Store
2121
.vscode/launch.json
2222

23+
Makefile
2324
lerna-debug.log
2425
package-lock.json
2526

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

Lines changed: 77 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,7 @@
11
import { AbortController } from "@aws-sdk/abort-controller";
22
import { HttpRequest } from "@aws-sdk/protocol-http";
3-
import { Server as HttpServer } from "http";
4-
import http from "http";
5-
import { Server as HttpsServer } from "https";
6-
import https from "https";
3+
import http, { Server as HttpServer } from "http";
4+
import https, { Server as HttpsServer } from "https";
75
import { AddressInfo } from "net";
86

97
import { NodeHttpHandler } from "./node-http-handler";
@@ -16,7 +14,7 @@ import {
1614
} from "./server.mock";
1715

1816
describe("NodeHttpHandler", () => {
19-
describe("constructor", () => {
17+
describe("constructor and #handle", () => {
2018
let hRequestSpy: jest.SpyInstance;
2119
let hsRequestSpy: jest.SpyInstance;
2220
const randomMaxSocket = Math.round(Math.random() * 50) + 1;
@@ -38,55 +36,87 @@ describe("NodeHttpHandler", () => {
3836
hRequestSpy.mockRestore();
3937
hsRequestSpy.mockRestore();
4038
});
39+
describe("constructor", () => {
40+
it.each([
41+
["empty", undefined],
42+
["a provider", async () => {}],
43+
])("sets keepAlive=true by default when input is %s", async (_, option) => {
44+
const nodeHttpHandler = new NodeHttpHandler(option);
45+
await nodeHttpHandler.handle({} as any);
46+
expect(hRequestSpy.mock.calls[0][0]?.agent.keepAlive).toEqual(true);
47+
});
4148

42-
it.each([
43-
["empty", undefined],
44-
["a provider", async () => {}],
45-
])("sets keepAlive=true by default when input is %s", async (_, option) => {
46-
const nodeHttpHandler = new NodeHttpHandler(option);
47-
await nodeHttpHandler.handle({} as any);
48-
expect(hRequestSpy.mock.calls[0][0]?.agent.keepAlive).toEqual(true);
49-
});
49+
it.each([
50+
["empty", undefined],
51+
["a provider", async () => {}],
52+
])("sets maxSockets=50 by default when input is %s", async (_, option) => {
53+
const nodeHttpHandler = new NodeHttpHandler(option);
54+
await nodeHttpHandler.handle({} as any);
55+
expect(hRequestSpy.mock.calls[0][0]?.agent.maxSockets).toEqual(50);
56+
});
5057

51-
it.each([
52-
["empty", undefined],
53-
["a provider", async () => {}],
54-
])("sets maxSockets=50 by default when input is %s", async (_, option) => {
55-
const nodeHttpHandler = new NodeHttpHandler(option);
56-
await nodeHttpHandler.handle({} as any);
57-
expect(hRequestSpy.mock.calls[0][0]?.agent.maxSockets).toEqual(50);
58-
});
58+
it.each([
59+
["an options hash", { httpAgent: new http.Agent({ keepAlive: false, maxSockets: randomMaxSocket }) }],
60+
[
61+
"a provider",
62+
async () => ({
63+
httpAgent: new http.Agent({ keepAlive: false, maxSockets: randomMaxSocket }),
64+
}),
65+
],
66+
])("sets httpAgent when input is %s", async (_, option) => {
67+
const nodeHttpHandler = new NodeHttpHandler(option);
68+
await nodeHttpHandler.handle({ protocol: "http:", headers: {}, method: "GET", hostname: "localhost" } as any);
69+
expect(hRequestSpy.mock.calls[0][0]?.agent.keepAlive).toEqual(false);
70+
expect(hRequestSpy.mock.calls[0][0]?.agent.maxSockets).toEqual(randomMaxSocket);
71+
});
5972

60-
it.each([
61-
["an options hash", { httpAgent: new http.Agent({ keepAlive: false, maxSockets: randomMaxSocket }) }],
62-
[
63-
"a provider",
64-
async () => ({
65-
httpAgent: new http.Agent({ keepAlive: false, maxSockets: randomMaxSocket }),
66-
}),
67-
],
68-
])("sets httpAgent when input is %s", async (_, option) => {
69-
const nodeHttpHandler = new NodeHttpHandler(option);
70-
await nodeHttpHandler.handle({ protocol: "http:", headers: {}, method: "GET", hostname: "localhost" } as any);
71-
expect(hRequestSpy.mock.calls[0][0]?.agent.keepAlive).toEqual(false);
72-
expect(hRequestSpy.mock.calls[0][0]?.agent.maxSockets).toEqual(randomMaxSocket);
73+
it.each([
74+
["an option hash", { httpsAgent: new https.Agent({ keepAlive: true, maxSockets: randomMaxSocket }) }],
75+
[
76+
"a provider",
77+
async () => ({
78+
httpsAgent: new https.Agent({ keepAlive: true, maxSockets: randomMaxSocket }),
79+
}),
80+
],
81+
])("sets httpsAgent when input is %s", async (_, option) => {
82+
const nodeHttpHandler = new NodeHttpHandler(option);
83+
await nodeHttpHandler.handle({ protocol: "https:" } as any);
84+
expect(hsRequestSpy.mock.calls[0][0]?.agent.keepAlive).toEqual(true);
85+
expect(hsRequestSpy.mock.calls[0][0]?.agent.maxSockets).toEqual(randomMaxSocket);
86+
});
7387
});
7488

75-
it.each([
76-
["an option hash", { httpsAgent: new https.Agent({ keepAlive: true, maxSockets: randomMaxSocket }) }],
77-
[
78-
"a provider",
79-
async () => ({
80-
httpsAgent: new https.Agent({ keepAlive: true, maxSockets: randomMaxSocket }),
81-
}),
82-
],
83-
])("sets httpsAgent when input is %s", async (_, option) => {
84-
const nodeHttpHandler = new NodeHttpHandler(option);
85-
await nodeHttpHandler.handle({ protocol: "https:" } as any);
86-
expect(hsRequestSpy.mock.calls[0][0]?.agent.keepAlive).toEqual(true);
87-
expect(hsRequestSpy.mock.calls[0][0]?.agent.maxSockets).toEqual(randomMaxSocket);
89+
describe("#handle", () => {
90+
it("should only generate a single config when the config provider is async and it is not ready yet", async () => {
91+
let providerInvokedCount = 0;
92+
let providerResolvedCount = 0;
93+
const slowConfigProvider = async () => {
94+
providerInvokedCount += 1;
95+
await new Promise((r) => setTimeout(r, 15));
96+
providerResolvedCount += 1;
97+
return {
98+
connectionTimeout: 12345,
99+
socketTimeout: 12345,
100+
httpAgent: null,
101+
httpsAgent: null,
102+
};
103+
};
104+
105+
const nodeHttpHandler = new NodeHttpHandler(slowConfigProvider);
106+
107+
const promises = Promise.all(
108+
Array.from({ length: 20 }).map(() => nodeHttpHandler.handle({} as unknown as HttpRequest))
109+
);
110+
111+
expect(providerInvokedCount).toBe(1);
112+
expect(providerResolvedCount).toBe(0);
113+
await promises;
114+
expect(providerInvokedCount).toBe(1);
115+
expect(providerResolvedCount).toBe(1);
116+
});
88117
});
89118
});
119+
90120
describe("http", () => {
91121
const mockHttpServer: HttpServer = createMockHttpServer().listen(54321);
92122

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

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -39,18 +39,23 @@ interface ResolvedNodeHttpHandlerConfig {
3939

4040
export class NodeHttpHandler implements HttpHandler {
4141
private config?: ResolvedNodeHttpHandlerConfig;
42-
private readonly configProvider?: Provider<ResolvedNodeHttpHandlerConfig>;
42+
private readonly configProvider: Promise<ResolvedNodeHttpHandlerConfig>;
43+
4344
// Node http handler is hard-coded to http/1.1: https://github.com/nodejs/node/blob/ff5664b83b89c55e4ab5d5f60068fb457f1f5872/lib/_http_server.js#L286
4445
public readonly metadata = { handlerProtocol: "http/1.1" };
4546

4647
constructor(options?: NodeHttpHandlerOptions | Provider<NodeHttpHandlerOptions | void>) {
47-
if (typeof options === "function") {
48-
this.configProvider = async () => {
49-
return this.resolveDefaultConfig(await options());
50-
};
51-
} else {
52-
this.config = this.resolveDefaultConfig(options);
53-
}
48+
this.configProvider = new Promise((resolve, reject) => {
49+
if (typeof options === "function") {
50+
options()
51+
.then((_options) => {
52+
resolve(this.resolveDefaultConfig(_options));
53+
})
54+
.catch(reject);
55+
} else {
56+
resolve(this.resolveDefaultConfig(options));
57+
}
58+
});
5459
}
5560

5661
private resolveDefaultConfig(options?: NodeHttpHandlerOptions | void): ResolvedNodeHttpHandlerConfig {
@@ -71,9 +76,8 @@ export class NodeHttpHandler implements HttpHandler {
7176
}
7277

7378
async handle(request: HttpRequest, { abortSignal }: HttpHandlerOptions = {}): Promise<{ response: HttpResponse }> {
74-
if (!this.config && this.configProvider) {
75-
// TODO: make resolving provide only resolve once at concurrent execution
76-
this.config = await this.configProvider();
79+
if (!this.config) {
80+
this.config = await this.configProvider;
7781
}
7882
return new Promise((resolve, reject) => {
7983
if (!this.config) {

0 commit comments

Comments
 (0)