fix: StdioServerTransportProvider readiness barrier uses wrong Mono operator - #1138
Open
Debasishhh wants to merge 2 commits into
Open
Debasishhh wants to merge 2 commits into
Debasishhh wants to merge 2 commits into
Conversation
sendMessage waited on inboundReady and outboundReady (two Sinks.One<Void>) via Mono.zip, but Mono.zip completes as soon as the first of two value-less sources completes -- it never actually waits for the second. Mono.when is the correct operator for waiting on multiple completion-only signals, and does wait for both. Fixes modelcontextprotocol#303
Pins down why Mono.when is the correct operator for the sendMessage readiness barrier: verifies Mono.zip incorrectly proceeds after only one of two Mono<Void> signals fires, while Mono.when correctly waits for both.
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.
Summary
Fixes #303.
StdioMcpSessionTransport#sendMessagegates the actual write oninboundReadyandoutboundReady(twoSinks.One<Void>) both completing before proceeding:Mono.zipis the wrong operator here. It's built to combine values, and it completes (empty) as soon as the first of the zipped sources completes without a value — it does not wait for the others. SinceinboundReady/outboundReadyareMono<Void>(they only ever complete, never emit a value viatryEmitValue(null)),Mono.ziphere only ever waits for whichever of the two signals fires first, silently breaking the intended "wait for both" barrier.I verified this empirically before touching anything (both with
tryEmitEmpty()and with the exacttryEmitValue(null)this code uses):Mono.whenis the correct operator for waiting on multiple completion-only signals — it waits for all of them regardless of whether they emit a value.Fix
One-line change:
Mono.zip(...)→Mono.when(...).Test
Added
monoZipDoesNotWaitForBothVoidSignals_monoWhenDoestoStdioServerTransportProviderTests, which reproduces the exact combinator pattern used bysendMessageand asserts:Mono.zipincorrectly proceeds after only one of the twoMono<Void>signals fires.Mono.whencorrectly waits for both.I want to be upfront about scope: this is a targeted unit test of the reactive operator semantics, not a full black-box integration test driving the real threading race through
sendMessageend-to-end —inboundReady/outboundReadyare private fields on a private inner class with no injectable scheduler, so reproducing the exact race deterministically through the public API would need either reflection or new test seams, which felt like more than a one-line fix warrants. Happy to extend it if a maintainer would rather see that.Verification
./mvnw -pl mcp-core,mcp-test -am test— 1007 tests, 0 failures attributable to this change (one pre-existing flaky timeout inHttpServletStatelessIntegrationTests, unrelated HTTP transport module, confirmed to pass in isolation and onmainwithout this change)../mvnw -pl mcp-core,mcp-test -am spring-javaformat:validate— clean.Related PRs
A few prior PRs proposed the identical one-line fix but appear to have gone stale without review: #846, #981, #987. This PR adds a regression test on top of the same fix.