Skip to content

Commit

Permalink
fix(node-http-handler): resolve config provider only once per NodeHtt…
Browse files Browse the repository at this point in the history 
…pHandler instance (#3545)

* fix(node-http-handler): resolve config provider only once per NodeHttpHandler instance

* fix(node-http-handler): test synchronization update
  • Loading branch information
@kuhe
kuhe committed Apr 20, 2022
1 parent c95bafd commit 8ffd6b2
Show file tree
Hide file tree
Showing 3 changed files with 93 additions and 58 deletions.
1 change: 1 addition & 0 deletions .gitignore
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ jspm_packages
.DS_Store
.vscode/launch.json

Makefile
lerna-debug.log
package-lock.json

Expand Down
124 changes: 77 additions & 47 deletions packages/node-http-handler/src/node-http-handler.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
import { AbortController } from "@aws-sdk/abort-controller";
import { HttpRequest } from "@aws-sdk/protocol-http";
import { Server as HttpServer } from "http";
import http from "http";
import { Server as HttpsServer } from "https";
import https from "https";
import http, { Server as HttpServer } from "http";
import https, { Server as HttpsServer } from "https";
import { AddressInfo } from "net";

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

describe("NodeHttpHandler", () => {
describe("constructor", () => {
describe("constructor and #handle", () => {
let hRequestSpy: jest.SpyInstance;
let hsRequestSpy: jest.SpyInstance;
const randomMaxSocket = Math.round(Math.random() * 50) + 1;
Expand All @@ -38,55 +36,87 @@ describe("NodeHttpHandler", () => {
hRequestSpy.mockRestore();
hsRequestSpy.mockRestore();
});
describe("constructor", () => {
it.each([
["empty", undefined],
["a provider", async () => {}],
])("sets keepAlive=true by default when input is %s", async (_, option) => {
const nodeHttpHandler = new NodeHttpHandler(option);
await nodeHttpHandler.handle({} as any);
expect(hRequestSpy.mock.calls[0][0]?.agent.keepAlive).toEqual(true);
});

it.each([
["empty", undefined],
["a provider", async () => {}],
])("sets keepAlive=true by default when input is %s", async (_, option) => {
const nodeHttpHandler = new NodeHttpHandler(option);
await nodeHttpHandler.handle({} as any);
expect(hRequestSpy.mock.calls[0][0]?.agent.keepAlive).toEqual(true);
});
it.each([
["empty", undefined],
["a provider", async () => {}],
])("sets maxSockets=50 by default when input is %s", async (_, option) => {
const nodeHttpHandler = new NodeHttpHandler(option);
await nodeHttpHandler.handle({} as any);
expect(hRequestSpy.mock.calls[0][0]?.agent.maxSockets).toEqual(50);
});

it.each([
["empty", undefined],
["a provider", async () => {}],
])("sets maxSockets=50 by default when input is %s", async (_, option) => {
const nodeHttpHandler = new NodeHttpHandler(option);
await nodeHttpHandler.handle({} as any);
expect(hRequestSpy.mock.calls[0][0]?.agent.maxSockets).toEqual(50);
});
it.each([
["an options hash", { httpAgent: new http.Agent({ keepAlive: false, maxSockets: randomMaxSocket }) }],
[
"a provider",
async () => ({
httpAgent: new http.Agent({ keepAlive: false, maxSockets: randomMaxSocket }),
}),
],
])("sets httpAgent when input is %s", async (_, option) => {
const nodeHttpHandler = new NodeHttpHandler(option);
await nodeHttpHandler.handle({ protocol: "http:", headers: {}, method: "GET", hostname: "localhost" } as any);
expect(hRequestSpy.mock.calls[0][0]?.agent.keepAlive).toEqual(false);
expect(hRequestSpy.mock.calls[0][0]?.agent.maxSockets).toEqual(randomMaxSocket);
});

it.each([
["an options hash", { httpAgent: new http.Agent({ keepAlive: false, maxSockets: randomMaxSocket }) }],
[
"a provider",
async () => ({
httpAgent: new http.Agent({ keepAlive: false, maxSockets: randomMaxSocket }),
}),
],
])("sets httpAgent when input is %s", async (_, option) => {
const nodeHttpHandler = new NodeHttpHandler(option);
await nodeHttpHandler.handle({ protocol: "http:", headers: {}, method: "GET", hostname: "localhost" } as any);
expect(hRequestSpy.mock.calls[0][0]?.agent.keepAlive).toEqual(false);
expect(hRequestSpy.mock.calls[0][0]?.agent.maxSockets).toEqual(randomMaxSocket);
it.each([
["an option hash", { httpsAgent: new https.Agent({ keepAlive: true, maxSockets: randomMaxSocket }) }],
[
"a provider",
async () => ({
httpsAgent: new https.Agent({ keepAlive: true, maxSockets: randomMaxSocket }),
}),
],
])("sets httpsAgent when input is %s", async (_, option) => {
const nodeHttpHandler = new NodeHttpHandler(option);
await nodeHttpHandler.handle({ protocol: "https:" } as any);
expect(hsRequestSpy.mock.calls[0][0]?.agent.keepAlive).toEqual(true);
expect(hsRequestSpy.mock.calls[0][0]?.agent.maxSockets).toEqual(randomMaxSocket);
});
});

it.each([
["an option hash", { httpsAgent: new https.Agent({ keepAlive: true, maxSockets: randomMaxSocket }) }],
[
"a provider",
async () => ({
httpsAgent: new https.Agent({ keepAlive: true, maxSockets: randomMaxSocket }),
}),
],
])("sets httpsAgent when input is %s", async (_, option) => {
const nodeHttpHandler = new NodeHttpHandler(option);
await nodeHttpHandler.handle({ protocol: "https:" } as any);
expect(hsRequestSpy.mock.calls[0][0]?.agent.keepAlive).toEqual(true);
expect(hsRequestSpy.mock.calls[0][0]?.agent.maxSockets).toEqual(randomMaxSocket);
describe("#handle", () => {
it("should only generate a single config when the config provider is async and it is not ready yet", async () => {
let providerInvokedCount = 0;
let providerResolvedCount = 0;
const slowConfigProvider = async () => {
providerInvokedCount += 1;
await new Promise((r) => setTimeout(r, 15));
providerResolvedCount += 1;
return {
connectionTimeout: 12345,
socketTimeout: 12345,
httpAgent: null,
httpsAgent: null,
};
};

const nodeHttpHandler = new NodeHttpHandler(slowConfigProvider);

const promises = Promise.all(
Array.from({ length: 20 }).map(() => nodeHttpHandler.handle({} as unknown as HttpRequest))
);

expect(providerInvokedCount).toBe(1);
expect(providerResolvedCount).toBe(0);
await promises;
expect(providerInvokedCount).toBe(1);
expect(providerResolvedCount).toBe(1);
});
});
});

describe("http", () => {
const mockHttpServer: HttpServer = createMockHttpServer().listen(54321);

Expand Down
26 changes: 15 additions & 11 deletions packages/node-http-handler/src/node-http-handler.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,18 +39,23 @@ interface ResolvedNodeHttpHandlerConfig {

export class NodeHttpHandler implements HttpHandler {
private config?: ResolvedNodeHttpHandlerConfig;
private readonly configProvider?: Provider<ResolvedNodeHttpHandlerConfig>;
private readonly configProvider: Promise<ResolvedNodeHttpHandlerConfig>;

// 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" };

constructor(options?: NodeHttpHandlerOptions | Provider<NodeHttpHandlerOptions | void>) {
if (typeof options === "function") {
this.configProvider = async () => {
return this.resolveDefaultConfig(await options());
};
} else {
this.config = this.resolveDefaultConfig(options);
}
this.configProvider = new Promise((resolve, reject) => {
if (typeof options === "function") {
options()
.then((_options) => {
resolve(this.resolveDefaultConfig(_options));
})
.catch(reject);
} else {
resolve(this.resolveDefaultConfig(options));
}
});
}

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

async handle(request: HttpRequest, { abortSignal }: HttpHandlerOptions = {}): Promise<{ response: HttpResponse }> {
if (!this.config && this.configProvider) {
// TODO: make resolving provide only resolve once at concurrent execution
this.config = await this.configProvider();
if (!this.config) {
this.config = await this.configProvider;
}
return new Promise((resolve, reject) => {
if (!this.config) {
Expand Down

0 comments on commit 8ffd6b2

Please sign in to comment.