diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs index 5329d1ff08..ae7b81d2e4 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs @@ -1470,19 +1470,13 @@ public int EndExecuteNonQueryAsync(IAsyncResult asyncResult) else { ThrowIfReconnectionHasBeenCanceled(); - // lock on _stateObj prevents races with close/cancel. - // If we have already initiate the End call internally, we have already done that, so no point doing it again. - if (!_internalEndExecuteInitiated) + if (!_internalEndExecuteInitiated && _stateObj != null) { - lock (_stateObj) - { - return EndExecuteNonQueryInternal(asyncResult); - } - } - else - { - return EndExecuteNonQueryInternal(asyncResult); + // call SetCancelStateClosed on the stateobject to ensure that cancel cannot + // happen after we have changed started the end processing + _stateObj.SetCancelStateClosed(); } + return EndExecuteNonQueryInternal(asyncResult); } } @@ -1904,19 +1898,15 @@ private XmlReader EndExecuteXmlReaderAsync(IAsyncResult asyncResult) else { ThrowIfReconnectionHasBeenCanceled(); - // lock on _stateObj prevents races with close/cancel. - // If we have already initiate the End call internally, we have already done that, so no point doing it again. - if (!_internalEndExecuteInitiated) - { - lock (_stateObj) - { - return EndExecuteXmlReaderInternal(asyncResult); - } - } - else + + if (!_internalEndExecuteInitiated && _stateObj != null) { - return EndExecuteXmlReaderInternal(asyncResult); + // call SetCancelStateClosed on the stateobject to ensure that cancel cannot + // happen after we have changed started the end processing + _stateObj.SetCancelStateClosed(); } + + return EndExecuteXmlReaderInternal(asyncResult); } } @@ -2102,18 +2092,15 @@ internal SqlDataReader EndExecuteReaderAsync(IAsyncResult asyncResult) else { ThrowIfReconnectionHasBeenCanceled(); - // lock on _stateObj prevents races with close/cancel. - if (!_internalEndExecuteInitiated) - { - lock (_stateObj) - { - return EndExecuteReaderInternal(asyncResult); - } - } - else + + if (!_internalEndExecuteInitiated && _stateObj != null) { - return EndExecuteReaderInternal(asyncResult); + // call SetCancelStateClosed on the stateobject to ensure that cancel cannot happen after + // we have changed started the end processing + _stateObj.SetCancelStateClosed(); } + + return EndExecuteReaderInternal(asyncResult); } } diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs index 7e06c3df98..077dd689cc 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs @@ -162,10 +162,18 @@ public TimeoutState(int value) // 2) post first packet write, but before session return - a call to cancel will send an // attention to the server // 3) post session close - no attention is allowed - private bool _cancelled; private const int _waitForCancellationLockPollTimeout = 100; private WeakReference _cancellationOwner = new WeakReference(null); + private static class CancelState + { + public const int Unset = 0; + public const int Closed = 1; + public const int Cancelled = 2; + } + + private int _cancelState; + // Cache the transaction for which this command was executed so upon completion we can // decrement the appropriate result count. internal SqlInternalTransaction _executedUnderTransaction; @@ -623,6 +631,11 @@ internal void Activate(object owner) Debug.Assert(result == 1, "invalid deactivate count"); } + internal bool SetCancelStateClosed() + { + return Interlocked.CompareExchange(ref _cancelState, CancelState.Closed, CancelState.Unset) == CancelState.Unset && _cancelState == CancelState.Closed; + } + // This method is only called by the command or datareader as a result of a user initiated // cancel request. internal void Cancel(object caller) @@ -630,61 +643,38 @@ internal void Cancel(object caller) Debug.Assert(caller != null, "Null caller for Cancel!"); Debug.Assert(caller is SqlCommand || caller is SqlDataReader, "Calling API with invalid caller type: " + caller.GetType()); - bool hasLock = false; - try + // only change state if it is Unset, so don't check the return value + Interlocked.CompareExchange(ref _cancelState, CancelState.Cancelled, CancelState.Unset); + + if ((_parser.State != TdsParserState.Closed) && (_parser.State != TdsParserState.Broken) + && (_cancellationOwner.Target == caller) && HasPendingData && !_attentionSent) { - // Keep looping until we either grabbed the lock (and therefore sent attention) or the connection closes\breaks - while ((!hasLock) && (_parser.State != TdsParserState.Closed) && (_parser.State != TdsParserState.Broken)) + bool hasParserLock = false; + // Keep looping until we have the parser lock (and so are allowed to write), or the connection closes\breaks + while ((!hasParserLock) && (_parser.State != TdsParserState.Closed) && (_parser.State != TdsParserState.Broken)) { - Monitor.TryEnter(this, _waitForCancellationLockPollTimeout, ref hasLock); - if (hasLock) - { // Lock for the time being - since we need to synchronize the attention send. - // This lock is also protecting against concurrent close and async continuations - - // Ensure that, once we have the lock, that we are still the owner - if ((!_cancelled) && (_cancellationOwner.Target == caller)) + try + { + _parser.Connection._parserLock.Wait(canReleaseFromAnyThread: false, timeout: _waitForCancellationLockPollTimeout, lockTaken: ref hasParserLock); + if (hasParserLock) { - _cancelled = true; - - if (HasPendingData && !_attentionSent) + _parser.Connection.ThreadHasParserLockForClose = true; + SendAttention(); + } + } + finally + { + if (hasParserLock) + { + if (_parser.Connection.ThreadHasParserLockForClose) { - bool hasParserLock = false; - // Keep looping until we have the parser lock (and so are allowed to write), or the connection closes\breaks - while ((!hasParserLock) && (_parser.State != TdsParserState.Closed) && (_parser.State != TdsParserState.Broken)) - { - try - { - _parser.Connection._parserLock.Wait(canReleaseFromAnyThread: false, timeout: _waitForCancellationLockPollTimeout, lockTaken: ref hasParserLock); - if (hasParserLock) - { - _parser.Connection.ThreadHasParserLockForClose = true; - SendAttention(); - } - } - finally - { - if (hasParserLock) - { - if (_parser.Connection.ThreadHasParserLockForClose) - { - _parser.Connection.ThreadHasParserLockForClose = false; - } - _parser.Connection._parserLock.Release(); - } - } - } + _parser.Connection.ThreadHasParserLockForClose = false; } + _parser.Connection._parserLock.Release(); } } } } - finally - { - if (hasLock) - { - Monitor.Exit(this); - } - } } // CancelRequest - use to cancel while writing a request to the server @@ -771,7 +761,7 @@ private void ResetCancelAndProcessAttention() lock (this) { // Reset cancel state. - _cancelled = false; + _cancelState = CancelState.Unset; _cancellationOwner.Target = null; if (_attentionSent) @@ -993,10 +983,10 @@ internal Task ExecuteFlush() { lock (this) { - if (_cancelled && 1 == _outputPacketNumber) + if (_cancelState != CancelState.Unset && 1 == _outputPacketNumber) { ResetBuffer(); - _cancelled = false; + _cancelState = CancelState.Unset; throw SQL.OperationCancelled(); } else @@ -3354,7 +3344,7 @@ internal Task WritePacket(byte flushMode, bool canAccumulate = false) byte packetNumber = _outputPacketNumber; // Set Status byte based whether this is end of message or not - bool willCancel = (_cancelled) && (_parser._asyncWrite); + bool willCancel = (_cancelState != CancelState.Unset) && (_parser._asyncWrite); if (willCancel) { status = TdsEnums.ST_EOM | TdsEnums.ST_IGNORE; @@ -3402,7 +3392,7 @@ internal Task WritePacket(byte flushMode, bool canAccumulate = false) private void CancelWritePacket() { - Debug.Assert(_cancelled, "Should not call CancelWritePacket if _cancelled is not set"); + Debug.Assert(_cancelState != CancelState.Unset, "Should not call CancelWritePacket if _cancelled is not set"); _parser.Connection.ThreadHasParserLockForClose = true; // In case of error, let the connection know that we are holding the lock try @@ -3988,7 +3978,7 @@ internal void AssertStateIsClean() Debug.Assert(_delayedWriteAsyncCallbackException == null, "StateObj has an unobserved exceptions from an async write"); // Attention\Cancellation\Timeouts Debug.Assert(!HasReceivedAttention && !_attentionSent && !_attentionSending, $"StateObj is still dealing with attention: Sent: {_attentionSent}, Received: {HasReceivedAttention}, Sending: {_attentionSending}"); - Debug.Assert(!_cancelled, "StateObj still has cancellation set"); + Debug.Assert(_cancelState == CancelState.Unset, "StateObj still has cancellation set"); Debug.Assert(_timeoutState == TimeoutState.Stopped, "StateObj still has internal timeout set"); // Errors and Warnings Debug.Assert(!_hasErrorOrWarning, "StateObj still has stored errors or warnings"); diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs index 774ee07582..65fb0e7875 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs @@ -1783,19 +1783,13 @@ private int EndExecuteNonQueryAsync(IAsyncResult asyncResult) else { ThrowIfReconnectionHasBeenCanceled(); - // lock on _stateObj prevents races with close/cancel. - // If we have already initiate the End call internally, we have already done that, so no point doing it again. - if (!_internalEndExecuteInitiated) + if (!_internalEndExecuteInitiated && _stateObj != null) { - lock (_stateObj) - { - return EndExecuteNonQueryInternal(asyncResult); - } - } - else - { - return EndExecuteNonQueryInternal(asyncResult); + // call SetCancelStateClosed on the stateobject to ensure that cancel cannot + // happen after we have changed started the end processing + _stateObj.SetCancelStateClosed(); } + return EndExecuteNonQueryInternal(asyncResult); } } @@ -2303,19 +2297,14 @@ private XmlReader EndExecuteXmlReaderAsync(IAsyncResult asyncResult) else { ThrowIfReconnectionHasBeenCanceled(); - // lock on _stateObj prevents races with close/cancel. - // If we have already initiate the End call internally, we have already done that, so no point doing it again. - if (!_internalEndExecuteInitiated) - { - lock (_stateObj) - { - return EndExecuteXmlReaderInternal(asyncResult); - } - } - else + if (!_internalEndExecuteInitiated && _stateObj != null) { - return EndExecuteXmlReaderInternal(asyncResult); + // call SetCancelStateClosed on the stateobject to ensure that cancel cannot + // happen after we have changed started the end processing + _stateObj.SetCancelStateClosed(); } + + return EndExecuteXmlReaderInternal(asyncResult); } } @@ -2562,19 +2551,15 @@ private SqlDataReader EndExecuteReaderAsync(IAsyncResult asyncResult) else { ThrowIfReconnectionHasBeenCanceled(); - // lock on _stateObj prevents races with close/cancel. - // If we have already initiate the End call internally, we have already done that, so no point doing it again. - if (!_internalEndExecuteInitiated) - { - lock (_stateObj) - { - return EndExecuteReaderInternal(asyncResult); - } - } - else + + if (!_internalEndExecuteInitiated && _stateObj != null) { - return EndExecuteReaderInternal(asyncResult); + // call SetCancelStateClosed on the stateobject to ensure that cancel cannot happen after + // we have changed started the end processing + _stateObj.SetCancelStateClosed(); } + + return EndExecuteReaderInternal(asyncResult); } } diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs index 8a6be2314e..1d96d61281 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs @@ -153,9 +153,17 @@ internal int ObjectID // 2) post first packet write, but before session return - a call to cancel will send an // attention to the server // 3) post session close - no attention is allowed - private bool _cancelled; private const int _waitForCancellationLockPollTimeout = 100; + private static class CancelState + { + public const int Unset = 0; + public const int Closed = 1; + public const int Cancelled = 2; + } + + private int _cancelState; + // This variable is used to prevent sending an attention by another thread that is not the // current owner of the stateObj. I currently do not know how this can happen. Mark added // the code but does not remember either. At some point, we need to research killing this @@ -644,68 +652,49 @@ internal void Activate(object owner) Debug.Assert(result == 1, "invalid deactivate count"); } + internal bool SetCancelStateClosed() + { + return Interlocked.CompareExchange(ref _cancelState, CancelState.Closed, CancelState.Unset) == CancelState.Unset && _cancelState == CancelState.Closed; + } + // This method is only called by the command or datareader as a result of a user initiated // cancel request. internal void Cancel(int objectID) { - bool hasLock = false; - try + // only change state if it is Unset, so don't check the return value + Interlocked.CompareExchange(ref _cancelState, CancelState.Cancelled, CancelState.Unset); + + // don't allow objectID -1 since it is reserved for 'not associated with a command' + // yes, the 2^32-1 comand won't cancel - but it also won't cancel when we don't want it + if ((_parser.State != TdsParserState.Closed) && (_parser.State != TdsParserState.Broken) + && (objectID == _allowObjectID) && (objectID != -1) && _pendingData && !_attentionSent) { - // Keep looping until we either grabbed the lock (and therefore sent attention) or the connection closes\breaks - while ((!hasLock) && (_parser.State != TdsParserState.Closed) && (_parser.State != TdsParserState.Broken)) + bool hasParserLock = false; + // Keep looping until we have the parser lock (and so are allowed to write), or the conneciton closes\breaks + while ((!hasParserLock) && (_parser.State != TdsParserState.Closed) && (_parser.State != TdsParserState.Broken)) { - - Monitor.TryEnter(this, _waitForCancellationLockPollTimeout, ref hasLock); - if (hasLock) - { // Lock for the time being - since we need to synchronize the attention send. - // At some point in the future, I hope to remove this. - // This lock is also protecting against concurrent close and async continuations - - // don't allow objectID -1 since it is reserved for 'not associated with a command' - // yes, the 2^32-1 comand won't cancel - but it also won't cancel when we don't want it - if ((!_cancelled) && (objectID == _allowObjectID) && (objectID != -1)) + try + { + _parser.Connection._parserLock.Wait(canReleaseFromAnyThread: false, timeout: _waitForCancellationLockPollTimeout, lockTaken: ref hasParserLock); + if (hasParserLock) { - _cancelled = true; - - if (_pendingData && !_attentionSent) + _parser.Connection.ThreadHasParserLockForClose = true; + SendAttention(); + } + } + finally + { + if (hasParserLock) + { + if (_parser.Connection.ThreadHasParserLockForClose) { - bool hasParserLock = false; - // Keep looping until we have the parser lock (and so are allowed to write), or the conneciton closes\breaks - while ((!hasParserLock) && (_parser.State != TdsParserState.Closed) && (_parser.State != TdsParserState.Broken)) - { - try - { - _parser.Connection._parserLock.Wait(canReleaseFromAnyThread: false, timeout: _waitForCancellationLockPollTimeout, lockTaken: ref hasParserLock); - if (hasParserLock) - { - _parser.Connection.ThreadHasParserLockForClose = true; - SendAttention(); - } - } - finally - { - if (hasParserLock) - { - if (_parser.Connection.ThreadHasParserLockForClose) - { - _parser.Connection.ThreadHasParserLockForClose = false; - } - _parser.Connection._parserLock.Release(); - } - } - } + _parser.Connection.ThreadHasParserLockForClose = false; } + _parser.Connection._parserLock.Release(); } } } } - finally - { - if (hasLock) - { - Monitor.Exit(this); - } - } } // CancelRequest - use to cancel while writing a request to the server @@ -798,7 +787,7 @@ private void ResetCancelAndProcessAttention() lock (this) { // Reset cancel state. - _cancelled = false; + _cancelState = CancelState.Unset; _allowObjectID = -1; if (_attentionSent) @@ -1102,10 +1091,10 @@ internal Task ExecuteFlush() { lock (this) { - if (_cancelled && 1 == _outputPacketNumber) + if (_cancelState != CancelState.Unset && 1 == _outputPacketNumber) { ResetBuffer(); - _cancelled = false; + _cancelState = CancelState.Unset; throw SQL.OperationCancelled(); } else @@ -3391,7 +3380,7 @@ internal Task WritePacket(byte flushMode, bool canAccumulate = false) byte packetNumber = _outputPacketNumber; // Set Status byte based whether this is end of message or not - bool willCancel = (_cancelled) && (_parser._asyncWrite); + bool willCancel = (_cancelState != CancelState.Unset) && (_parser._asyncWrite); if (willCancel) { status = TdsEnums.ST_EOM | TdsEnums.ST_IGNORE; @@ -3440,7 +3429,7 @@ internal Task WritePacket(byte flushMode, bool canAccumulate = false) private void CancelWritePacket() { - Debug.Assert(_cancelled, "Should not call CancelWritePacket if _cancelled is not set"); + Debug.Assert(_cancelState != CancelState.Unset, "Should not call CancelWritePacket if _cancelled is not set"); _parser.Connection.ThreadHasParserLockForClose = true; // In case of error, let the connection know that we are holding the lock try @@ -4122,7 +4111,7 @@ internal void AssertStateIsClean() Debug.Assert(_delayedWriteAsyncCallbackException == null, "StateObj has an unobserved exceptions from an async write"); // Attention\Cancellation\Timeouts Debug.Assert(!_attentionReceived && !_attentionSent && !_attentionSending, $"StateObj is still dealing with attention: Sent: {_attentionSent}, Received: {_attentionReceived}, Sending: {_attentionSending}"); - Debug.Assert(!_cancelled, "StateObj still has cancellation set"); + Debug.Assert(_cancelState == CancelState.Unset, "StateObj still has cancellation set"); Debug.Assert(_timeoutState == TimeoutState.Stopped, "StateObj still has internal timeout set"); // Errors and Warnings Debug.Assert(!_hasErrorOrWarning, "StateObj still has stored errors or warnings"); diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs index 601bffa42a..18dde97c6c 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs @@ -221,6 +221,21 @@ public static void AsyncCancelDoesNotWaitNP() AsyncCancelDoesNotWait(np_connStr).Wait(); } + // Synapse: WAITFOR not supported + ';' not supported. + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureSynapse))] + public static void AsyncCancelDoesNotWait2() + { + AsyncCancelDoesNotWait2(tcp_connStr); + } + + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureServer))] + [PlatformSpecific(TestPlatforms.Windows)] + public static void AsyncCancelDoesNotWaitNP2() + { + AsyncCancelDoesNotWait2(np_connStr); + } + + [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] public static void TCPAttentionPacketTestTransaction() { @@ -558,5 +573,54 @@ private static async Task AsyncCancelDoesNotWait(string connStr) Assert.InRange((ended - started).TotalSeconds, cancelSeconds, delaySeconds - 1); } } + + private static void AsyncCancelDoesNotWait2(string connStr) + { + const int delaySeconds = 30; + const int cancelSeconds = 1; + + var cancellationTokenSource = new CancellationTokenSource(); + DateTime started = DateTime.UtcNow; + DateTime ended = DateTime.UtcNow; + Exception exception = null; + + Task executing = ExecuteWaitForAsync(cancellationTokenSource.Token, connStr, delaySeconds); + + cancellationTokenSource.CancelAfter(TimeSpan.FromSeconds(cancelSeconds+1)); + + try + { + executing.Wait(); + } + catch (Exception ex) + { + exception = ex; + } + ended = DateTime.UtcNow; + + Assert.NotNull(exception); + Assert.IsType(exception); + Assert.NotNull(exception.InnerException); + Assert.IsType(exception.InnerException); + Assert.Contains("Operation cancelled by user.", exception.InnerException.Message); + Assert.InRange((ended - started).TotalSeconds, cancelSeconds, delaySeconds - 1); + } + + private static async Task ExecuteWaitForAsync(CancellationToken cancellationToken, string connectionString, int delaySeconds) + { + using (var connection = new SqlConnection(connectionString)) + { + await connection.OpenAsync().ConfigureAwait(false); + using (var command = new SqlCommand(@" +WHILE 1 = 1 +BEGIN + DECLARE @x INT = 1 +END", connection)) + { + command.CommandTimeout = delaySeconds + 10; + await command.ExecuteNonQueryAsync(cancellationToken).ConfigureAwait(false); + } + } + } } }