Initial Commit of Transport Creation Handler

[benrr101]

Description: Handler that creates the transport for a connection request chain of responsibility. It only works for TCP connections right now. IP addresses are looked up, if there is a preference for IPv4 or IPv6, the list is sorted and each one is tried until one connects or all fail.

Exceptions still need to be properly handled.

Testing: Everything builds :)

[edwardneal]

A few performance comments, but I'm looking forward to seeing how this pans out!

[edwardneal]

This seems like it'd introduce sync-over-async via Dns.GetHostAddressesAsync and OpenSocketAsync. There are some situations (MultiSubnetFailover == Enabled) where that's unavoidable because we need to connect to all IP addresses in parallel, but in most cases it's possible to open a connection synchronously without waiting on the thread pool.

[benrr101]

I spent sometime reading up on this and digging through the code for the calls, but I'm not sure I see the sync-over-asyn anti-pattern going on. It looks like Dns.GetHostAddressesAsync isn't really async, but Socket.ConnectAsync is definitely async. I could change the call to get the addresses to use the sync method, but I think we're justified in using async for connecting. It sounds like you might know more about this than I do, so please let me know if I'm missing something.

This might also be a time for using one of those ValueTasks I've heard so much about 😅

[edwardneal]

I'm thinking of a case where we call the method with isAsync set to false. Because isAsync is never passed down to HandleTcpRequest, it's effectively always asynchronous. A caller who waits on Handle is wrapping a synchronous operation around async operations occurring on the thread pool (sync-over-async.) Your points around Dns.GetHostAddressesAsync and Socket.ConnectAsync are absolutely correct, but protect against thread-level blocking within a task (async-over-sync.)

There's a minor (but immediate) performance hit because we might now allocate Tasks. There's also a more subtle one which is a pain point for the current iteration of SqlClient, which is that if the thread pool is heavily loaded it could be some time until the thread pool has enough capacity to start the work. A caller wouldn't necessarily expect that if MultiSubnetFailover is disabled. In the worst case, both sync-over-async and async-over-sync can cause deadlocks.

[saurabh500]

I think the Sync path would need a separate flow where the sync versions of DNS and sockets are used, rather than relying on the async overloads. Likely in this PR it would make sense to throw or add a TODO to say Sync is to be supported.

For MSF in sync mode, sync over async will suffer from the problems @edwardneal mentioned. However a potential path forward for sync mode is to open all sockets in non-blocking mode, and later check using Socket.Select to see which one is writeable for the timeout duration, and return that particular socket.
This way the I/O is used for socket open parallelization, and we dont rely on sync over async.

[edwardneal]

Either a throw or a TODO is fine by me, so long as the problem's raised explicitly rather than implicitly - getting the correct behaviour from various scenarios of MSF/TNIR/connection timeouts would probably be better handled separately.

[saurabh500]

For sync cases, the callers will provide a default Cancellation token. the ct.ThrowIfCancellationRequested() may not be needed in sync paths.

[benrr101]

I would like to utilize the cancellation token as the way to signal that either the timeout has expired or the user has requested cancellation. This will be adopted in a later PR. As such, I think it would still be valuable to keep the cancellation token in the sync code path. @edwardneal had a good suggestion for how to cancel while the socket is opening by registering event on the cancellation token.

[saurabh500]

I am still not sure how this would look end to end. If this theory holds, then great, else we will modify.

[benrr101]

CI build didn't kick off. It builds locally and unit tests work, so I think it's fine. Feel free to annoy me if I broke branch.