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
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.
Why
The drivers registered by
mysql.RegisterDriver,pgxv5.RegisterDriver(and the deprecatedpgxv4package) andmssql.RegisterDriverimplementdriver.Driverbut notdriver.DriverContext.sql.Opentherefore wraps them indatabase/sql's internaldsnConnector, whoseConnect(_ context.Context)discards the caller's context and callsDriver.Open(dsn).Each wrapper then establishes the connection on a context unrelated to the caller:
pgxv5:stdlib.Driver.Openusescontext.WithTimeout(context.Background(), 60*time.Second).mysql:MySQLDriver.OpencallsConnect(context.Background()).mssql: the wrapper itself callsc.Connect(context.Background()).database/sqlreserves a pool slot (db.numOpen++) before callingConnectand releases it only afterConnectreturns. So when connection establishment is slow (for example, the SQL Admin API is slow or unreachable andDialer.Dialis waiting for connection info), the slot stays occupied even after the caller's context has expired. The wait inside the connector does honorctx.Done(), but it never receives the caller's context.The effect is bounded differently per driver:
Connectwithout this fixpgxv5/pgxv4stdlib.Driver.Openmysqltimeout=(default 0)mssqldial timeoutdefaultFor MySQL the wait is bounded only by whatever gives up further down the stack: the connector's internal
RefreshTimeout(60 s) when the default refresh-ahead cache is waiting on the SQL Admin API, or OS-level TCP behavior for the dial to the instance. WithWithLazyRefreshthe refresh runs on the background context and has no connector-level bound at all.With a bounded pool, a single request whose deadline was 100 ms can hold a slot for up to 60 s, and every request queued behind it fails on its own deadline. A one-slot pool, a fake SQL Admin API transport delaying each response by 600 ms and a request with a 100 ms deadline reproduces this deterministically: at 150 ms the pool still reports
OpenConnections=1, InUse=1, the next request hits its deadline withWaitCount=1, and the first request returns only after ~1.2 s.Note that
sql.OpenDB(stdlib.GetConnector(...))with the same Cloud SQL dialer does not have this problem, becausepgx's connector receives the caller's context. Only the documentedRegisterDriver→sql.Openpath is affected. All three wrappers still behave this way in v1.25.2.What Changed
Implement
OpenConnectorin the MySQL, pgxv5 and SQL Server wrappers and delegate connection establishment to the underlying context-aware connectors. Preserve the Cloud SQL dial configuration, legacyOpenbehavior, and the registered wrapper returned byDB.Driver()/Connector.Driver(). The deprecated pgxv4 package inherits the fix through its existing pgxv5 delegation.For SQL Server this also means the DSN is parsed and
mssqldb.NewConnectoris called once per*sql.DBinstead of once per connection.Verification
A regression test exercises the public
RegisterDriver→sql.Open→PingContextpath with a blocked SQL Admin API transport. It covers cancellation and a one-second deadline for MySQL, pgxv5, the pgxv4 compatibility package and SQL Server.With the original wrappers, all eight cases remain blocked beyond the two-second assertion window. With the fix, they return the expected context error and leave zero open/in-use pool slots while the API remains blocked. The test also verifies driver identity and invalid-DSN error propagation. It needs neither Google credentials nor a live Cloud SQL instance.