Fix | Change IP connection logic in SNITCPHandle

[karinazhou]

This PR changes the logic in SNITCPHandle:Connect() when doing socket connection. This is to keep consistency with the driver's behavior in .NET Framework application.

Originally, the driver only keeps the last IPv4 address and the last IPv6 address if appeared. The driver goes through these two IPs and sees if any could connect successfully. This logic is different from the one in .NET Framework version where the driver tries each valid IPv4 address and followed by IPv6 addresses in a row until a successful connection is found.

[jinek]

Good point about difference with .NET, but this changes breaks the initial logic of this code: 2 sockets are created for 2 IP addresses, and then cancellation method cancels only 2 sockets.

With this changes:

1. Timeout does not work anymore if there are more than two ip addresses. That is because Cancel method in best scenario cancels only two sockets while that double loop continue two create and assign new Sockets
2. Because of double loop continue to create and assign the sockets they stay not disposed.

The original logic did not work in many cases either, for example, if cancellation happened before the loop, still this changes introduce new issues.

[jinek]

It also changes the logic of
pendingDNSInfo = new SQLDNSInfo(cachedFQDN, IPv4String, IPv6String, port.ToString());
which now happens only if one of the socket has been connected while previously it happened if we have one of the address.
I don't know whether that's intended by the topic, @karinazhou could you, please, check?

[cheenamalhotra]

@jinek
I'm investigating further.

[cheenamalhotra]

@jinek I think 1 change that should be made is the array size of Sockets. It can be made equal to the ipaddresses list we retrieve from DNS.GetHostAddresses.

However, it's tough to reproduce it practically to see a DNS resolving to more than 2 IPs at any time since it requires advanced networking configurations, but it is possible, and in times of failover this can actually cause issues. Fixing the size should address Timeout / cancellation and any potential Array out of bounds errors. I'm working on a change right away to fix this possibility.

Because of double loop continue to create and assign the sockets they stay not disposed.

These sockets are connected sequentially, so if first socket fails to connect, it's disposed right there before starting with second IP in the list. They're not connected in parallel so we're ensuring they're disposed if not connected. I'm not considering cancelation sync scenarios which I've mentioned later. Let me know if I missed something else.

It also changes the logic of
pendingDNSInfo = new SQLDNSInfo(cachedFQDN, IPv4String, IPv6String, port.ToString());
which now happens only if one of the socket has been connected while previously it happened if we have one of the address.

This is acceptable since we don't need IPs in pending cache before we could connect and cache is not utilized during first connect that's not already cached before.

The original logic did not work in many cases either, for example, if cancellation happened before the loop, still this changes introduce new issues.

Yes, if cancellation happens before loop, there's a risk of opening a socket right after processing cancel and I agree this is an existing issue. I don't however see any new issues. I'll review your PR later this week so we can address this issue too.

[jinek]

@cheenamalhotra Yes, correct, they are disposed, I missed "else" block.
Still new issue is timeout now does not work.
Previously, method Cancel would dispose all both sockets. But now even after disposing some two, others still continue to connect