Runtime: Reduce blocking calls throughout SqlConnection.OpenAsync() and other async APIs on SqlClient

[divega]

This is a follow up to the discussion at https://github.com/dotnet/corefx/issues/25620#issuecomment-349428958.

TL;DR

the managed SNI implementation in SqlClient contains code that performs the DNS resolution and initiates the socket connection in a blocking manner. Ideally we should replace this with proper async code.

Details

We now know about these cases in the constructor of https://github.com/dotnet/corefx/blob/master/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNITcpHandle.cs and we know that it impacts OpenAsync. But there could be other APIs impacted and other parts of the implementation that use blocking calls.

Currently the code in the constructor of SNITcpHandle is actually invoking the async versions of the methods (Dns.GetHostAddressesAsync() and Socket.ConnectAsync()) but then they are blocking on Task.Result.

@saurabh500 has proposed that we switch to the sync versions of those APIs in the constructor as part of the fix for dotnet/runtime#24301 in the short term.

However in the long term we should look into refactoring the code out of the constructor (e.g. defer the work to a EnsureConnectedAsync() private method) so that I can be made proper async.

[saurabh500]

This will require changes to a bunch of components and hence moving to post 2.1

[MNie]

@saurabh500 any update on this :)?

[igor-brylin]

@saurabh500 @divega any update on this?

[divega]

No update from my side. Pinging @saurabh500 @David-Engel in case they have an answer, and @Wraith2 in case he is interested.

[David-Engel]

@divega Our team doesn't have cycles to burn down Future items at the moment. This does sound like a nice clean up job, though, if anyone else wants to pick it up.

[Wraith2]

I've had a look through the code and it looks feasible to change it around so a connection task is initiated in the ctor and then the task status checked and possibly waited later whenever the Status property is read. From my readthrough that should affect all paths.

I've found some things in the code that prompt questions though.

private static async void ParallelConnectHelper

• Is a void async method which is a "Bad Thing(TM)" but might be technically safe, I'm not experienced with async to be able to tell by inspection. I think since it awaits the task passed in and that the task is awaited in the call but means that any exception from the task is always observed. But, the call itself isn't awaited so runs synchronously and is safe. If the intention is to farm out 64 parallel connections wouldn't using Task.Factory.StartNew with DenyChildAttach be appropriate to prevent them being run synchronously?
• I think the lastError variable is subject to a race condition. If you have 64 parallel tasks opening connections then it's possible that multiple of them fail and can be at different execution points in this method meaning that lastError can be set by another task after the first one fails, so you'll get the last error that occurs on that
• I don't think StongBox is needed since exception is a reference type. In fact I don't understand why the exception is passed through a parameter because it isn't used in the parent scope. It could just be a local variable which would be entirely safe.
• pendingCompleteCount is setup and decremented but never used. I think it should be used to guard returning until one connection has succeeded or all of them have failed. I not sure why it currently works unless the connection tasks are run synchronously.

/cc @stephentoub master of all things async

SniTCPHandle.ctor always creates an sslstream whether you want one or not which seems wasteful if you aren't going to be using it, ssl stream is not cheap.

[saurabh500]

I have done a deep dive of why SqlConnection.OpenAsync is not blocking at https://github.com/dotnet/corefx/pull/36667#issuecomment-480651762

Let me know if I can answer any questions. When this issue was opened, I didn't have great clarity on the Async aspect of SqlConnection.OpenAsync(). However after much of code reading, I can safely conclude that the network calls being sync in the code being referenced here is correct. More details in the PR thread above.

Feel free to re-open this issue in case there are any concerns or questions or suggestions.

Edit: ~network calls being async~ -> network calls being sync

[saurabh500]

@Wraith2

SniTCPHandle.ctor always creates an sslstream whether you want one or not which seems wasteful if you aren't going to be using it, ssl stream is not cheap.

TDS protocol 7.x needs the Login to happen over an encrypted channel. There is no getting away from it if connectivity is needed for SQL server. If encryption is not requested by client or not overridden by the server, then we ditch the SSLStream and move to an unencrypted channel.

[saurabh500]

private static async void ParallelConnectHelper

About this, this connection is invoked when we are connecting using a connection string which needs MultiSubnetFailover = true where it is expected that the sql server is part of an AG and the hostname can resolve to multiple subnets.

I dont like how ParallelConnectHelper is done, because it ends up using the threads from the app thread pool. In case of Native SNI the parallel connect utilizes the IO thread, but with .Net apis there is no way to enforce that in managed SNI (unless I don't know of any improvements made in this area)

[Wraith2]

Possibly a custom task scheduler, I'll put it on my list of things to look into.