Skip to content

oauthex: keep defaultDiscoveryTransport out of package initialization - #1279

Open
marler8997 wants to merge 1 commit into
modelcontextprotocol:mainfrom
marler8997:oauthex-linker-lazy-transport
Open

marler8997 wants to merge 1 commit into
modelcontextprotocol:mainfrom
marler8997:oauthex-linker-lazy-transport

Conversation

@marler8997

Copy link
Copy Markdown

PR #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 #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

  • Why not keep sync.OnceValue? Any package-level OnceValue is 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 with go build -ldflags=-dumpdep and go tool nm.
  • The test shells out to go build and go tool nm. This follows the standard library's net/http.TestCmdGoNoHTTPServer, which checks a built binary's symbols the same way. It checks symbols in both directions: mcp.(*Server).Run must be present, which confirms nm read the right binary, and crypto/tls.(*Conn).clientHandshake must be absent. The exported Handshake is 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.
  • The test builds examples/server/hello rather 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.
  • After this change, no SDK package initializer reaches net/http, crypto/tls or crypto/x509. Some crypto code is still linked, from the standard library's own FIPS self-test initializers, because net/http is still imported. The SDK can't avoid that without dropping the import.
  • Possible follow-up, not in this PR: mcp's method tables keep about 100KB of encoding/gob linked through pagination cursor decoding. Fixing that means restructuring code, so I've left it out of this PR.

Updates #1276

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant