fix: check if resp is nil on error for initialize methods #492
Conversation
WalkthroughThe update modifies the SendRequest method in the StreamableHTTP type to prevent a potential nil pointer dereference by adding a nil check before deferring the closure of resp.Body. Additionally, test cases in server/sse_test.go were improved by replacing boolean flags with channels to synchronize hook invocation more reliably. Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningsserver/sse_test.go (2)🔇 Additional comments (4)
✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/transport/streamable_http.go (1)
255-260: Good defensive fix for nil pointer dereference.The nil check before deferring
resp.Body.Close()correctly prevents the panic whensendHTTPfails and returns a nil response. This is a solid defensive programming practice.The comment accurately describes the issue and acknowledges that this is a temporary solution. The fallthrough logic around lines 247-254 does appear to need restructuring for better clarity and maintainability.
Consider refactoring the fallthrough logic to make the code flow clearer:
resp, err := c.sendHTTP(ctx, http.MethodPost, bytes.NewReader(requestBody), "application/json, text/event-stream") if err != nil { - if errors.Is(err, ErrSessionTerminated) && request.Method == string(mcp.MethodInitialize) { - // If the request is initialize, should not return a SessionTerminated error - // It should be a genuine endpoint-routing issue. - // ( Fall through to return StatusCode checking. ) - } else { - return nil, fmt.Errorf("failed to send request: %w", err) - } + // For initialize requests, ErrSessionTerminated should be treated as endpoint-routing issue + if !(errors.Is(err, ErrSessionTerminated) && request.Method == string(mcp.MethodInitialize)) { + return nil, fmt.Errorf("failed to send request: %w", err) + } + // resp might be nil here, so we need to check before using it } -// When sendHTTP fails and resp is nil but method is mcp.MethodInitialize -// defer resp.Body.Close() fails with nil pointer dereference. -// TODO: Restructure this fallthrough logic properly. if resp != nil { defer resp.Body.Close() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/transport/streamable_http.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ezynda3
PR: mark3labs/mcp-go#461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
client/transport/streamable_http.go (1)
Learnt from: ezynda3
PR: mark3labs/mcp-go#461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
|
Please allow me to comment that this does not look like a safe and solid fix. It would be clearer to handle the return in the if err statement above and then rely on resp being non-nil. This solution looks a bit awkward. Please ignore me if this comment is undesired. |
Description
Fixes the nil pointer dereference error when sendHTTP fails but the method is initialize and the resp is nil.
Ideally, we might need to rethink this to handle carefully.
There's also a race condition in the test which surfaced when I made this change. That is also fixed.
Type of Change
Checklist
Summary by CodeRabbit
Summary by CodeRabbit