perf: dispatch AuthMiddleware and middlewares synchronously when possible#1812
Open
pkeuter wants to merge 6 commits into
Open
perf: dispatch AuthMiddleware and middlewares synchronously when possible#1812pkeuter wants to merge 6 commits into
pkeuter wants to merge 6 commits into
Conversation
…ible AuthMiddleware was always async, so awaiting a synchronous authChecker scheduled a microtask and allocated a promise for every @Authorized field. On queries that resolve many authorized fields this dominates CPU time (profiled at ~40k authorized field resolutions costing several seconds of event-loop time, almost all promise/microtask churn). - AuthMiddleware: run the authChecker synchronously, only awaiting when the checker (or container instance) is actually promise-like. - applyMiddlewares: synchronous-capable dispatch that only awaits when a middleware, container instance or resolver returns a promise. A chain of synchronous middlewares now allocates zero promises instead of one each. - Widen MiddlewareFn/NextFn to MaybePromise<any> so synchronous middlewares are first-class. Behavior is preserved: same grant/deny logic, the same result-or-nextResult resolution and the multiple-next() guard. Auth denial now throws synchronously, which GraphQL execution handles identically to a rejected promise.
MichalLytek
approved these changes
Jun 8, 2026
MichalLytek
left a comment
MichalLytek
left a comment
Owner
There was a problem hiding this comment.
Could you also write some tests that somehow verifies that the new approach really works?
Widening NextFn to MaybePromise<any> made `return await next()` violate @typescript-eslint/return-await (next() was no longer provably a promise), failing CI on the error-logger example -- and the rule's autofix would have silently stopped that middleware from catching async errors. Only the middleware *return* type needs to allow synchronous values, so NextFn is restored to Promise<any> while MiddlewareFn keeps MaybePromise<any>. Also add tests for the two branches the synchronous-dispatch change introduced, neither covered before: - a fully synchronous middleware chain (ordering, result interception without next(), and the multiple-next() guard) in applyMiddlewares; - an asynchronous (promise-returning) authChecker in AuthMiddleware, which the existing sync-only auth tests never exercised.
Author
|
@MichalLytek quick reply! Of course, (A)I've added the tests. I noticed that this change, on large queries (thousands of authorized calls) breaks the query time in half. As you can probably see, the change has been authored by AI, but I went through it and I saw no strange things. Additionally I've tested in our own codebase. |
The common case is an `@Authorized`-only field whose sole middleware is the (now synchronous) AuthMiddleware. The general dispatcher allocates a recursive dispatchHandler plus next/finalize/invoke closures for every such field -- pure overhead when there is one function middleware. Invoke it directly with a terminal `next` instead, preserving the same `result ?? nextResult` resolution and the multiple-`next()` guard.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1812 +/- ##
==========================================
+ Coverage 95.50% 95.60% +0.09%
==========================================
Files 113 90 -23
Lines 1847 1934 +87
Branches 364 413 +49
==========================================
+ Hits 1764 1849 +85
- Misses 83 85 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Add tests for the previously-unhit branches in the sync/async-capable AuthMiddleware dispatch: an async class-checker `check`, an async container instance resolution, and denial under an unrecognized auth mode (the defensive fall-through). Brings src/helpers/auth-middleware.ts to 100% branch coverage.
src/typings/middleware.ts compiles to only the CommonJS `__esModule` marker and is never loaded at runtime (every consumer imports its types), so it can never be covered by tests. Use top-level `import type` so the imports fully elide, and drop src/typings/** from collectCoverageFrom since every declaration module there is the same.
Author
|
@MichalLytek, I've added some more tests to increase the coverage. Would you be so kind to let the workflows run again so I can see if the coverage didn't drop this time? Thanks! |
Author
|
@MichalLytek is there anything else you expect from me, or is this PR good as it is? |
MichalLytek
added
Enhancement 🆕
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AuthMiddleware was always async, so awaiting a synchronous authChecker scheduled a microtask and allocated a promise for every @Authorized field. On queries that resolve many authorized fields this dominates CPU time (profiled at ~40k authorized field resolutions costing several seconds of event-loop time, almost all promise/microtask churn).
Behavior is preserved: same grant/deny logic, the same result-or-nextResult resolution and the multiple-next() guard. Auth denial now throws synchronously, which GraphQL execution handles identically to a rejected promise.