Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix async deadlock issue when sending attention fails due to network failure #1766

Merged
merged 1 commit into from Oct 4, 2022
Merged

Fix async deadlock issue when sending attention fails due to network failure #1766

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 13 additions & 12 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs
View file
Open in desktop
Expand Up @@ -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.
Expand Down Expand Up @@ -2337,9 +2337,9 @@ private void OnTimeoutAsync(object state)
}
}

private bool OnTimeoutSync()
private bool OnTimeoutSync(bool asyncClose = false)
cheenamalhotra marked this conversation as resolved.
Show resolved Hide resolved
{
return OnTimeoutCore(TimeoutState.Running, TimeoutState.ExpiredSync);
return OnTimeoutCore(TimeoutState.Running, TimeoutState.ExpiredSync, asyncClose);
}

/// <summary>
Expand All @@ -2348,8 +2348,9 @@ private bool OnTimeoutSync()
/// </summary>
/// <param name="expectedState">the state that is the expected current state, state will change only if this is correct</param>
/// <param name="targetState">the state that will be changed to if the expected state is correct</param>
/// <param name="asyncClose">any close action to be taken by an async task to avoid deadlock.</param>
/// <returns>boolean value indicating whether the call changed the timeout state</returns>
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");

Expand Down Expand Up @@ -2382,7 +2383,7 @@ private bool OnTimeoutCore(int expectedState, int targetState)
{
try
{
SendAttention(mustTakeWriteLock: true);
SendAttention(mustTakeWriteLock: true, asyncClose);
}
catch (Exception e)
{
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
{
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
}
Expand All @@ -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)
{
Expand Down Expand Up @@ -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
Expand Down
21 changes: 11 additions & 10 deletions src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs
View file
Open in desktop
Expand Up @@ -2401,9 +2401,9 @@ private void OnTimeoutAsync(object state)
}
}

private bool OnTimeoutSync()
private bool OnTimeoutSync(bool asyncClose = false)
cheenamalhotra marked this conversation as resolved.
Show resolved Hide resolved
{
return OnTimeoutCore(TimeoutState.Running, TimeoutState.ExpiredSync);
return OnTimeoutCore(TimeoutState.Running, TimeoutState.ExpiredSync, asyncClose);
}

/// <summary>
Expand All @@ -2412,8 +2412,9 @@ private bool OnTimeoutSync()
/// </summary>
/// <param name="expectedState">the state that is the expected current state, state will change only if this is correct</param>
/// <param name="targetState">the state that will be changed to if the expected state is correct</param>
/// <param name="asyncClose">any close action to be taken by an async task to avoid deadlock.</param>
/// <returns>boolean value indicating whether the call changed the timeout state</returns>
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");

Expand Down Expand Up @@ -2447,7 +2448,7 @@ private bool OnTimeoutCore(int expectedState, int targetState)
{
try
{
SendAttention(mustTakeWriteLock: true);
SendAttention(mustTakeWriteLock: true, asyncClose);
}
catch (Exception e)
{
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -3566,7 +3567,7 @@ private Task SNIWritePacket(SNIHandle handle, SNIPacket packet, out UInt32 sniEr
SqlClientEventSource.Log.TryTraceEvent("<sc.TdsParser.WritePacket|Info> write async returned error code {0}", (int)error);

AddError(_parser.ProcessSNIError(this));
ThrowExceptionAndWarning();
ThrowExceptionAndWarning(false, asyncClose);
}
AssertValidState();
completion.SetResult(null);
Expand Down Expand Up @@ -3603,7 +3604,7 @@ private Task SNIWritePacket(SNIHandle handle, SNIPacket packet, out UInt32 sniEr
{
SqlClientEventSource.Log.TryTraceEvent("<sc.TdsParser.WritePacket|Info> write async returned error code {0}", (int)sniError);
AddError(_parser.ProcessSNIError(this));
ThrowExceptionAndWarning(callerHasConnectionLock);
ThrowExceptionAndWarning(callerHasConnectionLock, false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cheenamalhotra Did we purposely call ThrowExceptionAndWarning with the asyncClose parameter hard coded to false here? In the same line in netcore, we passed the local value of asyncClose to the same parameter. (Line 3573 in the netcore TdsParserStateObject.cs file above in this PR.)

This seems like a mistake and I've changed this line to match netcore in PR #2073.

}
AssertValidState();
}
Expand All @@ -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)
{
Expand Down Expand Up @@ -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("<sc.TdsParser.SendAttention|{0}> Send Attention ASync.", "Info");
}
finally
Expand Down