Fix: Move receive() outside repeat loop to fix stream consumption #170
+154
−68
Conversation
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
The Client was calling connection.receive() inside a repeat loop, which meant it was trying to iterate the same stream multiple times. Async streams can only be iterated once. This fix moves the receive() call outside the loop so the stream is obtained once and iterated continuously. This fixes the issue where ProcessTransport (and potentially other transports) would yield messages but the Client would not consume them.
CRITICAL BUG FIX: The message handling Task was created but never started executing because connect() immediately called initialize() which blocked waiting for a response. Classic deadlock: - Task created (line 181) but not yet started - initialize() called immediately (line 228) - initialize() calls send() which waits for response - Response needs to be consumed by Task - But Task hasn't started yet because we're blocking! Solution: Add Task.yield() after creating Task to allow it to start before calling initialize(). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Task.yield() alone wasn't enough to let the message handling Task start consuming from the stream. Need actual time delay to ensure Task begins execution before initialize() is called. Changed from Task.yield() to Task.sleep(0.1s) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
All await logger?.debug() calls were doing nothing because logger is optional and may be nil. Added NSLog() calls to actually see what's happening in the console. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Added NSLog before and after transport.connect() to determine if the hang is in the transport or elsewhere. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
NSLog was not in scope - needed to import it explicitly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Track the flow through initialization to see if send() is called. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The logger property is a computed property with 'get async' that crosses actor boundaries (await connection?.logger). Calling 'await logger?.debug()' in connect() was causing a 10-second hang before _initialize() could execute. Removed problematic logger calls outside the Task to allow connect() to proceed.
Removed await logger?.debug() calls in the Task before the for-await loop to prevent actor boundary crossing delays that could cause the loop to miss yielded data.
Testing if calling next() explicitly on the stream iterator works, or if the issue is specifically with the for-await syntax.
The task cannot throw, so need to catch the error from iterator.next()
The manual next() call proved the stream works (returned 198 bytes). Now removing the debug code to let the actual for-await loop consume messages properly.
…om response handlers - Removed await logger?.trace() from handleResponse() (lines 689-692) - Removed await logger?.trace() from handleMessage() (lines 716-719) - Removed await logger?.trace() from handleBatchResponse() (lines 762-763) These calls were causing 10-second actor boundary crossing delays when processing tool call responses, causing callTool() to appear to hang. Replaced with NSLog() for immediate, non-blocking logging. This fixes the final blocker preventing MCP tool execution from working.
…ssing loop Removed await logger?.debug() at line 207 in the for-await message loop. This was causing delays when processing every incoming message, including tool call responses. Line 207 was in the hot path - executed for EVERY message received.
… error paths Removed await logger?.warning() at line 223 (unexpected message handling) Removed await logger?.error() at line 232 (error handling) These calls were in the hot path of the message processing loop and would block whenever an error or unexpected message occurred, which could be happening during tool call responses. Replaced with NSLog() for immediate, non-blocking error logging.
Added NSLog tracing throughout callTool() and send() to identify exact hang point in tool execution flow. Debugging points: - callTool() entry/exit - send() entry and continuation flow - Task creation inside continuation - addPendingRequest() call - connection.send() call and result 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
NSLog debug statement was using wrong property name. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…cking logger calls from ProcessTransport stderr + Add 30s timeout to Client.send() Bug modelcontextprotocol#14: ProcessTransport.logStderr() had 2 blocking logger calls - logger.warning() and logger.error() cause actor isolation delays - Replaced with NSLog for synchronous stderr logging Bug modelcontextprotocol#15: Client.send() had no timeout mechanism - Requests would hang indefinitely if MCP server didn't respond - Added 30-second timeout using withThrowingTaskGroup - Properly cleans up pending requests on timeout - Logs timeout with clear error message These fixes allow us to: 1. See stderr output from MCP servers without delays 2. Detect and recover from hung MCP server processes 3. Provide better error messages to users when tools fail 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The removePendingRequest() method returns AnyPendingRequest? which is not Sendable. This caused errors when calling from Task context in the timeout wrapper. Solution: Added tryRemovePendingRequest() that returns Bool instead, which is Sendable and allows us to check if removal succeeded across actor boundaries. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
MCP servers cannot handle escaped forward slashes (\/) in paths. Changed JSONEncoder to use .withoutEscapingSlashes formatting. This fixes tool execution timeouts where tools/call requests were sent but servers never responded due to malformed path arguments. Bug: Path '/Users/...' was encoded as '\/Users\/...' causing 30s timeout 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
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.
The Client was calling connection.receive() inside a repeat loop, which meant it was trying to iterate the same stream multiple times. Async streams can only be iterated once. This fix moves the receive() call outside the loop so the stream is obtained once and iterated continuously.
This fixes the issue where ProcessTransport (and potentially other transports) would yield messages but the Client would not consume them.
Motivation and Context
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context