Fix | SqlClient-826 Missed synchronization

[jinek]

Fixes #826

Internal method SNITCPHandle.Connect now uses Socket.Select to connect the socket with a timeout or without

Additionally:

• Exceptions swallowing removed to satisfy CA1031 https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1031
• InternalException refactored to satisfy ca1032 https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1032 (Best practices https://docs.microsoft.com/en-us/dotnet/standard/exceptions/best-practices-for-exceptions#include-three-constructors-in-custom-exception-classes )
• InternalException constructor has been changed to public as the class is not marked as sealed.
• Logic of Fix | Change IP connection logic in SNITCPHandle #1016 refactored for simplicity and to avoid mentioned issues.
• Check for null of ipAddresses removed as Dns.GetHostAddresses does not return null

[jinek]

@cheenamalhotra I can't add label "Managed SNI". Could you, please? This comment can be removed then

[JRahnama]

@jinek we have PR #1016 in progress. There might be some conflicts between these two changes. If that happens, please wait for that PR to merge and pull the changes again.

[jinek]

@JRahnama I've took into account that PR and merged it. However I allowed myself to slightly fix that PR while keeping the idea of it.
@karinazhou Could you please check this PR to be sure I adopted your idea correctly.

[JRahnama]

@jinek thanks for contributing into this repository, We have a release scheduled for this week and pretty busy with release activities. We will get back to this next week when we have more time.
Thanks again.

[jinek]

I've updated the PR, @DavoudEshtehari .
But, requesting additional attention to two things:

1. Here additional improvement can be done. For infinite timeout case we can avoid exception throwing which would give small performance boost, but code would become more complex. Should I add the code for infinite timeout case (it's about 3 additional if blocks)?
2. I've removed the check of Dns.GetHostAddresses for returning null as seems it does not ever return null. However it's not declared by the documentation of the method and I don't understand where it is supposed always return an array or no. Should I brind back the code considering it returning null? (probably just adding one redundent if block)

[DavoudEshtehari]

I ran the failed pipelines one more time, and the result remained the same. The reason for these failures as I know is the execution time exceeds the allowed 1 hour. First of all, we need to verify in which situations it happens; Maybe running a benchmark test could help us too!

1. Here additional improvement can be done. For infinite timeout case we can avoid exception throwing which would give small performance boost, but code would become more complex. Should I add the code for infinite timeout case (it's about 3 additional if blocks)?

Feel free to add your suggested change for further discussions.

2. I've removed the check of Dns.GetHostAddresses for returning null as seems it does not ever return null. However it's not declared by the documentation of the method and I don't understand where it is supposed always return an array or no. Should I brind back the code considering it returning null? (probably just adding one redundent if block)

Right, I checked the source code of the Dns.GetHostAddresses, it won't return null in case of a free-error execution. So, as you mentioned, null-checking is not necessary.

[jinek]

I ran the failed pipelines one more time, and the result remained the same. The reason for these failures as I know is the execution time exceeds the allowed 1 hour. First of all, we need to verify in which situations it happens; Maybe running a benchmark test could help us too!

I have checked the issue: here the connection timeout is set to almost 3 hours (10000 seconds) :

SqlClient/src/Microsoft.Data.SqlClient/tests/ManualTests/AlwaysEncrypted/TestFixtures/Setup/CertificateUtility.cs

Line 298 in d65dc86

builder.ConnectTimeout = 10000;

Same time SqlInternalConnectionTds runs retry for all random errors:

SqlClient/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs

Line 1618 in 2ddf3a3

if (timeout.MillisecondsRemaining <= sleepInterval) {

In my case the error is "Arithmetic operation resulted in an overflow." because 10000 seconds can not be passed to Socket.Select as it requires the value in microseconds int32: https://github.com/dotnet/runtime/blob/9ce467fe6164e781b67897615ff7dd01820084ff/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs#L2199

@DavoudEshtehari
The question now is if it is fine to limit Connection Timeout to 35 minutes (int.Max microseconds) or must we keep 35000 minutes (int.Max seconds)?
If we have to keep possibility of the connection timeout to be more than 35 minutes than I can implement "async" alternative as mentioned here #826 (comment)

[jinek]

Also fixes this: #583

[cheenamalhotra]

Looking at the SocketPal.Select API, it seems they could do well with a long type of microseconds parameter, but because it's int. we have to limit our timeout? (Timeout in milliseconds should always be long/double IMHO)

There's also an open API Proposal from 6 years ago, you might want to ask dotnet/runtime teams to resurrect: dotnet/runtime#14336

Connection timeout cannot be limited as that would be a major issue for customer applications (we know of) running high loads of work and setting timeout to as long as 2 hours.

[jinek]

Looking at the SocketPal.Select API, it seems they could do well with a long type of microseconds parameter, but because it's int. we have to limit our timeout? (Timeout in milliseconds should always be long/double IMHO)

There's also an open API Proposal from 6 years ago, you might want to ask dotnet/runtime teams to resurrect: dotnet/runtime#14336

Connection timeout cannot be limited as that would be a major issue for customer applications (we know of) running high loads of work and setting timeout to as long as 2 hours.

Right right, but all this options are time consuming?
Does socket.Connect really have infinite or any big timeout? Im thinking may be it is already limited, i would just play with the math to leave it as is.
Otherwise, i can bring Dispose similar as it was before, but with synchronisation.

[codecov]

Codecov Report

Patch coverage: 90.41% and project coverage change: -0.18 ⚠️

Comparison is base (91694d8) 70.80% compared to head (82ad1c0) 70.63%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1029      +/-   ##
==========================================
- Coverage   70.80%   70.63%   -0.18%     
==========================================
  Files         305      305              
  Lines       61795    61807      +12     
==========================================
- Hits        43756    43657      -99     
- Misses      18039    18150     +111     

Flag	Coverage Δ
addons	92.88% <ø> (ø)
netcore	73.43% <90.41%> (-0.12%)	⬇️
netfx	69.22% <ø> (-0.22%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...nt/netcore/src/Microsoft/Data/SqlClient/SqlUtil.cs	39.14% <0.00%> (-0.37%)	⬇️
...e/src/Microsoft/Data/SqlClient/SNI/SNITcpHandle.cs	63.65% <91.66%> (+0.77%)	⬆️

... and 14 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[DavoudEshtehari]

Well done 🎉