mcp: decoupling client transports from http.Client #523
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Context propagation was broken in StreamableClientTransport because: 1. Connect() used context.Background() instead of the parent context 2. Close() created a race condition where DELETE requests were cancelled This change fixes both issues by: - Using the parent context when creating the connection context - Reordering Close() to perform cleanup DELETE before cancelling context This ensures request-scoped values (auth tokens, trace IDs) propagate correctly to background HTTP operations. Fixes modelcontextprotocol#513
Adds TestStreamableClientContextPropagation to verify that context values are properly propagated to background HTTP operations (SSE GET and cleanup DELETE requests) in StreamableClientTransport. The test uses a custom HTTP handler to capture request contexts and verify that context values from the parent context are accessible in both the SSE connection establishment and session cleanup requests.
The test now properly verifies that StreamableClientTransport can handle contexts with values without errors, demonstrating that the context propagation fix is working correctly. The test validates the fix at streamable.go:1021 where context.WithCancel(ctx) is used instead of context.WithCancel(context.Background()).
…nsport Replace bloated context propagation test with clean, idiomatic Go code that: - Follows existing test patterns in the codebase - Removes unnecessary comments and complex synchronization - Tests actual context derivation (cancellable contexts) - Demonstrates the fix works without HTTP value propagation complexity The test verifies that background HTTP operations (SSE GET and DELETE cleanup) use properly derived contexts from the original Connect() context, confirming the fix in streamable.go where context.WithCancel(ctx) replaced context.WithCancel(context.Background()).
Replace unsafe global testAuth boolean with atomic.Bool to prevent data race between test setup/teardown and background HTTP operations in StreamableClientTransport.
Add test to verify that context values and cancellation are properly propagated through StreamableClientTransport.Connect(). The test ensures: - Context values are preserved in the connection context - Connection context remains cancellable - Parent context cancellation propagates to connection context This addresses the context propagation requirements for the streamable transport implementation.
…centralize interface; docs: update protocol notes
Contributor
Author
|
Wrong branch! :( |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This change decouples client transports from the concrete
net/http.Clientby accepting any HTTP client that implementsDo(http.Request) (http.Response, error). This enables middleware (auth, tracing, retries), improves testability, and unifies behavior across streamable and SSE transports.What changed
HTTPClientinterface in themcppackage.StreamableClientTransport: changeHTTPClientfield type to the interface; default tohttp.DefaultClientwhen nil.Why
Compatibility
http.Clientcontinues to work (it satisfies the interface).Testing
go test ./...passes locally.Fixes #522