Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Fixed ReadAsync blocking issue #26595

Merged
merged 5 commits into from
Jan 31, 2018
Merged

Fixed ReadAsync blocking issue #26595

merged 5 commits into from
Jan 31, 2018

Conversation

geleems
Copy link

@geleems geleems commented Jan 26, 2018

This fix is for the issue https://github.com/dotnet/corefx/issues/26594


Customer reported an issue for SqlDataReader.ReadAsync() method actually runs synchronously, and it blocks the calling thread until data is fed from SQL Server.

@@ -2499,9 +2499,6 @@ private bool TryProcessDone(SqlCommand cmd, SqlDataReader reader, ref RunBehavio
ushort status;
int count;

// Can't retry TryProcessDone
stateObj._syncOverAsync = true;
Copy link
Member

Choose a reason for hiding this comment

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

Why this change specifically? Framework TdsParser also has this line

Copy link
Contributor

Choose a reason for hiding this comment

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

The comment // Can't retry TryProcessDone seems interesting. Looks like syncOverAsync was set to true to prevent a retry of processing the Done TDS token. Have you checked if this "retry" is not invoked after your changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

@corivera TryProcessDone sets syncOverAsync during async execution which makes the execution sync for pretty much everything, since syncOverAsync forces a blocking network call. TryProcessDone was turning out to be a bottleneck that @geleems figured out.

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked, and TryProcessDone() was not retried after my change. However, I have just become curious what happens if I force it to retry TryProcessDone() by following code:

I meant , are there code paths which could retry Done token processing in certain scenarios for a single SqlDataReader Read operation, instead of multiple Read() operations, which needs to be prevented?

Copy link
Author

Choose a reason for hiding this comment

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

@saurabh500
TryProcessDone() runs multiple times until ReadSni() actually returns packets, and it is processed by TryProcessDone(). Once DONE token is processed, codeflow moves forward, and there is no code path to RETRY the DONE token processing, which is already done.

@@ -0,0 +1,107 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
Copy link
Contributor

Choose a reason for hiding this comment

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

This test could be moved to AsyncTest.cs

public class ReadAsyncTest
{
[CheckConnStrSetupFact]
public static void Run()
Copy link
Contributor

Choose a reason for hiding this comment

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

Provide a more descriptive name for this Run() method.

{
double elapsedAsync = await RunReadAsync();
double elapsedSync = RunRead();
Assert.True(elapsedAsync < (elapsedSync / 2.0d));
Copy link
Contributor

Choose a reason for hiding this comment

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

The test Asserts that Async should take half the time taken by Sync.
This doesn't seems right. Async should take close to 0 ms.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can this assert be improved?

Copy link
Author

Choose a reason for hiding this comment

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

It took about 15~30ms in my environment for asynchronous operation.
For synchronous read, it took about 2000ms.

Copy link
Author

@geleems geleems Jan 26, 2018

Choose a reason for hiding this comment

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

I checked, and TryProcessDone() was not retried after my change. However, I have just become curious what happens if I force it to retry TryProcessDone() by following code:

Task<bool> t1, t2;
do
{
    t1 = reader.ReadAsync();
    t2 = reader.ReadAsync();
}
while (await t1 && await t2);

Copy link
Contributor

Choose a reason for hiding this comment

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

It took about 15~30ms in my environment for asynchronous operation.
For synchronous read, it took about 2000ms.

Based on this can you provide a better Assert for this test.
My concern is that if ReadSync takes 30 seconds in some cases, then ReadAsync should still take ~30 ms. So a proportionate comparison with the time taken by ReadAsync and ReadSync doesn't make sense

Copy link
Contributor

Choose a reason for hiding this comment

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

Task t1, t2;
do
{
t1 = reader.ReadAsync();
t2 = reader.ReadAsync();
}
while (await t1 && await t2);

This Code will not retry Done token processing in the middle of the TDS stream.

@@ -2499,9 +2499,6 @@ private bool TryProcessDone(SqlCommand cmd, SqlDataReader reader, ref RunBehavio
ushort status;
int count;

// Can't retry TryProcessDone
stateObj._syncOverAsync = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment // Can't retry TryProcessDone seems interesting. Looks like syncOverAsync was set to true to prevent a retry of processing the Done TDS token. Have you checked if this "retry" is not invoked after your changes?

@@ -2499,9 +2499,6 @@ private bool TryProcessDone(SqlCommand cmd, SqlDataReader reader, ref RunBehavio
ushort status;
int count;

// Can't retry TryProcessDone
stateObj._syncOverAsync = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

@corivera TryProcessDone sets syncOverAsync during async execution which makes the execution sync for pretty much everything, since syncOverAsync forces a blocking network call. TryProcessDone was turning out to be a bottleneck that @geleems figured out.

@geleems
Copy link
Author

geleems commented Jan 26, 2018

What causes this issue?

The issue resides in both .NET Framework and .NET Core.
Here is the stack trace of main thread when it was blocking after ReadAsync() call.

System.Data.dll!SNINativeMethodWrapper::SNIReadSyncOverAsync(System::Runtime::InteropServices::SafeHandle^ pConn, __int64 packet, int timeout) Line 1113 + 0xc bytes             C++
System.Data.dll!System.Data.SqlClient.TdsParserStateObject.ReadSniSyncOverAsync() Line 1957 + 0x18 bytes                C#
System.Data.dll!System.Data.SqlClient.TdsParserStateObject.TryReadNetworkPacket() Line 1913 C#
System.Data.dll!System.Data.SqlClient.TdsParserStateObject.TryPrepareBuffer() Line 1075 + 0x8 bytes    C#
System.Data.dll!System.Data.SqlClient.TdsParserStateObject.TryReadByteArray(byte[] buff, int offset, int len, out int totalRead) Line 1227 + 0x8 bytes C#
System.Data.dll!System.Data.SqlClient.TdsParserStateObject.TryReadInt64(out long value) Line 1401 + 0x23 bytes     C#
System.Data.dll!System.Data.SqlClient.TdsParser.TryProcessDone(System.Data.SqlClient.SqlCommand cmd, System.Data.SqlClient.SqlDataReader reader, ref System.Data.SqlClient.RunBehavior run, System.Data.SqlClient.TdsParserStateObject stateObj) Line 2698 + 0xc bytes        C#
System.Data.dll!System.Data.SqlClient.TdsParser.TryRun(System.Data.SqlClient.RunBehavior runBehavior, System.Data.SqlClient.SqlCommand cmdHandler, System.Data.SqlClient.SqlDataReader dataStream, System.Data.SqlClient.BulkCopySimpleResultSet bulkCopyHandler, System.Data.SqlClient.TdsParserStateObject stateObj, out bool dataReady) Line 1986 + 0x25 bytes     C#
System.Data.dll!System.Data.SqlClient.SqlDataReader.TryHasMoreRows(out bool moreRows) Line 2887 + 0x31 bytes          C#
System.Data.dll!System.Data.SqlClient.SqlDataReader.TryReadInternal(bool setTimeout, out bool more) Line 3171 + 0xd bytes              C#
System.Data.dll!System.Data.SqlClient.SqlDataReader.ReadAsync.AnonymousMethod__0(System.Threading.Tasks.Task t) Line 4292 + 0x18 bytes              C#
System.Data.dll!System.Data.SqlClient.SqlDataReader.InvokeRetryable<bool>(System.Func<System.Threading.Tasks.Task,System.Threading.Tasks.Task<bool>> moreFunc, System.Threading.Tasks.TaskCompletionSource<bool> source, System.IDisposable objectToDispose) Line 4625 + 0x9 bytes              C#
System.Data.dll!System.Data.SqlClient.SqlDataReader.ReadAsync(System.Threading.CancellationToken cancellationToken) Line 4318 + 0x11 bytes           C#
System.Data.dll!System.Data.Common.DbDataReader.ReadAsync() Line 243 + 0x15 bytes              C#

Default value of TdsParserStateObject._syncOverAsync is True. ReadAsync() can be done asynchronously only when the value is set to False :
https://github.com/dotnet/corefx/blob/master/src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObject.cs#L2030-L2036

When SqlDataReader.ReadAsync() is called, TdsParserStateObject._syncOverAsync is set to False for asynchronous operation :
https://github.com/dotnet/corefx/blob/master/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlDataReader.cs#L4798

But TdsParserStateObject._syncOverAsync is rolled back to True when the code flow hits TdsParser.TryProcessDone() : https://github.com/dotnet/corefx/blob/master/src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs#L2503

Look at the stack trace again:

SNINativeMethodWrapper::SNIReadSyncOverAsync()
TdsParserStateObject.ReadSniSyncOverAsync()
TdsParserStateObject.TryReadNetworkPacket()
TdsParserStateObject.TryPrepareBuffer()
TdsParserStateObject.TryReadByteArray()
TdsParserStateObject.TryReadInt64()
TdsParser.TryProcessDone() <--------------------------------_syncOverAsync is set back to True
TdsParser.TryRun()
SqlDataReader.TryHasMoreRows()
SqlDataReader.TryReadInternal()
SqlDataReader.ReadAsync.AnonymousMethod__0()
SqlDataReader.InvokeRetryable<bool>()
SqlDataReader.ReadAsync() <---------------------------------_syncOverAsync is set to False
System.Data.Common.DbDataReader.ReadAsync()

As result, TdsParserStateObject.ReadSniSyncOverAsync() is executed instead of TdsParserStateObject.ReadSni(). And ReadSniSyncOverAsync() always proceed operation synchronously if the protocol is TCP.

using (SqlCommand command = connection.CreateCommand())
{
command.CommandText = commandText;
using (SqlDataReader reader = command.ExecuteReader())
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use the async equivalent of ExecuteReader to make this method async. The same applies to connection.Open()

@stephentoub
Copy link
Member

Why does this _syncOverAsync thing exist at all? Why was it added in the first place? Can we entirely delete it?

@@ -36,7 +37,7 @@ public static void ExecuteTest()
con.Close();
}

[CheckConnStrSetupFact]
//[CheckConnStrSetupFact]
Copy link
Contributor

Choose a reason for hiding this comment

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

You may want to clean this method and the one above. Looks like they are not used and the [Fact] attribute has been removed

CompareReadSyncAndReadAsync();
}

private static async void CompareReadSyncAndReadAsync()
Copy link
Contributor

Choose a reason for hiding this comment

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

This method doesn't need to be async

+ " DROP TABLE #y"
+ " END"
+ " SELECT 'b'";
double elapsedAsync = await RunReadAsync(sql);
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to wait on the task returned by RunReadAsync(sql);

+ " SELECT 'b'";
double elapsedAsync = await RunReadAsync(sql);
double elapsedSync = RunReadSync(sql);
Assert.True(elapsedAsync < (elapsedSync / 2.0d));
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you plan to improve this assert in future commits?

@saurabh500
Copy link
Contributor

@stephentoub It seems like when the networking layer of SqlClient was changed in the framework (long ago), it was decided that Async APIs following the Begin and End pattern would use IOCP at the networking layer to make the calls so that the calls are not blocking. This behavior was put behind a flag called Async=true/false
At the same time the Sync calls were ported to be Sync over these Async network calls.
I found this blog https://blogs.msdn.microsoft.com/angelsb/2004/09/02/ado-net-2-0-asynchronous-command-execution-async-faq/ which has some answers.

@stephentoub
Copy link
Member

Thanks. After this fix, do any async paths still hit sync over async paths? And is this a contributor to the issue that made you make long running tasks, and if so, can we undo that now?

@geleems
Copy link
Author

geleems commented Jan 30, 2018

@stephentoub
The spot I removed was the only place that _syncOverAsync variable is consumed for synchronous processing. So..no.. there is no other path. :)

[CheckConnStrSetupFact]
public static void ExecuteTest()
public static void TestReadAsync()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be changed to TestReadAsyncTimeConsumed

double elapsedSync = RunReadSync(sql);
t.Wait();
double elapsedAsync = t.Result;
Console.WriteLine("Asynchronous Operation: " + elapsedAsync + "ms");
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove Console statements.

@geleems
Copy link
Author

geleems commented Jan 31, 2018

@saurabh500 Manual tests were also passed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants