From b68cf70ec5633fe850ad47f04bfc7b6570b3ea7e Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Wed, 3 Mar 2021 23:09:49 +0000 Subject: [PATCH 1/9] change async cancel and add test --- .../Microsoft/Data/SqlClient/SqlCommand.cs | 51 ++++------ .../Data/SqlClient/TdsParserStateObject.cs | 95 +++++++++---------- .../SQL/SqlCommand/SqlCommandCancelTest.cs | 56 +++++++++++ 3 files changed, 119 insertions(+), 83 deletions(-) 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 238e5d7221..958dbe893f 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 @@ -1404,19 +1404,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); } } @@ -1820,19 +1814,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); } } @@ -2017,18 +2007,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 8645cee07e..a4fdd6bb2c 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; @@ -629,6 +637,11 @@ internal void Activate(object owner) Debug.Assert(result == 1, "invalid deactivate count"); } + internal bool SetCancelStateClosed() + { + return Interlocked.CompareExchange(ref _cancelState, CancelState.Unset, CancelState.Closed) == CancelState.Unset; + } + // This method is only called by the command or datareader as a result of a user initiated // cancel request. internal void Cancel(object caller) @@ -636,61 +649,41 @@ 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 + if ( + (_parser.State != TdsParserState.Closed) && (_parser.State != TdsParserState.Broken) && + (_cancellationOwner.Target == caller) && + Interlocked.CompareExchange(ref _cancelState, CancelState.Unset, CancelState.Cancelled) == CancelState.Unset + ) { - // 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)) + if (HasPendingData && !_attentionSent) { - 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)) + 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 { - _cancelled = true; - - if (HasPendingData && !_attentionSent) + _parser.Connection._parserLock.Wait(canReleaseFromAnyThread: false, timeout: _waitForCancellationLockPollTimeout, lockTaken: ref hasParserLock); + if (hasParserLock) + { + _parser.Connection.ThreadHasParserLockForClose = true; + SendAttention(); + } + } + finally + { + if (hasParserLock) { - 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)) + if (_parser.Connection.ThreadHasParserLockForClose) { - 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 @@ -777,7 +770,7 @@ private void ResetCancelAndProcessAttention() lock (this) { // Reset cancel state. - _cancelled = false; + _cancelState = CancelState.Unset; _cancellationOwner.Target = null; if (_attentionSent) @@ -999,10 +992,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 @@ -3353,7 +3346,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; @@ -3401,7 +3394,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 @@ -3987,7 +3980,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/tests/ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs index 601bffa42a..3baee98245 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,46 @@ 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)); + + try + { + executing.Wait(); + } + catch (Exception ex) + { + exception = ex; + } + ended = DateTime.UtcNow; + + Assert.NotNull(exception); + 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($"WAITFOR DELAY '00:00:{delaySeconds:D2}'", connection)) + { + command.CommandTimeout = delaySeconds + 10; + await command.ExecuteNonQueryAsync(cancellationToken).ConfigureAwait(false); + } + } + } } } From 4f6aa83c63e085fce07d04f9fc0704dbbbe64c8c Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Fri, 5 Mar 2021 19:14:26 +0000 Subject: [PATCH 2/9] force cancel time on new test into range --- .../tests/ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 3baee98245..2d9d5ec6d8 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs @@ -586,7 +586,7 @@ private static void AsyncCancelDoesNotWait2(string connStr) Task executing = ExecuteWaitForAsync(cancellationTokenSource.Token, connStr, delaySeconds); - cancellationTokenSource.CancelAfter(TimeSpan.FromSeconds(cancelSeconds)); + cancellationTokenSource.CancelAfter(TimeSpan.FromSeconds(cancelSeconds+1)); try { From 598d4503f297ab6bcce009e22291b8fdd3e9c7c1 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Tue, 9 Mar 2021 00:32:59 +0000 Subject: [PATCH 3/9] change test to use loop --- .../ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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 2d9d5ec6d8..def19cc80a 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs @@ -607,7 +607,11 @@ private static async Task ExecuteWaitForAsync(CancellationToken cancellationToke using (var connection = new SqlConnection(connectionString)) { await connection.OpenAsync().ConfigureAwait(false); - using (var command = new SqlCommand($"WAITFOR DELAY '00:00:{delaySeconds:D2}'", connection)) + 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); From 41af8ac0444fef14177213eafd98b637620dc4f2 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Sat, 26 Jun 2021 16:46:51 +0100 Subject: [PATCH 4/9] implement on netfx --- .../Microsoft/Data/SqlClient/SqlCommand.cs | 51 ++++----- .../Data/SqlClient/TdsParserStateObject.cs | 101 +++++++++--------- 2 files changed, 67 insertions(+), 85 deletions(-) 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 19fd0902f3..6fb39436f9 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 @@ -1757,19 +1757,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); } } @@ -2270,19 +2264,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); } } @@ -2532,19 +2521,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 131b22088c..8c4f0dba93 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 @@ -650,68 +658,57 @@ internal void Activate(object owner) Debug.Assert(result == 1, "invalid deactivate count"); } + internal bool SetCancelStateClosed() + { + return Interlocked.CompareExchange(ref _cancelState, CancelState.Unset, CancelState.Closed) == CancelState.Unset; + } + // 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 + // Keep looping until we either grabbed the lock (and therefore sent attention) or the connection closes\breaks + if ( + (_parser.State != TdsParserState.Closed) && (_parser.State != TdsParserState.Broken) && + Interlocked.CompareExchange(ref _cancelState, CancelState.Unset, CancelState.Cancelled) == CancelState.Unset + ) { - // 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)) + // 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 ( + (objectID == _allowObjectID) && + (objectID != -1)) { - - 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)) + if (_pendingData && !_attentionSent) + { + 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)) { - _cancelled = true; - - if (_pendingData && !_attentionSent) + try { - 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)) + _parser.Connection._parserLock.Wait(canReleaseFromAnyThread: false, timeout: _waitForCancellationLockPollTimeout, lockTaken: ref hasParserLock); + if (hasParserLock) { - try - { - _parser.Connection._parserLock.Wait(canReleaseFromAnyThread: false, timeout: _waitForCancellationLockPollTimeout, lockTaken: ref hasParserLock); - if (hasParserLock) - { - _parser.Connection.ThreadHasParserLockForClose = true; - SendAttention(); - } - } - finally + _parser.Connection.ThreadHasParserLockForClose = true; + SendAttention(); + } + } + finally + { + if (hasParserLock) + { + if (_parser.Connection.ThreadHasParserLockForClose) { - 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 @@ -804,7 +801,7 @@ private void ResetCancelAndProcessAttention() lock (this) { // Reset cancel state. - _cancelled = false; + _cancelState = CancelState.Unset; _allowObjectID = -1; if (_attentionSent) @@ -1108,10 +1105,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 @@ -3397,7 +3394,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; @@ -3446,7 +3443,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 @@ -4128,7 +4125,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"); From aa78af283779b3acb082eccf7458f73ba6bfc7d3 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Tue, 20 Jul 2021 01:25:12 +0100 Subject: [PATCH 5/9] reinforce CompareExchange usage --- .../src/Microsoft/Data/SqlClient/TdsParserStateObject.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 a4fdd6bb2c..89a78fed92 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 @@ -639,7 +639,7 @@ internal void Activate(object owner) internal bool SetCancelStateClosed() { - return Interlocked.CompareExchange(ref _cancelState, CancelState.Unset, CancelState.Closed) == CancelState.Unset; + 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 @@ -652,7 +652,7 @@ internal void Cancel(object caller) if ( (_parser.State != TdsParserState.Closed) && (_parser.State != TdsParserState.Broken) && (_cancellationOwner.Target == caller) && - Interlocked.CompareExchange(ref _cancelState, CancelState.Unset, CancelState.Cancelled) == CancelState.Unset + Interlocked.CompareExchange(ref _cancelState, CancelState.Cancelled, CancelState.Unset) == CancelState.Unset && _cancelState == CancelState.Cancelled ) { if (HasPendingData && !_attentionSent) From b2dbb705e02381f07aca277666344eb6cfcaa9d5 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Tue, 20 Jul 2021 10:25:13 +0100 Subject: [PATCH 6/9] make cancel opportunistic not required to send attention. --- .../src/Microsoft/Data/SqlClient/TdsParserStateObject.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 89a78fed92..8689412133 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 @@ -649,10 +649,11 @@ 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()); + Interlocked.CompareExchange(ref _cancelState, CancelState.Cancelled, CancelState.Unset); // only change state if it is Unset, so don't check the return value + if ( (_parser.State != TdsParserState.Closed) && (_parser.State != TdsParserState.Broken) && - (_cancellationOwner.Target == caller) && - Interlocked.CompareExchange(ref _cancelState, CancelState.Cancelled, CancelState.Unset) == CancelState.Unset && _cancelState == CancelState.Cancelled + (_cancellationOwner.Target == caller) ) { if (HasPendingData && !_attentionSent) From 6c1521d9526a334de443f2f71809ffc87658c993 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Tue, 31 Aug 2021 10:23:24 -0700 Subject: [PATCH 7/9] Update src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs Co-authored-by: DavoudEshtehari <61173489+DavoudEshtehari@users.noreply.github.com> --- .../netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs | 1 - 1 file changed, 1 deletion(-) 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 8c4f0dba93..8266e13481 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 @@ -667,7 +667,6 @@ internal bool SetCancelStateClosed() // cancel request. internal void Cancel(int objectID) { - // Keep looping until we either grabbed the lock (and therefore sent attention) or the connection closes\breaks if ( (_parser.State != TdsParserState.Closed) && (_parser.State != TdsParserState.Broken) && Interlocked.CompareExchange(ref _cancelState, CancelState.Unset, CancelState.Cancelled) == CancelState.Unset From 5579af82dff480c25acbc25e8f90cfca0ea59486 Mon Sep 17 00:00:00 2001 From: Davoud Eshtehari Date: Tue, 31 Aug 2021 18:10:14 -0700 Subject: [PATCH 8/9] Fix The mistaken commits were rolled back and was merged with the current source code. --- .../Data/SqlClient/TdsParserStateObject.cs | 42 +++++++------- .../Data/SqlClient/TdsParserStateObject.cs | 57 ++++++++----------- 2 files changed, 44 insertions(+), 55 deletions(-) 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 2e0e2f1627..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 @@ -643,38 +643,34 @@ 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()); - Interlocked.CompareExchange(ref _cancelState, CancelState.Cancelled, CancelState.Unset); // only change state if it is Unset, so don't check the return value + // 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) - ) + if ((_parser.State != TdsParserState.Closed) && (_parser.State != TdsParserState.Broken) + && (_cancellationOwner.Target == caller) && HasPendingData && !_attentionSent) { - if (HasPendingData && !_attentionSent) + 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)) { - 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 { - try + _parser.Connection._parserLock.Wait(canReleaseFromAnyThread: false, timeout: _waitForCancellationLockPollTimeout, lockTaken: ref hasParserLock); + if (hasParserLock) { - _parser.Connection._parserLock.Wait(canReleaseFromAnyThread: false, timeout: _waitForCancellationLockPollTimeout, lockTaken: ref hasParserLock); - if (hasParserLock) - { - _parser.Connection.ThreadHasParserLockForClose = true; - SendAttention(); - } + _parser.Connection.ThreadHasParserLockForClose = true; + SendAttention(); } - finally + } + finally + { + if (hasParserLock) { - if (hasParserLock) + if (_parser.Connection.ThreadHasParserLockForClose) { - if (_parser.Connection.ThreadHasParserLockForClose) - { - _parser.Connection.ThreadHasParserLockForClose = false; - } - _parser.Connection._parserLock.Release(); + _parser.Connection.ThreadHasParserLockForClose = false; } + _parser.Connection._parserLock.Release(); } } } 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 520d42d77f..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 @@ -654,50 +654,43 @@ internal void Activate(object owner) internal bool SetCancelStateClosed() { - return Interlocked.CompareExchange(ref _cancelState, CancelState.Unset, CancelState.Closed) == CancelState.Unset; + 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) { - if ( - (_parser.State != TdsParserState.Closed) && (_parser.State != TdsParserState.Broken) && - Interlocked.CompareExchange(ref _cancelState, CancelState.Unset, CancelState.Cancelled) == CancelState.Unset - ) + // 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) { - // 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 ( - (objectID == _allowObjectID) && - (objectID != -1)) + 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)) { - if (_pendingData && !_attentionSent) + try { - 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)) + _parser.Connection._parserLock.Wait(canReleaseFromAnyThread: false, timeout: _waitForCancellationLockPollTimeout, lockTaken: ref hasParserLock); + if (hasParserLock) { - try - { - _parser.Connection._parserLock.Wait(canReleaseFromAnyThread: false, timeout: _waitForCancellationLockPollTimeout, lockTaken: ref hasParserLock); - if (hasParserLock) - { - _parser.Connection.ThreadHasParserLockForClose = true; - SendAttention(); - } - } - finally + _parser.Connection.ThreadHasParserLockForClose = true; + SendAttention(); + } + } + finally + { + if (hasParserLock) + { + if (_parser.Connection.ThreadHasParserLockForClose) { - if (hasParserLock) - { - if (_parser.Connection.ThreadHasParserLockForClose) - { - _parser.Connection.ThreadHasParserLockForClose = false; - } - _parser.Connection._parserLock.Release(); - } + _parser.Connection.ThreadHasParserLockForClose = false; } + _parser.Connection._parserLock.Release(); } } } From 35dbec5715d7b52148af7c78b67979406f731978 Mon Sep 17 00:00:00 2001 From: Davoud Eshtehari Date: Tue, 31 Aug 2021 19:20:40 -0700 Subject: [PATCH 9/9] Improve test --- .../tests/ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs | 4 ++++ 1 file changed, 4 insertions(+) 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 def19cc80a..18dde97c6c 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs @@ -599,6 +599,10 @@ private static void AsyncCancelDoesNotWait2(string connStr) 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); }