/azp run

Thanks.

The PacketHandle and SessionHandle designs were added indotnet/corefx#34044, and they were added to avoid boxing. This boxing was introduced as part of the original implementation of the managed SNI indotnet/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'sType == NativePacketType, we know that it's transporting anSNIPacket instance. This type is a SafeHandle derivative, and it's marshalledinto the unmanaged SNI when we set a packet's data and write it out to the remote instance. Conversely, a PacketHandle'sType == NativePointerType when we know that it's transporting nothing more than an IntPtr. This is marshalledout 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:

internalabstractclassTdsParserStateObject<TOutputPacket,TInputPacket,TSession>{publicabstractTSessionSession{get;}publicabstractSetPacketData(TOutputPacketpacket,byte[]buffer,intbytesUsed);publicabstractvoidReleasePacket(TInputPacketsyncReadPacket);// 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 usedpublicvoidPostReadAsyncForMars();// ...}internalsealedclassTdsParserStateObjectNative:TdsParserStateObject<SNIPacket,IntPtr,SNIHandle>{}internalsealedclassTdsParserStateObjectManaged:TdsParserStateObject<SNI.SNIPacket,SNI.SNIPacket,SNI.SNIHandle>{}

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

/azp run

Codecov Report

Attention: Patch coverage is82.50000% with7 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.

@@            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

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