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.

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

Revert change that made SqlDataReader.ReadAsync() non-blocking #547

Merged
merged 10 commits into from
May 1, 2020

Conversation

cheenamalhotra
Copy link
Member

This PR reverts dotnet/corefx#26595 and #490 (attempted to fix MARS TDS header issues introduced with dotnet/corefx#26595) with additional tests.

This is an alternate solution to fix #542 but due to #544 and #545 failing intermittently on Unix w. Managed SNI, it doesn't seem an appropriate fix either.

This revert is likely to hit performance by re-introducing old behavior of SqlDataReader.ReadAsync() method to run synchronously, and will block the calling thread until data is fed from SQL Server. But given the side-effects of the original change, reverting seems most appropriate at this point and a proper investigation would be needed to be done for original performance issue.

@cheenamalhotra cheenamalhotra merged commit c489f19 into dotnet:master May 1, 2020
@ErikEJ
Copy link
Contributor

ErikEJ commented May 1, 2020

@cheenamalhotra still planning a 2.0 preview3 release soon?

@kornelpal
Copy link

After changing the target framework to .NET Framework 4.8, we started to encounter random MARS errors even though we are not using MARS: The incoming tabular data stream (TDS) protocol stream is incorrect. The MARS TDS header contained errors.

Unfortunately there is extremely limited amount of information available, and took us several days to figure out what is happening. The comment of @David-Engel on issue #85 was most helpful to us.

Although most official documentation is not listing the change, I remembered that .NET Framework 4.8 Release Notes mentions that SqlDataReader.ReadAsync() now runs asynchronously that was the next useful pointer.

Unfortunately the "Switch.Microsoft.Data.SqlClient.MakeReadAsyncBlocking" AppContext switch did not work on .NET Framework 4.8, and I could not find any relevant documentation about it either. While the searchable .NET Framework Reference Source and the referencesource repository do not include the relevant LocalAppContextSwitches class, luckily the downloadable version contains the missing LocalAppContextSwitches.cs file that helped figuring out the correct workaround:
AppContext.SetSwitch("Switch.System.Data.SqlClient.MakeReadAsyncBlocking", true);

Note that AppContextDefaultValues.Defaults.cs from the downloadable Reference Source shows that targeting .NET Framework 4.8 changes the default value from true to false.

Since enabling the AppContext switch, we have not seen any more MARS errors, but apparently we are using an undocumented switch. Although we haven't tried those, other possible workarounds are targeting a version earlier than .NET Framework 4.8 or not using SqlDataReader.ReadAsync().

@cheenamalhotra and @David-Engel, I believe that .NET Framework 4.8 is also affected by this issue (#547). Could you please share whether you have any plans of backporting this fix/revert to .NET Framework 4.8?

@cheenamalhotra
Copy link
Member Author

@kornelpal

Do you have a repro code available that we can try to reproduce what you observed?
Our test case (AsyncCancelledConnectionsTest) that was added specifically for this issue in .NET Core does not reproduce the error in .NET Framework 4.8.

@kornelpal
Copy link

I could easily reproduce the issue with your test code. I run it in a standalone console application, but I made trivial changes only to remove dependency on the unit testing framework.

Your AppDomain has to target .NET Framework 4.8, not just run on .NET Framework 4.8 (or the .dll target it) to have the switch disabled by default. AppContext.TargetFrameworkName returns ".NETFramework,Version=v4.8" in this case. Your unit test framework might not target .NET Framework 4.8.

Alternatively, you can disable the switch when running on .NET Framework 4.8, but not targeting it:
AppContext.SetSwitch("Switch.System.Data.SqlClient.MakeReadAsyncBlocking", false);

@cheenamalhotra
Copy link
Member Author

cheenamalhotra commented Oct 27, 2020

Hi @kornelpal

Yes I can reproduce issue with System.Data.SqlClient (default behavior, no custom switches).
I was testing initially with Microsoft.Data.SqlClient that does not reproduce this issue by default, but only when setting below switch:

AppContext.SetSwitch("Switch.Microsoft.Data.SqlClient.MakeReadAsyncBlocking", false);

You can find documentation of App Context switches for Microsoft.Data.SqlClient here: https://docs.microsoft.com/en-us/sql/connect/ado-net/appcontext-switches?view=sql-server-ver15. I agree the System.Data.SqlClient switches are not documented well, we'll update our documentation to provide notes for users for transferred switches.

In regards to System.Data.SqlClient, since the product is no longer actively maintained but only receives high security issue updates, I'm afraid the default behavior of S.D.S is likely not going to change. I'd recommend you to transition to Microsoft.Data.SqlClient as we actively maintain and deliver new releases and bug fixes here now.

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

Successfully merging this pull request may close these issues.

SqlDependency.Stop() in Microsoft.Data.SqlClient 1.1.2 doesn't remove stored procedure
5 participants