fix(stdio): process messages concurrently to prevent deadlocks#736
Open
blackwell-systems wants to merge 1 commit intomodelcontextprotocol:mainfrom
Open
fix(stdio): process messages concurrently to prevent deadlocks#736blackwell-systems wants to merge 1 commit intomodelcontextprotocol:mainfrom
blackwell-systems wants to merge 1 commit intomodelcontextprotocol:mainfrom
Conversation
StdioServerTransport processed messages sequentially in processReadBuffer: each _onMessage.invoke() call blocked the loop until the handler completed. This meant a long-running tool call blocked all subsequent messages, including pings, elicitation responses, and progress notifications. This also created deadlocks for any bidirectional communication. Launch each message handler in its own coroutine via scope.launch so handlers run concurrently. The ReadBuffer parsing remains sequential (required for correct JSON-RPC framing), but handler execution is now parallel. Updated the error-handling test to account for the async error delivery that results from concurrent dispatch. Fixes modelcontextprotocol#572 Signed-off-by: Dayna Blackwell <dayna@blackwell-systems.com>
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.
Motivation and Context
Fixes #572
StdioServerTransport.processReadBuffer()invokes each message handler sequentially:_onMessage.invoke(message)blocks the loop until the handler returns. A long-running tool call (e.g., one that waits for elicitation) blocks all subsequent messages, including pings, progress notifications, and the elicitation response itself. This creates deadlocks for any bidirectional communication pattern.As noted by @rnett in #572:
Fix
Launch each message handler in its own coroutine via
scope.launchinstead of awaiting it inline. TheReadBufferparsing remains sequential (required for correct JSON-RPC framing), but handler execution is now concurrent.This is the minimal transport-level fix. A prior attempt (#610) placed concurrency in
AbstractTransportbut was closed due to complications with transports that already handle concurrency (SSE server). This PR scopes the change toStdioServerTransportonly.Test changes
The error-handling parameterized test (
should continue processing messages after handler throws) needed a briefdelay(100)aftersecondMessageProcessed.await()because the error callback from the first message's coroutine now fires asynchronously onIODispatcherrather than synchronously in the processing loop.How Has This Been Tested?
All 17
StdioServerTransportTestcases pass, including the 3 parameterized error-handling variants.Types of changes
Checklist