fix(node): reuse getRequestListener instead of creating one per request#2091
Open
RomKadria wants to merge 2 commits into
Open
fix(node): reuse getRequestListener instead of creating one per request#2091RomKadria wants to merge 2 commits into
RomKadria wants to merge 2 commits into
Conversation
NodeStreamableHTTPServerTransport.handleRequest() creates a new getRequestListener from @hono/node-server on every call, even though the constructor already creates one (this._requestListener). The constructor's version was unusable because it relied on a WeakMap keyed by the Web Standard Request object — but that object is created inside getRequestListener, so the WeakMap key can never be set before the callback fires. Fix: replace the WeakMap with AsyncLocalStorage to pass per-request context (authInfo, parsedBody) through to the shared listener callback. This is concurrent-safe and appropriate for this Node.js-specific middleware package. Impact: eliminates one getRequestListener allocation + closure per HTTP request. In production MCP servers handling sustained traffic, this reduces GC pressure from per-request overhead. Relates to modelcontextprotocol#2090
🦋 Changeset detectedLatest commit: a303cc7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
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
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.
Problem
NodeStreamableHTTPServerTransport.handleRequest()creates a newgetRequestListenerfrom@hono/node-serveron every call (line 173), even though the constructor already creates one (this._requestListener, line 80).The constructor's listener was intended to be reusable — it uses a
WeakMap<Request, ...>to pass per-request context. But this approach has a fundamental flaw: the Web StandardRequestobject is created insidegetRequestListenerby Hono, so the WeakMap key can never be set before the callback fires. ThehandleRequestmethod works around this by creating a new listener per call that closes overauthInfoandparsedBodydirectly.This means every request allocates a new
getRequestListenerwith its internal conversion infrastructure, contributing to GC pressure under sustained load.Fix
Replace the
WeakMapwithAsyncLocalStorageto pass per-request context (authInfo,parsedBody) through to the shared listener callback. This is:packages/middleware/node)getRequestListenerfrom the constructor is reused for all requestsBefore
After
Test results
All 74 tests pass (
packages/middleware/node).Relates to #2090