Merge | TdsParserStateObject PacketHandle usage

[edwardneal]

Relates to #1261.

This continues the TdsParserStateObject merge work. At present, there's a deviation between netcore and netfx: netcore uses a PacketHandle type to shuttle the native or managed packet handles around, while netfx just treats PacketHandle as an alias for IntPtr. This PR is focused strictly on merging that handling, so we can move on to the identical methods which use the type.

[mdaigle]

All of your changes look reasonable.

Would you be able to add a high level summary to the PacketHandle class docs that covers when the different packet types are used? In particular, it's not obvious to me when we would use NativePointerType vs NativePacketType.

Bigger picture (and not blocking for this PR, just looking to discuss), do you have any insight into what this design pattern gives us? It feels like we're fighting the type system. I can't think of a reason why we'd want this pattern vs just using the underlying types directly or a ref struct per underlying type, but I may be missing something.

[benrr101]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[edwardneal]

Thanks.

The PacketHandle and SessionHandle designs were added in dotnet/corefx#34044, and they were added to avoid boxing. This boxing was introduced as part of the original implementation of the managed SNI in dotnet/corefx#16589. The original native SNI design had the two types we care about (IntPtr and SNIPacket) and this has evolved from that.

To speculate on the original design pattern: it feels like we're driven partially by marshalling here. When a PacketHandle's Type == NativePacketType, we know that it's transporting an SNIPacket instance. This type is a SafeHandle derivative, and it's marshalled into the unmanaged SNI when we set a packet's data and write it out to the remote instance. Conversely, a PacketHandle's Type == NativePointerType when we know that it's transporting nothing more than an IntPtr. This is marshalled out of the unmanaged SNI when we receive a packet and read its data in managed code.

Strictly speaking, I'm not sure we'd need to maintain that separation; I'd personally be surprised if the unmanaged SNI DLL had a type separation between the inbound and the outbound packets, so the managed library might not need it either. If there's no separation, we might be able to rely on the normal SafeHandle marshalling mechanisms to let us use SNIPacket instances from end to end. This'd simplify matters, but it'd also increase GC traffic (because every packet received from SQL Server is now a full object with its own finalizer, rather than just an IntPtr.)

If we wanted to avoid that specific regression, another way of representing this might be a generics-based design similar to below:

internal abstract class TdsParserStateObject<TOutputPacket, TInputPacket, TSession>
{
    public abstract TSession Session { get; }
    public abstract SetPacketData(TOutputPacket packet, byte[] buffer, int bytesUsed);
    public abstract void ReleasePacket(TInputPacket syncReadPacket);

    // This was in TdsParser, but would need to be be moved into TdsParserStateObject - it's the
    // only place outside this class (besides NativeSspiContextProvider) where PacketHandle and SessionHandle are used
    public void PostReadAsyncForMars();
    // ...
}

internal sealed class TdsParserStateObjectNative : TdsParserStateObject<SNIPacket, IntPtr, SNIHandle>
{ }

internal sealed class TdsParserStateObjectManaged : TdsParserStateObject<SNI.SNIPacket, SNI.SNIPacket, SNI.SNIHandle>
{ }

It might be worth considering and benchmarking after these are merged.

[benrr101]

Would like to think through the PacketHandle class before we approve this. But otherwise, I think these will be good changes to move us through the merge classes.
Also, this is going to run into conflicts with #3345...

[benrr101]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[codecov]

Codecov Report

Attention: Patch coverage is 82.50000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 61.38%. Comparing base (dc1298a) to head (1c38fd9).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
...osoft/Data/SqlClient/TdsParserStateObject.netfx.cs	79.41%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3353      +/-   ##
==========================================
- Coverage   65.16%   61.38%   -3.78%     
==========================================
  Files         300      294       -6     
  Lines       65379    65082     -297     
==========================================
- Hits        42606    39953    -2653     
- Misses      22773    25129    +2356     

Flag	Coverage Δ
addons	?
netcore	66.59% <ø> (-1.83%)	⬇️
netfx	59.59% <82.50%> (-6.74%)	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.