feat: add WithHTTPClient option for SSEMCPClient configuration #133
Conversation
WalkthroughThe pull request adds two new functions to the Changes
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 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)
56-60: Good addition - allows users to customize HTTP behavior.This feature adds valuable flexibility by allowing users to configure their own HTTP client with custom settings like timeouts, proxies, or transport configurations. The implementation follows the existing options pattern correctly.
Consider adding a brief comment above the function to document its purpose, similar to how
WithSSEReadTimeoutis documented.+// WithHTTPClient sets a custom HTTP client for the SSE connection and requests. func WithHTTPClient(client *http.Client) ClientOption { return func(sc *SSEMCPClient) { sc.httpClient = client } }
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/sse.go(1 hunks)
🔇 Additional comments (1)
client/sse.go (1)
55-59: Implementation looks goodThe
WithHTTPClientfunction correctly allows setting a custom HTTP client. This implementation properly overrides the default client created inNewSSEMCPClientat line 71.
| func WithSSEReadTimeout(timeout time.Duration) ClientOption { | ||
| return func(sc *SSEMCPClient) { | ||
| sc.sseReadTimeout = timeout | ||
| } | ||
| } | ||
|
|
||
| func WithHTTPClient(client *http.Client) ClientOption { | ||
| return func(sc *SSEMCPClient) { | ||
| sc.httpClient = client | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing the read timeout functionality
After adding the sseReadTimeout field to the struct, you should modify the readSSE function to actually use this timeout. The current implementation does not utilize the timeout value.
Here's a suggested implementation for using the timeout in the readSSE function:
func (c *SSEMCPClient) readSSE(reader io.ReadCloser) {
defer reader.Close()
br := bufio.NewReader(reader)
var event, data string
for {
select {
case <-c.done:
return
default:
+ // Set read deadline if timeout is specified
+ if c.sseReadTimeout > 0 {
+ if deadline, ok := reader.(interface{ SetReadDeadline(time.Time) error }); ok {
+ _ = deadline.SetReadDeadline(time.Now().Add(c.sseReadTimeout))
+ }
+ }
line, err := br.ReadString('\n')
if err != nil {
// ...existing error handling...
}
// ...rest of the function...
}
}
}Note that this implementation handles the case where the reader supports setting deadlines via the SetReadDeadline method. Not all readers support this, so it's implemented as a type assertion.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func WithSSEReadTimeout(timeout time.Duration) ClientOption { | |
| return func(sc *SSEMCPClient) { | |
| sc.sseReadTimeout = timeout | |
| } | |
| } | |
| func WithHTTPClient(client *http.Client) ClientOption { | |
| return func(sc *SSEMCPClient) { | |
| sc.httpClient = client | |
| } | |
| } | |
| func (c *SSEMCPClient) readSSE(reader io.ReadCloser) { | |
| defer reader.Close() | |
| br := bufio.NewReader(reader) | |
| var event, data string | |
| for { | |
| select { | |
| case <-c.done: | |
| return | |
| default: | |
| // Set read deadline if timeout is specified | |
| if c.sseReadTimeout > 0 { | |
| if deadline, ok := reader.(interface{ SetReadDeadline(time.Time) error }); ok { | |
| _ = deadline.SetReadDeadline(time.Now().Add(c.sseReadTimeout)) | |
| } | |
| } | |
| line, err := br.ReadString('\n') | |
| if err != nil { | |
| // ...existing error handling... | |
| } | |
| // ...rest of the function... | |
| } | |
| } | |
| } |
| func WithSSEReadTimeout(timeout time.Duration) ClientOption { | ||
| return func(sc *SSEMCPClient) { | ||
| sc.sseReadTimeout = timeout | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing sseReadTimeout field in SSEMCPClient struct
The WithSSEReadTimeout function attempts to set sc.sseReadTimeout, but there's no corresponding field in the SSEMCPClient struct (lines 25-39). This will cause a compilation error.
You need to add the field to the struct definition:
type SSEMCPClient struct {
baseURL *url.URL
endpoint *url.URL
httpClient *http.Client
requestID atomic.Int64
responses map[int64]chan RPCResponse
mu sync.RWMutex
done chan struct{}
initialized bool
notifications []func(mcp.JSONRPCNotification)
notifyMu sync.RWMutex
endpointChan chan struct{}
capabilities mcp.ServerCapabilities
headers map[string]string
+ sseReadTimeout time.Duration
}Additionally, the sseReadTimeout field is never used in the implementation. You should add timeout handling in the readSSE function to actually use this configuration option.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func WithSSEReadTimeout(timeout time.Duration) ClientOption { | |
| return func(sc *SSEMCPClient) { | |
| sc.sseReadTimeout = timeout | |
| } | |
| } | |
| type SSEMCPClient struct { | |
| baseURL *url.URL | |
| endpoint *url.URL | |
| httpClient *http.Client | |
| requestID atomic.Int64 | |
| responses map[int64]chan RPCResponse | |
| mu sync.RWMutex | |
| done chan struct{} | |
| initialized bool | |
| notifications []func(mcp.JSONRPCNotification) | |
| notifyMu sync.RWMutex | |
| endpointChan chan struct{} | |
| capabilities mcp.ServerCapabilities | |
| headers map[string]string | |
| sseReadTimeout time.Duration | |
| } |
Summary by CodeRabbit