Add option to StreamableHTTPServer to allow custom http server instance #347
Conversation
…setting a custom HTTP server instance, similar to existing functionality in SSE.
WalkthroughA new option function enables injection of a custom Changes
Possibly related PRs
Suggested labels
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/streamable_http.go (1)
165-183: Good implementation with proper address conflict detection.The logic correctly handles different scenarios:
- Creates new server with mux when no custom server provided
- Assigns address to custom server when none set
- Detects and reports address conflicts appropriately
- Properly manages mutex locking/unlocking
However, consider documenting that when a custom server is provided, users are responsible for setting up the handler/routing themselves.
Consider adding a code comment to clarify the handler responsibility:
// WithStreamableHTTPServer sets the HTTP server instance for StreamableHTTPServer +// Note: When providing a custom server, you are responsible for configuring +// the handler and routing. The Start method will only manage the address. func WithStreamableHTTPServer(srv *http.Server) StreamableHTTPOption {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/streamable_http.go(2 hunks)server/streamable_http_test.go(1 hunks)
🔇 Additional comments (1)
server/streamable_http.go (1)
76-81: LGTM!The
WithStreamableHTTPServeroption function is correctly implemented and follows the established pattern for other option functions.
| func TestStreamableHTTPServer_WithOptions(t *testing.T) { | ||
| t.Run("WithStreamableHTTPServer sets httpServer field", func(t *testing.T) { | ||
| mcpServer := NewMCPServer("test", "1.0.0") | ||
| customServer := &http.Server{Addr: ":9999"} | ||
| httpServer := NewStreamableHTTPServer(mcpServer, WithStreamableHTTPServer(customServer)) | ||
|
|
||
| if httpServer.httpServer != customServer { | ||
| t.Errorf("Expected httpServer to be set to custom server instance, got %v", httpServer.httpServer) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("Start with conflicting address returns error", func(t *testing.T) { | ||
| mcpServer := NewMCPServer("test", "1.0.0") | ||
| customServer := &http.Server{Addr: ":9999"} | ||
| httpServer := NewStreamableHTTPServer(mcpServer, WithStreamableHTTPServer(customServer)) | ||
|
|
||
| err := httpServer.Start(":8888") | ||
| if err == nil { | ||
| t.Error("Expected error for conflicting address, got nil") | ||
| } else if !strings.Contains(err.Error(), "conflicting listen address") { | ||
| t.Errorf("Expected error message to contain 'conflicting listen address', got '%s'", err.Error()) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("Options consistency test", func(t *testing.T) { | ||
| mcpServer := NewMCPServer("test", "1.0.0") | ||
| endpointPath := "/test-mcp" | ||
| customServer := &http.Server{} | ||
|
|
||
| // Options to test | ||
| options := []StreamableHTTPOption{ | ||
| WithEndpointPath(endpointPath), | ||
| WithStreamableHTTPServer(customServer), | ||
| } | ||
|
|
||
| // Apply options multiple times and verify consistency | ||
| for i := 0; i < 10; i++ { | ||
| server := NewStreamableHTTPServer(mcpServer, options...) | ||
|
|
||
| if server.endpointPath != endpointPath { | ||
| t.Errorf("Expected endpointPath %s, got %s", endpointPath, server.endpointPath) | ||
| } | ||
|
|
||
| if server.httpServer != customServer { | ||
| t.Errorf("Expected httpServer to match, got %v", server.httpServer) | ||
| } | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Good test coverage but missing some important edge cases.
The current tests cover the basic functionality well:
- ✅ Custom server field assignment
- ✅ Address conflict detection
- ✅ Options consistency
However, consider adding tests for these scenarios to improve coverage:
Add these missing test cases:
+ t.Run("Start with custom server and no address sets address", func(t *testing.T) {
+ mcpServer := NewMCPServer("test", "1.0.0")
+ customServer := &http.Server{} // No address set
+ httpServer := NewStreamableHTTPServer(mcpServer, WithStreamableHTTPServer(customServer))
+
+ // This should work and set the address
+ go func() {
+ time.Sleep(10 * time.Millisecond)
+ httpServer.Shutdown(context.Background())
+ }()
+
+ err := httpServer.Start(":0") // Use :0 for random port
+ if err != nil && !strings.Contains(err.Error(), "use of closed network connection") {
+ t.Errorf("Expected no error or connection closed error, got: %v", err)
+ }
+
+ if customServer.Addr != ":0" {
+ t.Errorf("Expected custom server address to be set to ':0', got '%s'", customServer.Addr)
+ }
+ })
+
+ t.Run("Start with custom server and matching address succeeds", func(t *testing.T) {
+ mcpServer := NewMCPServer("test", "1.0.0")
+ customServer := &http.Server{Addr: ":0"}
+ httpServer := NewStreamableHTTPServer(mcpServer, WithStreamableHTTPServer(customServer))
+
+ go func() {
+ time.Sleep(10 * time.Millisecond)
+ httpServer.Shutdown(context.Background())
+ }()
+
+ err := httpServer.Start(":0") // Same address
+ if err != nil && !strings.Contains(err.Error(), "use of closed network connection") {
+ t.Errorf("Expected no error or connection closed error, got: %v", err)
+ }
+ })🤖 Prompt for AI Agents
In server/streamable_http_test.go around lines 673 to 721, the existing tests
cover basic functionality but lack edge case coverage. Add tests for scenarios
such as starting the server with an empty or invalid address, verifying behavior
when no options are provided, testing the default endpoint path, and ensuring
proper error handling when the underlying HTTP server fails to start. Implement
these additional test cases to improve overall test coverage and robustness.
| } else { | ||
| if s.httpServer.Addr == "" { | ||
| s.httpServer.Addr = addr | ||
| } else if s.httpServer.Addr != addr { | ||
| return fmt.Errorf("conflicting listen address: WithStreamableHTTPServer(%q) vs Start(%q)", s.httpServer.Addr, addr) | ||
| } |
There was a problem hiding this comment.
Nit: Should we document in this scenario that routing must be handled by the user? In the default/previous case, routing was handled by the Start method, but for the custom HTTP server instance, it is not. Maybe document it in a user-facing way with the WithStreamableHTTPServer function?
It can be a brief note because we can reasonably expect someone who wants to use a custom HTTP server to do this, regardless. But if they don't, the server would still start, but it wouldn't be able to do anything, right?
There was a problem hiding this comment.
okay, makes sense, will update for both sse and streamable http.
Summary by CodeRabbit