From 8ae26ab71e5ecff115533c342bb5b075f6456431 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Tue, 4 Oct 2022 09:34:24 -0700 Subject: [PATCH] [Release 5.0] Fix async deadlock issue when sending attention fails due to network failure --- .../Data/SqlClient/TdsParserStateObject.cs | 25 ++++++++++--------- .../Data/SqlClient/TdsParserStateObject.cs | 21 ++++++++-------- 2 files changed, 24 insertions(+), 22 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 21f4e9b61b..fbaa3dc480 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 @@ -55,7 +55,7 @@ public TimeoutState(int value) private const int AttentionTimeoutSeconds = 5; - private static readonly ContextCallback s_readAdyncCallbackComplete = ReadAsyncCallbackComplete; + private static readonly ContextCallback s_readAsyncCallbackComplete = ReadAsyncCallbackComplete; // Ticks to consider a connection "good" after a successful I/O (10,000 ticks = 1 ms) // The resolution of the timer is typically in the range 10 to 16 milliseconds according to msdn. @@ -2337,9 +2337,9 @@ private void OnTimeoutAsync(object state) } } - private bool OnTimeoutSync() + private bool OnTimeoutSync(bool asyncClose = false) { - return OnTimeoutCore(TimeoutState.Running, TimeoutState.ExpiredSync); + return OnTimeoutCore(TimeoutState.Running, TimeoutState.ExpiredSync, asyncClose); } /// @@ -2348,8 +2348,9 @@ private bool OnTimeoutSync() /// /// the state that is the expected current state, state will change only if this is correct /// the state that will be changed to if the expected state is correct + /// any close action to be taken by an async task to avoid deadlock. /// boolean value indicating whether the call changed the timeout state - private bool OnTimeoutCore(int expectedState, int targetState) + private bool OnTimeoutCore(int expectedState, int targetState, bool asyncClose = false) { Debug.Assert(targetState == TimeoutState.ExpiredAsync || targetState == TimeoutState.ExpiredSync, "OnTimeoutCore must have an expiry state as the targetState"); @@ -2382,7 +2383,7 @@ private bool OnTimeoutCore(int expectedState, int targetState) { try { - SendAttention(mustTakeWriteLock: true); + SendAttention(mustTakeWriteLock: true, asyncClose); } catch (Exception e) { @@ -2927,7 +2928,7 @@ public void ReadAsyncCallback(IntPtr key, PacketHandle packet, uint error) // synchrnously and then call OnTimeoutSync to force an atomic change of state. if (TimeoutHasExpired) { - OnTimeoutSync(); + OnTimeoutSync(true); } // try to change to the stopped state but only do so if currently in the running state @@ -2958,7 +2959,7 @@ public void ReadAsyncCallback(IntPtr key, PacketHandle packet, uint error) { if (_executionContext != null) { - ExecutionContext.Run(_executionContext, s_readAdyncCallbackComplete, source); + ExecutionContext.Run(_executionContext, s_readAsyncCallbackComplete, source); } else { @@ -3441,7 +3442,7 @@ private void CancelWritePacket() #pragma warning disable 0420 // a reference to a volatile field will not be treated as volatile - private Task SNIWritePacket(PacketHandle packet, out uint sniError, bool canAccumulate, bool callerHasConnectionLock) + private Task SNIWritePacket(PacketHandle packet, out uint sniError, bool canAccumulate, bool callerHasConnectionLock, bool asyncClose = false) { // Check for a stored exception var delayedException = Interlocked.Exchange(ref _delayedWriteAsyncCallbackException, null); @@ -3534,7 +3535,7 @@ private Task SNIWritePacket(PacketHandle packet, out uint sniError, bool canAccu { SqlClientEventSource.Log.TryTraceEvent("TdsParserStateObject.SNIWritePacket | Info | State Object Id {0}, Write async returned error code {1}", _objectID, (int)error); AddError(_parser.ProcessSNIError(this)); - ThrowExceptionAndWarning(); + ThrowExceptionAndWarning(false, asyncClose); } AssertValidState(); completion.SetResult(null); @@ -3569,7 +3570,7 @@ private Task SNIWritePacket(PacketHandle packet, out uint sniError, bool canAccu { SqlClientEventSource.Log.TryTraceEvent("TdsParserStateObject.SNIWritePacket | Info | State Object Id {0}, Write async returned error code {1}", _objectID, (int)sniError); AddError(_parser.ProcessSNIError(this)); - ThrowExceptionAndWarning(callerHasConnectionLock); + ThrowExceptionAndWarning(callerHasConnectionLock, asyncClose); } AssertValidState(); } @@ -3581,7 +3582,7 @@ private Task SNIWritePacket(PacketHandle packet, out uint sniError, bool canAccu internal abstract uint WritePacket(PacketHandle packet, bool sync); // Sends an attention signal - executing thread will consume attn. - internal void SendAttention(bool mustTakeWriteLock = false) + internal void SendAttention(bool mustTakeWriteLock = false, bool asyncClose = false) { if (!_attentionSent) { @@ -3623,7 +3624,7 @@ internal void SendAttention(bool mustTakeWriteLock = false) uint sniError; _parser._asyncWrite = false; // stop async write - SNIWritePacket(attnPacket, out sniError, canAccumulate: false, callerHasConnectionLock: false); + SNIWritePacket(attnPacket, out sniError, canAccumulate: false, callerHasConnectionLock: false, asyncClose); SqlClientEventSource.Log.TryTraceEvent("TdsParserStateObject.SendAttention | Info | State Object Id {0}, Sent Attention.", _objectID); } finally 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 6e8afce1ea..8d9057bc02 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 @@ -2401,9 +2401,9 @@ private void OnTimeoutAsync(object state) } } - private bool OnTimeoutSync() + private bool OnTimeoutSync(bool asyncClose = false) { - return OnTimeoutCore(TimeoutState.Running, TimeoutState.ExpiredSync); + return OnTimeoutCore(TimeoutState.Running, TimeoutState.ExpiredSync, asyncClose); } /// @@ -2412,8 +2412,9 @@ private bool OnTimeoutSync() /// /// the state that is the expected current state, state will change only if this is correct /// the state that will be changed to if the expected state is correct + /// any close action to be taken by an async task to avoid deadlock. /// boolean value indicating whether the call changed the timeout state - private bool OnTimeoutCore(int expectedState, int targetState) + private bool OnTimeoutCore(int expectedState, int targetState, bool asyncClose = false) { Debug.Assert(targetState == TimeoutState.ExpiredAsync || targetState == TimeoutState.ExpiredSync, "OnTimeoutCore must have an expiry state as the targetState"); @@ -2447,7 +2448,7 @@ private bool OnTimeoutCore(int expectedState, int targetState) { try { - SendAttention(mustTakeWriteLock: true); + SendAttention(mustTakeWriteLock: true, asyncClose); } catch (Exception e) { @@ -2988,7 +2989,7 @@ public void ReadAsyncCallback(IntPtr key, IntPtr packet, UInt32 error) // synchrnously and then call OnTimeoutSync to force an atomic change of state. if (TimeoutHasExpired) { - OnTimeoutSync(); + OnTimeoutSync(asyncClose: true); } // try to change to the stopped state but only do so if currently in the running state @@ -3475,7 +3476,7 @@ private void CancelWritePacket() #pragma warning disable 420 // a reference to a volatile field will not be treated as volatile - private Task SNIWritePacket(SNIHandle handle, SNIPacket packet, out UInt32 sniError, bool canAccumulate, bool callerHasConnectionLock) + private Task SNIWritePacket(SNIHandle handle, SNIPacket packet, out UInt32 sniError, bool canAccumulate, bool callerHasConnectionLock, bool asyncClose = false) { // Check for a stored exception var delayedException = Interlocked.Exchange(ref _delayedWriteAsyncCallbackException, null); @@ -3566,7 +3567,7 @@ private Task SNIWritePacket(SNIHandle handle, SNIPacket packet, out UInt32 sniEr SqlClientEventSource.Log.TryTraceEvent(" write async returned error code {0}", (int)error); AddError(_parser.ProcessSNIError(this)); - ThrowExceptionAndWarning(); + ThrowExceptionAndWarning(false, asyncClose); } AssertValidState(); completion.SetResult(null); @@ -3603,7 +3604,7 @@ private Task SNIWritePacket(SNIHandle handle, SNIPacket packet, out UInt32 sniEr { SqlClientEventSource.Log.TryTraceEvent(" write async returned error code {0}", (int)sniError); AddError(_parser.ProcessSNIError(this)); - ThrowExceptionAndWarning(callerHasConnectionLock); + ThrowExceptionAndWarning(callerHasConnectionLock, false); } AssertValidState(); } @@ -3613,7 +3614,7 @@ private Task SNIWritePacket(SNIHandle handle, SNIPacket packet, out UInt32 sniEr #pragma warning restore 420 // Sends an attention signal - executing thread will consume attn. - internal void SendAttention(bool mustTakeWriteLock = false) + internal void SendAttention(bool mustTakeWriteLock = false, bool asyncClose = false) { if (!_attentionSent) { @@ -3660,7 +3661,7 @@ internal void SendAttention(bool mustTakeWriteLock = false) UInt32 sniError; _parser._asyncWrite = false; // stop async write - SNIWritePacket(Handle, attnPacket, out sniError, canAccumulate: false, callerHasConnectionLock: false); + SNIWritePacket(Handle, attnPacket, out sniError, canAccumulate: false, callerHasConnectionLock: false, asyncClose); SqlClientEventSource.Log.TryTraceEvent(" Send Attention ASync.", "Info"); } finally