diff --git a/.changeset/middleware-stack-optimization.md b/.changeset/middleware-stack-optimization.md new file mode 100644 index 00000000000..3537113861a --- /dev/null +++ b/.changeset/middleware-stack-optimization.md @@ -0,0 +1,5 @@ +--- +"@smithy/middleware-stack": patch +--- + +Optimize middleware stack performance: add `_addBulk` fast path in `cloneTo` to skip redundant validation when copying entries to empty stacks, cache `getMiddlewareList` result with mutation-based invalidation, replace `map`+`reduce` with `flatMap`, and fix `.reverse()` mutation bug in `expandRelativeMiddlewareList` that could corrupt results on repeated calls. diff --git a/packages/middleware-stack/src/MiddlewareStack.spec.ts b/packages/middleware-stack/src/MiddlewareStack.spec.ts index 28aaa4a2c6f..207604e0794 100644 --- a/packages/middleware-stack/src/MiddlewareStack.spec.ts +++ b/packages/middleware-stack/src/MiddlewareStack.spec.ts @@ -614,4 +614,194 @@ describe("MiddlewareStack", () => { expect(inner.mock.calls.length).toBe(1); }); }); + + describe("_addBulk (internal fast path)", () => { + it("should bulk-add absolute entries without validation", async () => { + const stack = constructStack(); + (stack as any)._addBulk( + [ + { middleware: getConcatMiddleware("A"), name: "A", step: "initialize", priority: "normal" }, + { middleware: getConcatMiddleware("B"), name: "B", step: "initialize", priority: "low" }, + ], + [] + ); + const inner = vi.fn(); + await stack.resolve(inner, {} as any)({ input: [] }); + expect(inner).toBeCalledWith({ input: ["A", "B"] }); + }); + + it("should bulk-add relative entries", async () => { + const stack = constructStack(); + (stack as any)._addBulk( + [{ middleware: getConcatMiddleware("A"), name: "A", step: "initialize", priority: "normal" }], + [{ middleware: getConcatMiddleware("B"), name: "B", relation: "after", toMiddleware: "A" }] + ); + const inner = vi.fn(); + await stack.resolve(inner, {} as any)({ input: [] }); + expect(inner).toBeCalledWith({ input: ["A", "B"] }); + }); + + it("should not affect source stack when bulk-added entries are modified", async () => { + const stack1 = constructStack(); + stack1.add(getConcatMiddleware("A"), { name: "A" }); + + const stack2 = constructStack(); + stack1.applyToStack(stack2); // uses cloneTo internally + + // Modify stack2 + stack2.add(getConcatMiddleware("B"), { name: "B" }); + + // stack1 should be unaffected + const inner = vi.fn(); + await stack1.resolve(inner, {} as any)({ input: [] }); + expect(inner).toBeCalledWith({ input: ["A"] }); + }); + + it("should register names so duplicate detection still works after bulk add", () => { + const stack = constructStack(); + (stack as any)._addBulk( + [{ middleware: getConcatMiddleware("A"), name: "A", step: "initialize", priority: "normal" }], + [] + ); + expect(() => stack.add(getConcatMiddleware("A2"), { name: "A" })).toThrow("Duplicate middleware name 'A'"); + }); + + it("should throw on duplicate names when bulk-adding to a non-empty stack", () => { + const stack = constructStack(); + stack.add(getConcatMiddleware("A"), { name: "A" }); + expect(() => + (stack as any)._addBulk( + [{ middleware: getConcatMiddleware("A2"), name: "A", step: "initialize", priority: "normal" }], + [] + ) + ).toThrow("Duplicate middleware name 'A'"); + }); + + it("should allow override when bulk-adding to a non-empty stack with override flag", async () => { + const stack = constructStack(); + stack.add(getConcatMiddleware("A"), { name: "A" }); + (stack as any)._addBulk( + [{ middleware: getConcatMiddleware("A-replaced"), name: "A", step: "initialize", priority: "normal", override: true }], + [] + ); + const inner = vi.fn(); + await stack.resolve(inner, {} as any)({ input: [] }); + expect(inner).toBeCalledWith({ input: ["A-replaced"] }); + }); + }); + + describe("middleware list caching", () => { + it("should return consistent results across multiple resolve calls", async () => { + const stack = constructStack(); + stack.add(getConcatMiddleware("A"), { name: "A" }); + stack.add(getConcatMiddleware("B"), { name: "B", priority: "low" }); + + const inner1 = vi.fn(); + const inner2 = vi.fn(); + await stack.resolve(inner1, {} as any)({ input: [] }); + await stack.resolve(inner2, {} as any)({ input: [] }); + expect(inner1).toBeCalledWith({ input: ["A", "B"] }); + expect(inner2).toBeCalledWith({ input: ["A", "B"] }); + }); + + it("should invalidate cache when entries are added after resolve", async () => { + const stack = constructStack(); + stack.add(getConcatMiddleware("A"), { name: "A" }); + + const inner1 = vi.fn(); + await stack.resolve(inner1, {} as any)({ input: [] }); + expect(inner1).toBeCalledWith({ input: ["A"] }); + + stack.add(getConcatMiddleware("B"), { name: "B" }); + + const inner2 = vi.fn(); + await stack.resolve(inner2, {} as any)({ input: [] }); + expect(inner2).toBeCalledWith({ input: ["A", "B"] }); + }); + + it("should invalidate cache when relative entries are added after resolve", async () => { + const stack = constructStack(); + stack.add(getConcatMiddleware("A"), { name: "A" }); + + const inner1 = vi.fn(); + await stack.resolve(inner1, {} as any)({ input: [] }); + expect(inner1).toBeCalledWith({ input: ["A"] }); + + stack.addRelativeTo(getConcatMiddleware("B"), { name: "B", relation: "after", toMiddleware: "A" }); + + const inner2 = vi.fn(); + await stack.resolve(inner2, {} as any)({ input: [] }); + expect(inner2).toBeCalledWith({ input: ["A", "B"] }); + }); + + it("should invalidate cache when entries are removed by name", async () => { + const stack = constructStack(); + stack.add(getConcatMiddleware("A"), { name: "A" }); + stack.add(getConcatMiddleware("B"), { name: "B" }); + + const inner1 = vi.fn(); + await stack.resolve(inner1, {} as any)({ input: [] }); + expect(inner1).toBeCalledWith({ input: ["A", "B"] }); + + stack.remove("B"); + + const inner2 = vi.fn(); + await stack.resolve(inner2, {} as any)({ input: [] }); + expect(inner2).toBeCalledWith({ input: ["A"] }); + }); + + it("should invalidate cache when entries are removed by tag", async () => { + const stack = constructStack(); + stack.add(getConcatMiddleware("A"), { name: "A", tags: ["keep"] }); + stack.add(getConcatMiddleware("B"), { name: "B", tags: ["remove"] }); + + const inner1 = vi.fn(); + await stack.resolve(inner1, {} as any)({ input: [] }); + expect(inner1).toBeCalledWith({ input: ["A", "B"] }); + + stack.removeByTag("remove"); + + const inner2 = vi.fn(); + await stack.resolve(inner2, {} as any)({ input: [] }); + expect(inner2).toBeCalledWith({ input: ["A"] }); + }); + + it("should invalidate cache when entries are removed by reference", async () => { + const stack = constructStack(); + const mwB = getConcatMiddleware("B"); + stack.add(getConcatMiddleware("A"), { name: "A" }); + stack.add(mwB, { name: "B" }); + + const inner1 = vi.fn(); + await stack.resolve(inner1, {} as any)({ input: [] }); + expect(inner1).toBeCalledWith({ input: ["A", "B"] }); + + stack.remove(mwB); + + const inner2 = vi.fn(); + await stack.resolve(inner2, {} as any)({ input: [] }); + expect(inner2).toBeCalledWith({ input: ["A"] }); + }); + }); + + describe("expandRelativeMiddlewareList stability", () => { + it("should produce consistent results when getMiddlewareList is called multiple times", async () => { + const stack = constructStack(); + stack.add(getConcatMiddleware("A"), { name: "A" }); + stack.addRelativeTo(getConcatMiddleware("B"), { name: "B", relation: "after", toMiddleware: "A" }); + stack.addRelativeTo(getConcatMiddleware("C"), { name: "C", relation: "after", toMiddleware: "A" }); + + // Resolve twice — previously .reverse() would mutate the after array, + // causing different ordering on the second call. + const inner1 = vi.fn(); + const inner2 = vi.fn(); + await stack.resolve(inner1, {} as any)({ input: [] }); + // Invalidate cache to force re-computation and verify no mutation occurred. + stack.add(getConcatMiddleware("_noop"), { name: "_noop", step: "deserialize", priority: "low" }); + stack.remove("_noop"); + await stack.resolve(inner2, {} as any)({ input: [] }); + expect(inner1).toBeCalledWith({ input: ["A", "C", "B"] }); + expect(inner2).toBeCalledWith({ input: ["A", "C", "B"] }); + }); + }); }); diff --git a/packages/middleware-stack/src/MiddlewareStack.ts b/packages/middleware-stack/src/MiddlewareStack.ts index 72dc9524f25..7e5d9cdf289 100644 --- a/packages/middleware-stack/src/MiddlewareStack.ts +++ b/packages/middleware-stack/src/MiddlewareStack.ts @@ -40,6 +40,11 @@ export const constructStack = (): M let relativeEntries: RelativeMiddlewareEntry[] = []; let identifyOnResolve = false; const entriesNameSet: Set = new Set(); + let cachedMiddlewareList: Array> | null = null; + + const invalidateCache = () => { + cachedMiddlewareList = null; + }; const sort = >(entries: T[]): T[] => entries.sort( @@ -63,6 +68,7 @@ export const constructStack = (): M }; absoluteEntries = absoluteEntries.filter(filterCb); relativeEntries = relativeEntries.filter(filterCb); + if (isRemoved) invalidateCache(); return isRemoved; }; @@ -80,20 +86,27 @@ export const constructStack = (): M }; absoluteEntries = absoluteEntries.filter(filterCb); relativeEntries = relativeEntries.filter(filterCb); + if (isRemoved) invalidateCache(); return isRemoved; }; const cloneTo = ( toStack: MiddlewareStack ): MiddlewareStack => { - absoluteEntries.forEach((entry) => { - //@ts-ignore - toStack.add(entry.middleware, { ...entry }); - }); - relativeEntries.forEach((entry) => { - //@ts-ignore - toStack.addRelativeTo(entry.middleware, { ...entry }); - }); + // Use internal bulk-add if available (same implementation), otherwise fall back + // to public API for cross-version compatibility. + if ("_addBulk" in toStack) { + (toStack as any)._addBulk(absoluteEntries, relativeEntries); + } else { + absoluteEntries.forEach((entry) => { + //@ts-ignore + toStack.add(entry.middleware, { ...entry }); + }); + relativeEntries.forEach((entry) => { + //@ts-ignore + toStack.addRelativeTo(entry.middleware, { ...entry }); + }); + } toStack.identifyOnResolve?.(stack.identifyOnResolve()); return toStack; }; @@ -110,13 +123,14 @@ export const constructStack = (): M } }); expandedMiddlewareList.push(from); - from.after.reverse().forEach((entry) => { + for (let i = from.after.length - 1; i >= 0; i--) { + const entry = from.after[i]; if (entry.before.length === 0 && entry.after.length === 0) { expandedMiddlewareList.push(entry); } else { expandedMiddlewareList.push(...expandRelativeMiddlewareList(entry)); } - }); + } return expandedMiddlewareList; }; @@ -125,6 +139,10 @@ export const constructStack = (): M * @param debug - don't throw, getting info only. */ const getMiddlewareList = (debug = false): Array> => { + if (!debug && cachedMiddlewareList) { + return [...cachedMiddlewareList]; + } + const normalizedAbsoluteEntries: Normalized, Input, Output>[] = []; const normalizedRelativeEntries: Normalized, Input, Output>[] = []; const normalizedEntriesNameMap: Record, Input, Output>> = {}; @@ -175,20 +193,53 @@ export const constructStack = (): M } }); - const mainChain = sort(normalizedAbsoluteEntries) - .map(expandRelativeMiddlewareList) - .reduce( - (wholeList, expandedMiddlewareList) => { - // TODO: Replace it with Array.flat(); - wholeList.push(...expandedMiddlewareList); - return wholeList; - }, - [] as MiddlewareEntry[] - ); + const mainChain = sort(normalizedAbsoluteEntries).flatMap(expandRelativeMiddlewareList); + + if (!debug) { + cachedMiddlewareList = mainChain; + } return mainChain; }; const stack: MiddlewareStack = { + /** + * @internal - Bulk-add entries from another stack. Used by cloneTo for performance. + * Skips override logic but still checks for duplicate names to preserve correctness + * when the target stack is non-empty (e.g. applyToStack on a populated stack). + */ + _addBulk: ( + absEntries: AbsoluteMiddlewareEntry[], + relEntries: RelativeMiddlewareEntry[] + ) => { + for (const entry of absEntries) { + const aliases = getAllAliases(entry.name, entry.aliases); + if (aliases.length > 0 && aliases.some((alias) => entriesNameSet.has(alias))) { + // Fall back to the full add() path which handles override and error reporting. + //@ts-ignore + stack.add(entry.middleware, { ...entry }); + continue; + } + // Shallow copy to prevent mutation of source stack's entries. + absoluteEntries.push({ ...entry }); + for (const alias of aliases) { + entriesNameSet.add(alias); + } + } + for (const entry of relEntries) { + const aliases = getAllAliases(entry.name, entry.aliases); + if (aliases.length > 0 && aliases.some((alias) => entriesNameSet.has(alias))) { + //@ts-ignore + stack.addRelativeTo(entry.middleware, { ...entry }); + continue; + } + relativeEntries.push({ ...entry }); + for (const alias of aliases) { + entriesNameSet.add(alias); + } + } + invalidateCache(); + }, + add: (middleware: MiddlewareType, options: HandlerOptions & AbsoluteLocation = {}) => { const { name, override, aliases: _aliases } = options; const entry: AbsoluteMiddlewareEntry = { @@ -225,6 +276,7 @@ export const constructStack = (): M } } absoluteEntries.push(entry); + invalidateCache(); }, addRelativeTo: (middleware: MiddlewareType, options: HandlerOptions & RelativeLocation) => { @@ -261,6 +313,7 @@ export const constructStack = (): M } } relativeEntries.push(entry); + invalidateCache(); }, clone: () => cloneTo(constructStack()), @@ -290,6 +343,7 @@ export const constructStack = (): M }; absoluteEntries = absoluteEntries.filter(filterCb); relativeEntries = relativeEntries.filter(filterCb); + if (isRemoved) invalidateCache(); return isRemoved; }, @@ -326,10 +380,9 @@ export const constructStack = (): M handler: DeserializeHandler, context: HandlerExecutionContext ): Handler => { - for (const middleware of getMiddlewareList() - .map((entry) => entry.middleware) - .reverse()) { - handler = middleware(handler as Handler, context) as any; + const middlewareList = getMiddlewareList(); + for (let i = middlewareList.length - 1; i >= 0; i--) { + handler = middlewareList[i].middleware(handler as Handler, context) as any; } if (identifyOnResolve) { console.log(stack.identify());