Optimize SqlClient query memory allocation by Wraith2 · Pull Request #34047 · dotnet/corefx (original) (raw)

This approach of splitting the code is way easier to review and digest. I personally could get through this much faster. I have left a few comments of which 2 are important in terms of behavior changes, one is about the PacketRelease having been removed and the other is the usage of _connHandler instead of connHandler property.
My confidence in this PR w.r.t. EF tests is quite high.

@Wraith2 You mention that

the tests cannot be successfully run in managed mode due to

I haven't faced this issue. I am using the Release build, built locally and I haven't had to build a Nuget package to consume this change. Honestly, I am building the dotnet/corefx code after a long time and I did setup everything from scratch except for installing Visual Studio.

I understand that you haven't been able to test on Linux and you have been testing by flipping the ManagedSNI flag on Windows and that is causing issues as well. Testing with Managed SNI on Windows hasn't been great in terms of gaining confidence in Managed SNI code for Linux. (SNI is the networking interface part of SqlClient which is implemented in sni.dll for Windows and System.Data.SqlClient.SNI namespace for Unix)

The reason for the lack of reliability is because the networking stack of .Net Core on Windows Unix and macOS are different. Ideally the author of SqlClient shouldn't have to worry about it, however in past while working on SqlClient on .Net Core Linux, we have uncovered problems with networking stack because of test execution on Linux. We have found 2 kinds of issues, the first kind (and many) in SqlClient where we should have handled certain failures and we were not, and the second is that we occasionally discovered issues in the networking stack of Core.

I have been running EF tests (ran a few tests earlier this afternoon) with a Release build of SqlClient (build on Ubuntu 1804) against the Debug build of EFCore.SqlServer provider by replacing the DLL with the change in this PR and all worked well.

I haven't hit any asserts and so shouldn't you or anyone else, with the Release build of SqlClient. I will post a bit more about these asserts and how the code has been structured, on the issue #33930 (perhaps in the next couple of days)

Since you mentioned that there is a good amount of learning curve involved with Linux testing for you, I expect that you will take sometime to get this right. However we could work together on this. You could ping me on my email address on the Github profile and I could walk you through the steps there. Trying to keep the infrastructure issues separate from code review discussions.