Fix connection resiliency check

[David-Engel]

Fixes issue #304.

The connection resiliency check was broken in corefx PR 34047. Specifically, this change:
https://github.com/dotnet/corefx/pull/34047/files#diff-6ec231dd165f403aa5aee08548594035L2422

[saurabh500]

How does this change fix the issue?

[Wraith2]

Me breaking things again. :sad:
Though i confess i don't understand what was wrong, can you explain a bit for me?

[cheenamalhotra]

EmptyReadPacket is abstract property in TdsParserStateObject.cs and is implemented in TdsParserStateObjectNative.cs#L186 which makes native call to read empty (0 byte) packet, and is an important step to detect active/idle connection in this case.

[Wraith2]

Oh. So what looked like assigning to an unneeded variable was causing a side effect that was used to check the connection status. In managed mode getting an empty read packet shouldn't do a network read so you need to check resiliency works in unix as well.

[cheenamalhotra]

So we have TdsParserStateObjectManaged.cs#L140 implemented for Managed code, I guess this would trigger an Empty read, isn't it?

[Wraith2]

I don't see where the network read is coming from. What I see is that I removed a packet variable and then a call to EmptyReadPacket try-finally with calls to IsPacketEmpty and ReleasePacket. We've got two implementations of those 3 methods.

Native:

protected override PacketHandle EmptyReadPacket => PacketHandle.FromNativePointer(default);

internal override bool IsPacketEmpty(PacketHandle readPacket)
{
    Debug.Assert(readPacket.Type == PacketHandle.NativePointerType || readPacket.Type == 0, "unexpected packet type when requiring NativePointer");
    return IntPtr.Zero == readPacket.NativePointer;
}

internal override void ReleasePacket(PacketHandle syncReadPacket)
{
    Debug.Assert(syncReadPacket.Type == PacketHandle.NativePointerType, "unexpected packet type when requiring NativePointer");
    SNINativeMethodWrapper.SNIPacketRelease(syncReadPacket.NativePointer);
}

Managed

protected override PacketHandle EmptyReadPacket => PacketHandle.FromManagedPacket(null);

internal override bool IsPacketEmpty(PacketHandle packet) => packet.ManagedPacket == null;

internal override void ReleasePacket(PacketHandle syncReadPacket) => syncReadPacket.ManagedPacket?.Dispose();

You can see that both implementation of EmptyReadPacket and IsPacketEmpty only check values on the packet handle structure and don't do any sort of IO. The only place there is IO is in the native ReleasePacket implementation where it calls into the native wrapper so in sni dll there could be something that causes io or checks state and signals connection failure. There's nothing like that in the managed implementation though so it wouldn't work the same way. In the managed case is it this call to ReleasePacket that causes the required behaviour?

[David-Engel]

@saurabh500 @Wraith2
In order to get a timely and accurate detection of the network connection state, we need to read a packet from the network. It's only at that time that a connection will change state if a disconnect has happened. It looks like EmptyReadPacket in the managed implementation is not reading anything off the network and never has so I'm guessing connection resiliency might not be working there, either.

[Wraith2]

If you can confirm that a simple call to ReleasePacket(EmptyReadPacket); restores the functionality in native mode i'd suggest adding a new abstact state manager method called something like CheckNetworkState and make that pattern the native implementation, then we can provide a managed implementation that reaches into the SNIHandle and checks it in some way to produce the same effect.

[David-Engel]

Well shoot. In retrying my repro, I realized I forgot to switch my using from SDS back to MDS after plugging in my private MDS nuget build yesterday. The code change looked very likely but this does not fix it. 😞 I guess I know what I'm doing with the rest of my day.