From c1a2e1d492948d7e0ab1889fd52f144ee85cb568 Mon Sep 17 00:00:00 2001 From: AllanZhengYP Date: Fri, 29 Apr 2022 16:16:22 +0000 Subject: [PATCH] chore(node-http-handler): h2 handler accept options in provider --- .../src/node-http2-handler.spec.ts | 58 +++++++++++++------ .../src/node-http2-handler.ts | 44 ++++++++------ packages/smithy-client/src/defaults-mode.ts | 13 +++++ 3 files changed, 81 insertions(+), 34 deletions(-) diff --git a/packages/node-http-handler/src/node-http2-handler.spec.ts b/packages/node-http-handler/src/node-http2-handler.spec.ts index 2ca3159715b4..e2ab24446a4d 100644 --- a/packages/node-http-handler/src/node-http2-handler.spec.ts +++ b/packages/node-http-handler/src/node-http2-handler.spec.ts @@ -43,7 +43,12 @@ describe(NodeHttp2Handler.name, () => { mockH2Server.close(); }); - describe("without options", () => { + describe.each([ + ["undefined", undefined], + ["empty object", {}], + ["undefined provider", async () => void 0], + ["empty object provider", async () => ({})], + ])("without options in constructor parameter of %s", (_, option) => { let createdSessions!: ClientHttp2Session[]; const connectReal = http2.connect; let connectSpy!: jest.SpiedFunction; @@ -58,7 +63,7 @@ describe(NodeHttp2Handler.name, () => { return session; }); - nodeH2Handler = new NodeHttp2Handler(); + nodeH2Handler = new NodeHttp2Handler(option); }); const closeConnection = async (response: HttpResponse) => { @@ -357,36 +362,48 @@ describe(NodeHttp2Handler.name, () => { const requestTimeout = 200; describe("does not throw error when request not timed out", () => { - it("disableConcurrentStreams: false (default)", async () => { + it.each([ + ["static object", { requestTimeout }], + ["object provider", async () => ({ requestTimeout })], + ])("disableConcurrentStreams: false (default) in constructor parameter of %s", async (_, options) => { mockH2Server.removeAllListeners("request"); mockH2Server.on("request", createResponseFunctionWithDelay(mockResponse, requestTimeout - 100)); - nodeH2Handler = new NodeHttp2Handler({ requestTimeout }); + nodeH2Handler = new NodeHttp2Handler(options); await nodeH2Handler.handle(new HttpRequest(getMockReqOptions()), {}); }); - it("disableConcurrentStreams: true", async () => { + it.each([ + ["static object", { requestTimeout, disableConcurrentStreams: true }], + ["object provider", async () => ({ requestTimeout, disableConcurrentStreams: true })], + ])("disableConcurrentStreams: true in constructor parameter of %s", async (_, options) => { mockH2Server.removeAllListeners("request"); mockH2Server.on("request", createResponseFunctionWithDelay(mockResponse, requestTimeout - 100)); - nodeH2Handler = new NodeHttp2Handler({ requestTimeout, disableConcurrentStreams: true }); + nodeH2Handler = new NodeHttp2Handler(options); await nodeH2Handler.handle(new HttpRequest(getMockReqOptions()), {}); }); }); describe("throws timeoutError on requestTimeout", () => { - it("disableConcurrentStreams: false (default)", async () => { + it.each([ + ["static object", { requestTimeout }], + ["object provider", async () => ({ requestTimeout })], + ])("disableConcurrentStreams: false (default) in constructor parameter of %s", async (_, options) => { mockH2Server.removeAllListeners("request"); mockH2Server.on("request", createResponseFunctionWithDelay(mockResponse, requestTimeout + 100)); - nodeH2Handler = new NodeHttp2Handler({ requestTimeout }); + nodeH2Handler = new NodeHttp2Handler(options); await rejects(nodeH2Handler.handle(new HttpRequest(getMockReqOptions()), {}), { name: "TimeoutError", message: `Stream timed out because of no activity for ${requestTimeout} ms`, }); }); - it("disableConcurrentStreams: true", async () => { + it.each([ + ["object provider", async () => ({ requestTimeout })], + ["static object", { requestTimeout }], + ])("disableConcurrentStreams: true in constructor parameter of %s", async () => { mockH2Server.removeAllListeners("request"); mockH2Server.on("request", createResponseFunctionWithDelay(mockResponse, requestTimeout + 100)); @@ -403,8 +420,11 @@ describe(NodeHttp2Handler.name, () => { const sessionTimeout = 200; describe("destroys sessions on sessionTimeout", () => { - it("disableConcurrentStreams: false (default)", async () => { - nodeH2Handler = new NodeHttp2Handler({ sessionTimeout }); + it.each([ + ["object provider", async () => ({ sessionTimeout })], + ["static object", { sessionTimeout }], + ])("disableConcurrentStreams: false (default) in constructor parameter of %s", async (_, options) => { + nodeH2Handler = new NodeHttp2Handler(options); await nodeH2Handler.handle(new HttpRequest(getMockReqOptions()), {}); const authority = `${protocol}//${hostname}:${port}`; @@ -419,11 +439,14 @@ describe(NodeHttp2Handler.name, () => { expect(nodeH2Handler.sessionCache.get(authority).length).toStrictEqual(0); }); - it("disableConcurrentStreams: true", async () => { + it.each([ + ["object provider", async () => ({ sessionTimeout, disableConcurrentStreams: true })], + ["static object", { sessionTimeout, disableConcurrentStreams: true }], + ])("disableConcurrentStreams: true in constructor parameter of %s", async (_, options) => { let session; const authority = `${protocol}//${hostname}:${port}`; - nodeH2Handler = new NodeHttp2Handler({ sessionTimeout, disableConcurrentStreams: true }); + nodeH2Handler = new NodeHttp2Handler(options); mockH2Server.removeAllListeners("request"); mockH2Server.on("request", (request: any, response: any) => { @@ -487,11 +510,12 @@ describe(NodeHttp2Handler.name, () => { ); }); - describe("disableConcurrentStreams", () => { + describe.each([ + ["object provider", async () => ({ disableConcurrentStreams: true })], + ["static object", { disableConcurrentStreams: true }], + ])("disableConcurrentStreams in constructor parameter of %s", (_, options) => { beforeEach(() => { - nodeH2Handler = new NodeHttp2Handler({ - disableConcurrentStreams: true, - }); + nodeH2Handler = new NodeHttp2Handler(options); }); describe("number calls to http2.connect", () => { diff --git a/packages/node-http-handler/src/node-http2-handler.ts b/packages/node-http-handler/src/node-http2-handler.ts index ca7023d9177b..4fa4a36ff6b4 100644 --- a/packages/node-http-handler/src/node-http2-handler.ts +++ b/packages/node-http-handler/src/node-http2-handler.ts @@ -1,6 +1,6 @@ import { HttpHandler, HttpRequest, HttpResponse } from "@aws-sdk/protocol-http"; import { buildQueryString } from "@aws-sdk/querystring-builder"; -import { HttpHandlerOptions } from "@aws-sdk/types"; +import { HttpHandlerOptions, Provider } from "@aws-sdk/types"; import { ClientHttp2Session, connect, constants } from "http2"; import { getTransformedHeaders } from "./get-transformed-headers"; @@ -33,17 +33,24 @@ export interface NodeHttp2HandlerOptions { } export class NodeHttp2Handler implements HttpHandler { - private readonly requestTimeout?: number; - private readonly sessionTimeout?: number; - private readonly disableConcurrentStreams?: boolean; + private config?: NodeHttp2HandlerOptions; + private readonly configProvider: Promise; public readonly metadata = { handlerProtocol: "h2" }; private sessionCache: Map; - constructor({ requestTimeout, sessionTimeout, disableConcurrentStreams }: NodeHttp2HandlerOptions = {}) { - this.requestTimeout = requestTimeout; - this.sessionTimeout = sessionTimeout; - this.disableConcurrentStreams = disableConcurrentStreams; + constructor(options?: NodeHttp2HandlerOptions | Provider) { + this.configProvider = new Promise((resolve, reject) => { + if (typeof options === "function") { + options() + .then((opts) => { + resolve(opts || {}); + }) + .catch(reject); + } else { + resolve(options || {}); + } + }); this.sessionCache = new Map(); } @@ -54,7 +61,11 @@ export class NodeHttp2Handler implements HttpHandler { this.sessionCache.clear(); } - handle(request: HttpRequest, { abortSignal }: HttpHandlerOptions = {}): Promise<{ response: HttpResponse }> { + async handle(request: HttpRequest, { abortSignal }: HttpHandlerOptions = {}): Promise<{ response: HttpResponse }> { + if (!this.config) { + this.config = await this.configProvider; + } + const { requestTimeout, disableConcurrentStreams } = this.config; return new Promise((resolve, rejectOriginal) => { // It's redundant to track fulfilled because promises use the first resolution/rejection // but avoids generating unnecessary stack traces in the "close" event handler. @@ -71,10 +82,10 @@ export class NodeHttp2Handler implements HttpHandler { const { hostname, method, port, protocol, path, query } = request; const authority = `${protocol}//${hostname}${port ? `:${port}` : ""}`; - const session = this.getSession(authority, this.disableConcurrentStreams || false); + const session = this.getSession(authority, disableConcurrentStreams || false); const reject = (err: Error) => { - if (this.disableConcurrentStreams) { + if (disableConcurrentStreams) { this.destroySession(session); } fulfilled = true; @@ -100,7 +111,7 @@ export class NodeHttp2Handler implements HttpHandler { }); fulfilled = true; resolve({ response: httpResponse }); - if (this.disableConcurrentStreams) { + if (disableConcurrentStreams) { // Gracefully closes the Http2Session, allowing any existing streams to complete // on their own and preventing new Http2Stream instances from being created. session.close(); @@ -108,7 +119,6 @@ export class NodeHttp2Handler implements HttpHandler { } }); - const requestTimeout = this.requestTimeout; if (requestTimeout) { req.setTimeout(requestTimeout, () => { req.close(); @@ -141,7 +151,7 @@ export class NodeHttp2Handler implements HttpHandler { // an 'error' event will have also been emitted. req.on("close", () => { session.unref(); - if (this.disableConcurrentStreams) { + if (disableConcurrentStreams) { session.destroy(); } if (!fulfilled) { @@ -162,6 +172,7 @@ export class NodeHttp2Handler implements HttpHandler { */ private getSession(authority: string, disableConcurrentStreams: boolean): ClientHttp2Session { const sessionCache = this.sessionCache; + const existingSessions = sessionCache.get(authority) || []; // If concurrent streams are not disabled, we can use the existing session. @@ -179,9 +190,8 @@ export class NodeHttp2Handler implements HttpHandler { newSession.on("error", destroySessionCb); newSession.on("frameError", destroySessionCb); - const sessionTimeout = this.sessionTimeout; - if (sessionTimeout) { - newSession.setTimeout(sessionTimeout, destroySessionCb); + if (this.config?.sessionTimeout) { + newSession.setTimeout(this.config.sessionTimeout, destroySessionCb); } existingSessions.push(newSession); diff --git a/packages/smithy-client/src/defaults-mode.ts b/packages/smithy-client/src/defaults-mode.ts index 43f2357c7a72..c2c53f9bcf25 100644 --- a/packages/smithy-client/src/defaults-mode.ts +++ b/packages/smithy-client/src/defaults-mode.ts @@ -50,7 +50,20 @@ export type ResolvedDefaultsMode = Exclude; * @internal */ export interface DefaultsModeConfigs { + /** + * The retry mode describing how the retry strategy control the traffic flow. + */ retryMode?: string; + /** + * The maximum time in milliseconds that the connection phase of a request + * may take before the connection attempt is abandoned. + */ connectionTimeout?: number; + /** + * This timeout measures the time between when the first byte is sent over an + * established, open connection and when the last byte is received from the + * service. If the response is not received by the timeout, then the request + * is considered timed out. + */ requestTimeout?: number; }