Work around SqlClient deadlock when a cancelled peek open enlists in the peek's TransactionScope - #1793
Open
SimonCropp wants to merge 1 commit into
Conversation
QueuePeeker.Peek opens its connection with the receive cancellation token inside an ambient TransactionScope. MessageReceiver.StopReceive cancels that token, so every endpoint shutdown cancels an in-flight open. Against a Pooling=false connection string that deadlocks inside Microsoft.Data.SqlClient. The client finishes the open the caller has already abandoned and enlists it on one thread, while TransactionScope.Dispose rolls the transaction back on another, and the two take the connection monitor and the parser lock in opposite orders. The peek never completes, so StopReceive waits on it forever and the endpoint never shuts down. Stop passing the token to the open, and bail out early when cancellation has already been requested. The open is then bounded by Connect Timeout instead of by the token, while TryPeek and the trailing Task.Delay still observe it, so shutdown stays prompt in the normal case. This is a workaround, marked with a TODO to undo. The underlying bug is dotnet/SqlClient#4696 Also adds an explicit stress test that reproduces the hang. It fails within a minute without the workaround and passes with it.
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.
This is a workaround, not a fix
The bug is in Microsoft.Data.SqlClient, filed as dotnet/SqlClient#4696. This PR only stops the transport from stepping into it. The change carries a
TODOto undo it once that issue is fixed and the minimum SqlClient version this transport depends on carries the fix.The problem
QueuePeeker.Peekopens its connection with the receive cancellation token, inside an ambientTransactionScope:MessageReceiver.StopReceivecancels that token, so every endpoint shutdown cancels an in-flight open. Against aPooling=falseconnection string that intermittently deadlocks inside SqlClient:Nothing breaks the cycle.
MessageReceiver.StopReceivethen awaitsmessageReceivingTaskforever (there is no timeout on it), so the endpoint never shuts down.Pooling=falseis the precondition: SqlClient callsCompleteLogin(!ConnectionOptions.Pooling), and the auto-enlistment only runs inside the login, while the connection ctor holds the parser lock, when pooling is off. Test helpers that hand out non-pooled connection strings per test database hit this; pooled production connection strings should not.This surfaced as CI builds hanging until the job timeout roughly once a week, with no test output and no exception. The hang dump is what identified it.
The change
Do not pass the token to the open, and return early when cancellation has already been requested. The open is then bounded by
Connect Timeoutinstead of by the token.Trade-off, stated plainly: shutdown now waits for an in-flight open to finish rather than cancelling it. Normally that is milliseconds. Against an unreachable server it is up to
Connect Timeout(15s by default). That is a bounded wait in place of an unbounded hang, but it is a real change in shutdown behaviour and worth a second opinion.TryPeekand the trailingTask.Delaystill observe the token, so the loop still exits promptly in the normal case.This also affects the PostgreSql transport, since
QueuePeeker.csis compiled into both. I have not run the PostgreSql tests.The test
When_peek_is_cancelled_during_connection_opendrives the realQueuePeeker.Peek, cancelling the token partway through the open, and fails via a watchdog when a worker stops making progress. Two details matter and are commented in the test: it forcesPooling=false, and it spins onStopwatchrather than usingCancelAfter, because the OS timer (~15.6ms) is an order of magnitude coarser than a login and never lands inside the window.SqlServer.UnitTestsSqlServer.IntegrationTestsVerified against the pinned Microsoft.Data.SqlClient 6.1.6 and also 7.0.3.
It is marked
[Explicit]on purpose and will not run in the normal suite: it burns up to two minutes when it passes, and when it reproduces it deliberately leaves deadlocked threads and a wedged SQL connection behind, so the test host is poisoned afterwards. Happy to drop it from this PR if you would rather not carry a stress test — the workaround stands on its own.