-
Notifications
You must be signed in to change notification settings - Fork 287
Description
Describe the bug
The comment for MaxRetries is slightly misleading.
For example a value of 0 means the reconnect for loop will automatically terminate immediately after client.Connect() and never even attempt a connection.
To Reproduce
Steps to reproduce the behavior:
Construct a client and transport with the zero struct which will have MaxRetries = 0:
transport := mcp.NewStreamableClientTransport(addr, &mcp.StreamableClientTransportOptions{
HTTPClient: httpClient,
ReconnectOptions: &mcp.StreamableReconnectOptions{},
})
client := mcp.NewClient(&mcp.Implementation{Name: "mcp-client", Version: "v0.0.1"}, nil)
ctx := context.Background()
session, err := client.Connect(ctx, transport)
if err != nil {
return fmt.Errorf("client.Connect error: %w", err)
}
Expected behavior
If the MaxRetries comment behavior is correct, then I would expect the for loop condition to change from < to <= which would mean at least one connection attempt when MaxRetries is 0, and 1+MaxRetries attempts when MaxRetries is any value above 0.
If zero actually means never connect, then a clearer explanation in the comment and/or a helpful check at the beginning of reconnect to return an error about a possible misconfiguration would also be helpful:
func (s *streamableClientConn) reconnect(lastEventID string) (*http.Response, error) {
if s.ReconnectOptions.MaxRetries == 0 {
return nil, fmt.Errorf("no connections attempted; was StreamableReconnectOptions.MaxRetries properly configured?")
}
...
Also nice to have would be a comment on ReconnectOptions that mentions if nil will default to DefaultReconnectOptions.