fix: handle SSE stream disconnections #89
Conversation
…eam is active before a new request can be sent
WalkthroughThe update enhances the SSE client by introducing a new boolean field Changes
✨ 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 (2)
client/sse.go (2)
132-134: Good addition of the Connected() method.The new
Connected()method provides a clean way for client code to check the connection status before attempting operations that require an active connection.Consider adding documentation comments for this method to match the style of other methods in the file.
+// Connected returns whether the SSE stream is currently connected. func (c *SSEMCPClient) Connected() bool { return c.connected.Load() }
127-140: Consider adding a reconnection mechanism.While the current changes correctly track connection status, there's no automatic reconnection mechanism when a disconnection is detected. Consider adding a method to attempt reconnection or providing guidance in documentation for how client code should handle
ErrDisconnected.// Connected returns whether the SSE stream is currently connected. func (c *SSEMCPClient) Connected() bool { return c.connected.Load() } +// Reconnect attempts to reestablish the SSE connection. +// Returns an error if reconnection fails. +func (c *SSEMCPClient) Reconnect(ctx context.Context) error { + if c.Connected() { + return nil // Already connected + } + return c.Start(ctx) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/sse.go(3 hunks)
🔇 Additional comments (6)
client/sse.go (6)
40-40: Good addition of thread-safe connection tracking.The new
connectedfield usingatomic.Boolprovides a thread-safe way to track the connection status, which will be essential for handling SSE stream disconnections.
43-47: Well-defined error variables improve error handling.Replacing inline error messages with predefined error variables is a good practice. It makes the code more maintainable and allows callers to check for specific error types when handling disconnections.
127-128: Connection status correctly initialized.Setting the connection status to true after the endpoint is received and the connection is fully established ensures accurate tracking of the client's readiness to process requests.
140-140: Properly handling disconnection events.The deferred function that sets
connectedto false whenreadSSEends is crucial for correctly tracking disconnections. This ensures that when the SSE stream ends (due to network issues or server-side closure), the client immediately updates its status.
286-288: Key improvement in request handling.This check prevents sending requests when the connection is lost, which directly addresses the PR objective of handling SSE stream disconnections. By returning a specific error, client code can handle the disconnection appropriately and potentially attempt to reconnect.
279-284: Good use of predefined error variables.Replacing inline error messages with the predefined error variables improves consistency and makes error handling more robust.
| return nil | ||
| } | ||
|
|
||
| func (c *SSEMCPClient) Connected() bool { |
There was a problem hiding this comment.
This seems unused; was the intention to use it in line 286?
|
@mrene - Sorry for the delay here, I think this is a good addition and would love to land it if you have the time to fixup the merge conflicts? It would also be great to get some tests for the new field (and possibly an explicit test for the case you really care about -- disconnection of the SSE then a subsequent request from the client). If the test is too onerous though, that's totally fine -- I just like having regression tests for this kind of thing to ensure we don't break it again in the future... |
rwjblue-glean
added
the
status: needs submitter response
pottekkat
added
type: bug
If the HTTP SSE stream gets disconnected, future calls will fail due to a missing transport for response messages. This adds a
Connected() boolmethod to verify the connection status, and adds a predefined error indicating that the connection was interrupted.Summary by CodeRabbit