fix: SSE client hangs after 30 seconds #88
Conversation
WalkthroughThis pull request simplifies the SSE client setup by removing the timeout mechanism previously used to terminate read operations. The changes include removing the Changes
Possibly related PRs
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/sse.go (1)
1-578: Consider updating documentation to reflect the behavior change.The removal of the timeout mechanism changes how the SSE client behaves. Consider adding a comment in the
readSSEmethod or in the struct documentation explaining that the client now relies on thedonechannel for termination rather than a timeout, and that users can implement their own timeout using context if needed.// readSSE continuously reads the SSE stream and processes events. -// It runs until the connection is closed or an error occurs. +// It runs until the connection is closed, an error occurs, or the done channel is closed. +// Users can implement their own timeout by using a context with timeout when calling Start() +// and other methods, rather than relying on a global timeout for the SSE connection. func (c *SSEMCPClient) readSSE(reader io.ReadCloser) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/sse.go(3 hunks)
🔇 Additional comments (4)
client/sse.go (4)
25-39: Removal of timeout field aligns with PR objectives.The removal of the
sseReadTimeoutfield from theSSEMCPClientstruct is a positive change that addresses the core issue. By eliminating this field, we prevent the SSE client from hanging after 30 seconds, allowing for persistent connections as expected in SSE architecture.
57-64: Simplified client initialization without timeout configuration.The removal of the timeout initialization in the
NewSSEMCPClientfunction is consistent with the changes to the struct. This correctly eliminates the default 30-second timeout that was causing the client to hang.
123-127: Fixed SSE connection handling with proper termination approach.The
readSSEmethod now correctly checks for the closure of thedonechannel instead of using a context with timeout. This change ensures that the SSE reading loop continues indefinitely until explicitly terminated, which is the expected behavior for SSE connections.
41-47:✅ Verification successful
Verify WithSSEReadTimeout function was completely removed.
I notice that while the
sseReadTimeoutfield was removed from the struct and its initialization, we should verify that theWithSSEReadTimeoutfunction mentioned in the summary was actually removed from the codebase.
🏁 Script executed:
#!/bin/bash # Check if WithSSEReadTimeout function exists in the current code echo "Checking for WithSSEReadTimeout function in client/sse.go:" grep -n "WithSSEReadTimeout" client/sse.go || echo "Function not found (good)" # Check if there are any references to this function elsewhere in the codebase echo -e "\nChecking for references to WithSSEReadTimeout in other files:" rg "WithSSEReadTimeout" --type go --files-with-matches || echo "No references found (good)"Length of output: 487
Verification Confirmed:
WithSSEReadTimeoutRemoval is CompleteThe verification scripts confirm that the
WithSSEReadTimeoutfunction is not present inclient/sse.goand no references to it exist elsewhere in the codebase. Please proceed knowing that all related removal steps have been properly implemented.
|
I am in favor of removing this in favor of a more Go aligned method of dealing with timeouts. However, maybe it would be good to add a test or, at a minimum, some documentation about how someone could use context to handle timeouts in the README |
|
Are there any updates? The client is borken, should be fixed ASAP. |
If it helps I have a tag with the fix that you can replace in go.mod pending this being merged: |
* add transport layer interface * universal client * impl sse & stdio transport based on the original client * refactor old client to provide compibility * rename * remove old client types * add test for stdio transport * rename 'done' to 'closed', to distinguish with ctx.Done * add cancelSSEStream for better handling of close * fix connection leak when start timeout * avoid multiple starting * use atomic for closed to be more natural compared to started * fix leak of timer * Create sse_test.go * enforce test * add comment * sse: add custom header in start request * update comment * comment * cover #88 * cover #107 * fix demo sse server in race test
In #55, an option was added to set an sse read timeout. This timeout, however, applies to the entire
readSSEfunction which is responsible for receiving messages from the server after the client's initialization. Because of this, any SSE client stops working 30 seconds after initialization. Any request hangs because no responses are being processed.The original author mentioned parity with the Python SDK as a justification for adding this option. However, in the python SDK, the timeout applies to a specific call, not to the entire message loop. In Go, callers can directly create their context with a given timeout or deadline without a specific option for timeouts.
For this reason, I would recommend removing the option altogether.
Summary by CodeRabbit