feat: add X-Accel-Buffering: no header to SSE responses - #1139
Open
Debasishhh wants to merge 2 commits into
Open
Debasishhh wants to merge 2 commits into
Debasishhh wants to merge 2 commits into
Conversation
Without this header, reverse proxies like Nginx buffer SSE responses by default, delaying delivery of streamed content instead of forwarding it in real time. Adds the header to all three SSE-producing response paths in the servlet-based SSE and streamable HTTP transports. Fixes modelcontextprotocol#293
Covers all three response paths that open an SSE stream: the legacy SSE transport's GET endpoint, the streamable transport's GET listening stream, and the streamable transport's POST tool-call response. Verified each test fails without the header and passes with it.
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
1 participant
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.
Summary
Fixes #293.
None of the servlet-based SSE responses set
X-Accel-Buffering: no. Without it, reverse proxies like Nginx buffer the response by default, batching what should be a real-time stream of JSON-RPC messages/notifications instead of forwarding each one as it's written.Changes
Added the header alongside the existing
Cache-Control/Connectionheaders at all three places an SSE response is opened:HttpServletSseServerTransportProvider#doGet(the legacy SSE transport's connection endpoint)HttpServletStreamableServerTransportProvider#doGet(the streamable transport's GET listening stream)HttpServletStreamableServerTransportProvider#doPost(the streamable transport's SSE response to a streamed tool call)Tests
Added one integration test per call site above, each opening a real connection through a real Tomcat instance and asserting
X-Accel-Buffering: nois present on the response. I verified all three fail without the header and pass with it (reverted the header lines locally, confirmed the exact assertion failures, then restored the fix).Verification
./mvnw -pl mcp-core -am install && ./mvnw -pl mcp-test -Dtest=HttpServletSseIntegrationTests,HttpServletStreamableIntegrationTests test— 43 + 55 tests, 0 failures../mvnw -pl mcp-core,mcp-test -am spring-javaformat:validate— clean.Related PR
#848 proposed the same header addition (2 of the 3 sites) but appears to have gone stale without review. This PR covers all three SSE response sites and adds test coverage.