From 2da43c420d7f0191fccc0652c4cdcbbee220ed80 Mon Sep 17 00:00:00 2001 From: George Fu Date: Fri, 30 Aug 2024 14:51:24 +0000 Subject: [PATCH 1/3] feat(smithy-client): add handler cache --- .changeset/tall-cameras-reply.md | 6 +++ .../middleware-stack/src/MiddlewareStack.ts | 22 +++++++-- packages/smithy-client/src/client.spec.ts | 24 ++++++++++ packages/smithy-client/src/client.ts | 46 ++++++++++++++++--- 4 files changed, 89 insertions(+), 9 deletions(-) create mode 100644 .changeset/tall-cameras-reply.md diff --git a/.changeset/tall-cameras-reply.md b/.changeset/tall-cameras-reply.md new file mode 100644 index 00000000000..6e02321c650 --- /dev/null +++ b/.changeset/tall-cameras-reply.md @@ -0,0 +1,6 @@ +--- +"@smithy/middleware-stack": minor +"@smithy/smithy-client": minor +--- + +add client handler caching diff --git a/packages/middleware-stack/src/MiddlewareStack.ts b/packages/middleware-stack/src/MiddlewareStack.ts index 52caaac9ad1..bfc6f18584b 100644 --- a/packages/middleware-stack/src/MiddlewareStack.ts +++ b/packages/middleware-stack/src/MiddlewareStack.ts @@ -32,7 +32,14 @@ const getMiddlewareNameWithAliases = (name: string | undefined, aliases: Array 0 ? ` (a.k.a. ${aliases.join(",")})` : ""}`; }; -export const constructStack = (): MiddlewareStack => { +export const constructStack = ( + stackOptions: { + /** + * Optional change listener, called with stack instance when middleware added/removed. + */ + onChange?: (middlewareStack: MiddlewareStack) => void; + } = {} +): MiddlewareStack => { let absoluteEntries: AbsoluteMiddlewareEntry[] = []; let relativeEntries: RelativeMiddlewareEntry[] = []; let identifyOnResolve = false; @@ -222,6 +229,7 @@ export const constructStack = (): M } } absoluteEntries.push(entry); + stackOptions.onChange?.(stack); }, addRelativeTo: (middleware: MiddlewareType, options: HandlerOptions & RelativeLocation) => { @@ -258,6 +266,7 @@ export const constructStack = (): M } } relativeEntries.push(entry); + stackOptions.onChange?.(stack); }, clone: () => cloneTo(constructStack()), @@ -267,8 +276,14 @@ export const constructStack = (): M }, remove: (toRemove: MiddlewareType | string): boolean => { - if (typeof toRemove === "string") return removeByName(toRemove); - else return removeByReference(toRemove); + let isRemoved: boolean; + if (typeof toRemove === "string") { + isRemoved = removeByName(toRemove); + } else { + isRemoved = removeByReference(toRemove); + } + stackOptions.onChange?.(stack); + return isRemoved; }, removeByTag: (toRemove: string): boolean => { @@ -287,6 +302,7 @@ export const constructStack = (): M }; absoluteEntries = absoluteEntries.filter(filterCb); relativeEntries = relativeEntries.filter(filterCb); + stackOptions?.onChange?.(stack); return isRemoved; }, diff --git a/packages/smithy-client/src/client.spec.ts b/packages/smithy-client/src/client.spec.ts index 56ed3944e00..6dcde8cdfe3 100644 --- a/packages/smithy-client/src/client.spec.ts +++ b/packages/smithy-client/src/client.spec.ts @@ -50,4 +50,28 @@ describe("SmithyClient", () => { }; client.send(getCommandWithOutput("foo") as any, options, callback); }); + + describe("handler caching", () => { + beforeEach(() => { + delete (client as any).handlers; + }); + + const privateAccess = () => (client as any).handlers; + + it("should cache the resolved handler", async () => { + await expect(client.send(getCommandWithOutput("foo") as any)).resolves.toEqual("foo"); + expect(privateAccess().get({}.constructor)).toBeDefined(); + }); + + it("should not cache the resolved handler if called with request options", async () => { + await expect(client.send(getCommandWithOutput("foo") as any, {})).resolves.toEqual("foo"); + expect(privateAccess()).toBeUndefined(); + }); + + it("should clear the handler cache when the middleareStack is modified", async () => { + await expect(client.send(getCommandWithOutput("foo") as any)).resolves.toEqual("foo"); + client.middlewareStack.add((n) => (a) => n(a)); + expect(privateAccess()).toBeUndefined(); + }); + }); }); diff --git a/packages/smithy-client/src/client.ts b/packages/smithy-client/src/client.ts index c0fd856b53c..1afcb911ad2 100644 --- a/packages/smithy-client/src/client.ts +++ b/packages/smithy-client/src/client.ts @@ -3,6 +3,7 @@ import { Client as IClient, Command, FetchHttpHandlerOptions, + Handler, MetadataBearer, MiddlewareStack, NodeHttpHandlerOptions, @@ -44,11 +45,21 @@ export class Client< ResolvedClientConfiguration extends SmithyResolvedConfiguration, > implements IClient { - public middlewareStack: MiddlewareStack = constructStack(); - readonly config: ResolvedClientConfiguration; - constructor(config: ResolvedClientConfiguration) { - this.config = config; + public middlewareStack: MiddlewareStack = constructStack({ + onChange: () => { + delete this.handlers; + }, + }); + /** + * May be used to cache the resolved handler function for a Command class. + */ + private handlers?: WeakMap> | undefined; + private configRef?: ResolvedClientConfiguration | undefined; + + constructor(public readonly config: ResolvedClientConfiguration) { + this.configRef = this.config; } + send( command: Command>, options?: HandlerOptions @@ -69,7 +80,29 @@ export class Client< ): Promise | void { const options = typeof optionsOrCb !== "function" ? optionsOrCb : undefined; const callback = typeof optionsOrCb === "function" ? (optionsOrCb as (err: any, data?: OutputType) => void) : cb; - const handler = command.resolveMiddleware(this.middlewareStack as any, this.config, options); + + const useHandlerCache = options === undefined && this.config === this.configRef; + + let handler: Handler; + + if (useHandlerCache) { + if (!this.handlers) { + this.handlers = new WeakMap(); + } + const handlers = this.handlers!; + + if (handlers.has(command.constructor)) { + handler = handlers.get(command.constructor)!; + } else { + handler = command.resolveMiddleware(this.middlewareStack as any, this.config, options); + handlers.set(command.constructor, handler); + } + } else { + delete this.handlers; + handler = command.resolveMiddleware(this.middlewareStack as any, this.config, options); + this.configRef = this.config; + } + if (callback) { handler(command) .then( @@ -87,6 +120,7 @@ export class Client< } destroy() { - if (this.config.requestHandler.destroy) this.config.requestHandler.destroy(); + this.config.requestHandler.destroy?.(); + delete this.handlers; } } From 78c26dd68e49124c06af21255e4fb8fac83747d4 Mon Sep 17 00:00:00 2001 From: George Fu Date: Fri, 6 Sep 2024 15:40:47 +0000 Subject: [PATCH 2/3] make handler cache configurable --- .changeset/tall-cameras-reply.md | 1 - .../middleware-stack/src/MiddlewareStack.ts | 22 ++----------- packages/smithy-client/src/client.spec.ts | 8 +---- packages/smithy-client/src/client.ts | 31 ++++++++++++------- 4 files changed, 24 insertions(+), 38 deletions(-) diff --git a/.changeset/tall-cameras-reply.md b/.changeset/tall-cameras-reply.md index 6e02321c650..f44eabadba9 100644 --- a/.changeset/tall-cameras-reply.md +++ b/.changeset/tall-cameras-reply.md @@ -1,5 +1,4 @@ --- -"@smithy/middleware-stack": minor "@smithy/smithy-client": minor --- diff --git a/packages/middleware-stack/src/MiddlewareStack.ts b/packages/middleware-stack/src/MiddlewareStack.ts index bfc6f18584b..52caaac9ad1 100644 --- a/packages/middleware-stack/src/MiddlewareStack.ts +++ b/packages/middleware-stack/src/MiddlewareStack.ts @@ -32,14 +32,7 @@ const getMiddlewareNameWithAliases = (name: string | undefined, aliases: Array 0 ? ` (a.k.a. ${aliases.join(",")})` : ""}`; }; -export const constructStack = ( - stackOptions: { - /** - * Optional change listener, called with stack instance when middleware added/removed. - */ - onChange?: (middlewareStack: MiddlewareStack) => void; - } = {} -): MiddlewareStack => { +export const constructStack = (): MiddlewareStack => { let absoluteEntries: AbsoluteMiddlewareEntry[] = []; let relativeEntries: RelativeMiddlewareEntry[] = []; let identifyOnResolve = false; @@ -229,7 +222,6 @@ export const constructStack = ( } } absoluteEntries.push(entry); - stackOptions.onChange?.(stack); }, addRelativeTo: (middleware: MiddlewareType, options: HandlerOptions & RelativeLocation) => { @@ -266,7 +258,6 @@ export const constructStack = ( } } relativeEntries.push(entry); - stackOptions.onChange?.(stack); }, clone: () => cloneTo(constructStack()), @@ -276,14 +267,8 @@ export const constructStack = ( }, remove: (toRemove: MiddlewareType | string): boolean => { - let isRemoved: boolean; - if (typeof toRemove === "string") { - isRemoved = removeByName(toRemove); - } else { - isRemoved = removeByReference(toRemove); - } - stackOptions.onChange?.(stack); - return isRemoved; + if (typeof toRemove === "string") return removeByName(toRemove); + else return removeByReference(toRemove); }, removeByTag: (toRemove: string): boolean => { @@ -302,7 +287,6 @@ export const constructStack = ( }; absoluteEntries = absoluteEntries.filter(filterCb); relativeEntries = relativeEntries.filter(filterCb); - stackOptions?.onChange?.(stack); return isRemoved; }, diff --git a/packages/smithy-client/src/client.spec.ts b/packages/smithy-client/src/client.spec.ts index 6dcde8cdfe3..ff8325945fb 100644 --- a/packages/smithy-client/src/client.spec.ts +++ b/packages/smithy-client/src/client.spec.ts @@ -9,7 +9,7 @@ describe("SmithyClient", () => { const getCommandWithOutput = (output: string) => ({ resolveMiddleware: mockResolveMiddleware, }); - const client = new Client({} as any); + const client = new Client({ cacheMiddleware: true } as any); beforeEach(() => { jest.clearAllMocks(); @@ -67,11 +67,5 @@ describe("SmithyClient", () => { await expect(client.send(getCommandWithOutput("foo") as any, {})).resolves.toEqual("foo"); expect(privateAccess()).toBeUndefined(); }); - - it("should clear the handler cache when the middleareStack is modified", async () => { - await expect(client.send(getCommandWithOutput("foo") as any)).resolves.toEqual("foo"); - client.middlewareStack.add((n) => (a) => n(a)); - expect(privateAccess()).toBeUndefined(); - }); }); }); diff --git a/packages/smithy-client/src/client.ts b/packages/smithy-client/src/client.ts index 1afcb911ad2..2ed72dfb32e 100644 --- a/packages/smithy-client/src/client.ts +++ b/packages/smithy-client/src/client.ts @@ -25,6 +25,22 @@ export interface SmithyConfiguration { * @internal */ readonly apiVersion: string; + /** + * @public + * + * Default false. + * + * When true, the client will only resolve the middleware stack once per + * Command class. This means modifying the middlewareStack of the + * command or client after requests have been made will not be + * recognized. + * + * Calling client.destroy() also clears this cache. + * + * Enable this only if needing the additional time saved (0-1ms per request) + * and not needing middleware modifications between requests. + */ + cacheMiddleware?: boolean; } /** @@ -33,6 +49,7 @@ export interface SmithyConfiguration { export type SmithyResolvedConfiguration = { requestHandler: RequestHandler; readonly apiVersion: string; + cacheMiddleware?: boolean; }; /** @@ -45,20 +62,13 @@ export class Client< ResolvedClientConfiguration extends SmithyResolvedConfiguration, > implements IClient { - public middlewareStack: MiddlewareStack = constructStack({ - onChange: () => { - delete this.handlers; - }, - }); + public middlewareStack: MiddlewareStack = constructStack(); /** * May be used to cache the resolved handler function for a Command class. */ private handlers?: WeakMap> | undefined; - private configRef?: ResolvedClientConfiguration | undefined; - constructor(public readonly config: ResolvedClientConfiguration) { - this.configRef = this.config; - } + constructor(public readonly config: ResolvedClientConfiguration) {} send( command: Command>, @@ -81,7 +91,7 @@ export class Client< const options = typeof optionsOrCb !== "function" ? optionsOrCb : undefined; const callback = typeof optionsOrCb === "function" ? (optionsOrCb as (err: any, data?: OutputType) => void) : cb; - const useHandlerCache = options === undefined && this.config === this.configRef; + const useHandlerCache = options === undefined && this.config.cacheMiddleware === true; let handler: Handler; @@ -100,7 +110,6 @@ export class Client< } else { delete this.handlers; handler = command.resolveMiddleware(this.middlewareStack as any, this.config, options); - this.configRef = this.config; } if (callback) { From 082b7f91d85dd16285bb034992a70b7a97f0250d Mon Sep 17 00:00:00 2001 From: George Fu Date: Mon, 9 Sep 2024 11:29:16 -0400 Subject: [PATCH 3/3] Update client.spec.ts --- packages/smithy-client/src/client.spec.ts | 6 ++++++ packages/smithy-client/src/client.ts | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/smithy-client/src/client.spec.ts b/packages/smithy-client/src/client.spec.ts index ff8325945fb..aca906032da 100644 --- a/packages/smithy-client/src/client.spec.ts +++ b/packages/smithy-client/src/client.spec.ts @@ -67,5 +67,11 @@ describe("SmithyClient", () => { await expect(client.send(getCommandWithOutput("foo") as any, {})).resolves.toEqual("foo"); expect(privateAccess()).toBeUndefined(); }); + + it("unsets the cache if client.destroy() is called.", async () => { + await expect(client.send(getCommandWithOutput("foo") as any)).resolves.toEqual("foo"); + client.destroy(); + expect(privateAccess()).toBeUndefined(); + }); }); }); diff --git a/packages/smithy-client/src/client.ts b/packages/smithy-client/src/client.ts index 2ed72dfb32e..aa12da841ac 100644 --- a/packages/smithy-client/src/client.ts +++ b/packages/smithy-client/src/client.ts @@ -129,7 +129,7 @@ export class Client< } destroy() { - this.config.requestHandler.destroy?.(); + this.config?.requestHandler?.destroy?.(); delete this.handlers; } }