oauthex: keep defaultDiscoveryTransport out of package initialization - #1279
Open
marler8997 wants to merge 1 commit into
Open
marler8997 wants to merge 1 commit into
marler8997 wants to merge 1 commit into
Conversation
PR modelcontextprotocol#1278 made the default discovery transport lazy with a package-level sync.OnceValue. The closure passed to OnceValue is still referenced from the package initializer, and initializers are always linked, so the linker keeps newDiscoveryTransport and everything it reaches: http.Transport.Clone, the HTTP/2 transport setup, and crypto/tls. The mcp package imports auth, which imports oauthex, so every program using the SDK pays for this, including stdio-only servers that never make an HTTP request. The hello example server (linux/amd64) shrinks from 11.1MB to 8.7MB with Go 1.25.0, and from 11.5MB to 9.1MB with Go 1.26.8. Make defaultDiscoveryTransport a function backed by sync.Once, the same pattern net/http uses for its proxy environment. The transport is still built once on first use, preserving the proxy behavior from modelcontextprotocol#1276, but it is only linked into programs that can call it. Add a test that builds the hello example and fails if it links the TLS client handshake. Building the transport at import time would also re-link it, so the test guards modelcontextprotocol#1276 as well. Updates modelcontextprotocol#1276
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
PR #1278 made the default discovery transport lazy with a package-level
sync.OnceValue. The closure passed toOnceValueis still referenced from the package initializer, and initializers are always linked, so the linker keepsnewDiscoveryTransportand everything it reaches:http.Transport.Clone, the HTTP/2 transport setup, andcrypto/tls.The
mcppackage importsauth, which importsoauthex, so every program using the SDK pays for this, including stdio-only servers that never make an HTTP request. The hello example server (linux/amd64) shrinks from 11.1MB to 8.7MB with Go 1.25.0, and from 11.5MB to 9.1MB with Go 1.26.8.Make
defaultDiscoveryTransporta function backed bysync.Once, the same patternnet/httpuses for its proxy environment. The transport is still built once on first use, preserving the proxy behavior from #1276, but it is only linked into programs that can call it.Add a test that builds the hello example and fails if it links the TLS client handshake. Building the transport at import time would also re-link it, so the test guards #1276 as well.
Notes for reviewers
sync.OnceValue? Any package-levelOnceValueis built by the package initializer, which the linker always keeps, so everything the function reaches stays linked. Passing a named function instead of a closure made no difference: I checked withgo build -ldflags=-dumpdepandgo tool nm.go buildandgo tool nm. This follows the standard library'snet/http.TestCmdGoNoHTTPServer, which checks a built binary's symbols the same way. It checks symbols in both directions:mcp.(*Server).Runmust be present, which confirmsnmread the right binary, andcrypto/tls.(*Conn).clientHandshakemust be absent. The exportedHandshakeis inlined away, so it can't be the marker. The test takes about 1.5s with a cold build cache and 0.4s warm. I verified it fails without the fix on Go 1.25.0 and 1.26.0.examples/server/hellorather than a separate testdata program, because hello is already a minimal stdio-only server in the main module. The trade-off is that the test assumes hello stays stdio-only.net/http,crypto/tlsorcrypto/x509. Some crypto code is still linked, from the standard library's own FIPS self-test initializers, becausenet/httpis still imported. The SDK can't avoid that without dropping the import.mcp's method tables keep about 100KB ofencoding/goblinked through pagination cursor decoding. Fixing that means restructuring code, so I've left it out of this PR.Updates #1276