SqlClient handle socket network error gracefully

[Wraith2]

Fixes https://github.com/dotnet/corefx/issues/33930

Then a running the SqlClient test suite in managed mode (Linux, osx, uap) and Debug build, an assert fires indicating that an invalid state has been reached. The assertion indicates that a packet has reached the processing stage and has no contents or that the packet has been received in a state where none was expected. The test failure was caused by an assertion but could lead to silent unexpected non-crashing behaviour in release builds leading to unreliability.

After investigation and debugging I've arrived at three related changes.

• plumb the error message from the packet read function all the way through the handler chain so that the recipients can identify what the error was
• change the read function so that socket disconnections use an existing but unused error code to identify that the connection has been reset and add specific error catching for disconnections.
• detect disconnections using the above changes and close the connection gracefully, and change a check for null network packet sources so that unexpected packets in the now closed state are correctly ignored.

The functional and manual tests pass correctly with the affected tests re-enabled.
/cc @afsanehr, @tarikulsabbir, @Gary-Zh , @David-Engel @saurabh500

Minor changes also included are:
Made the async nested functions static in the packet read so that you don't have to be careful not to refer to outer function variables and degrade performance.
Changed the callback object in mars handle and connection to be strongly typed since it was object and always cast to the concrete type before use.

[AfsanehR-zz]

As mentioned in #36735 we will wait on merging these PRs once we have the DataAccess performance tests. @Wraith2 I was just wondering if you get a chance to work on porting these tests?

[Wraith2]

You suggested it but I wasn't sure how it could be usefully accomplished, I wasn't aware it was blocking unrelated PRs. As I explained in the other thread I wasn't able to reproduce the problem I'd introduced in debug mode so running the tests wouldn't be particularly useful. I can add something which spends a specific time querying on CPUCount*4 threads but it isn't a deterministic test it's straying into mean time to failure testing which isn't what the integration testing is really for.

Aside from the new required test does this code make sense, seem correct and otherwise function?

[saurabh500]

I had a cursory look and it seems good. Will look in depth tomorrow.

[saurabh500]

So what was the inconsistent state that was being reached? I understand that the PR plumbs errors all the way up. What really did this fix?

[Wraith2]

When the socket was closed the code was following the path for receipt of a packet but there was no data in the packet so an caught the problem later on. Once that is fixed with the error propagation a corner case can turn up where the callback could be invoked after the connection was in the closed state, this is fixed by the slight change to the null check in the receive method.

[saurabh500]

This condition removal doesn't seem right. This shouldn't be done in Native SNI. In native SNI, all callbacks happen on the same thread and this differentiation of the physical connection object is the only way to differentiate if the callback corresponds to this object.

Also why is this correct for Managed SNI?

[Wraith2]

Because I've introduced the close method on disconnection the physical connection will now be nulled, so checking if it's the current instance doesn't help. This catches the case where the socket is closed while the socket wait is in progress which causes a 0 packet receive.

The socket in the native version is handled in the sni lib and the same condition doesn't occur there.

[saurabh500]

Because I've introduced the close method on disconnection the physical connection will now be nulled

What close method?

[Wraith2]

In SNIMarsHadle.HandleReceiveError,

if (sniErrorCode == TdsEnums.SNI_WSAECONNRESET)
{
    TdsParser parser = _callbackObject.Parser;
    parser.State = TdsParserState.Broken;
    parser.Connection.BreakConnection();
}

BreakConnection dooms closes and does a ton of other stuff one of which is nulling the state object. Since the only two values that object can have are null and this and if it's either null or this and the source tcs is null it's not going to work the check on the state object seemed redundant.

[saurabh500]

The BreakConnection, only impacts breaking the connection for Managed SNI, this code would never run for Native SNI, and in that case the removal of this check can have unpredictable impact when Native SNI is being used.

[Wraith2]

I see. Then I'm going to have to put that back in and do some debugging and tracing to repo the problem it fixes and see if there's another way to detect it strictly in the managed side. I think if i'd seen one I would have used it so this might be tricky.

[saurabh500]

Can you retain the comment?

[Wraith2]

I can but it's no longer needed since a static can't refer to outer locals, sure you want to keep it?

[Wraith2]

I'm going to close this because the git history is a mess, I'm investigating and will rework and open a new PR when it's ready.