Managed SNI prevent orphaned active packets being GC'ed without clear

[Wraith2]

In carefully timed situations a packet received asynchronously can be received after the session handle that owns the packet has been cleared.

This change takes a safety copy of the session handle (as other methods in this class already did) and checks to see if the handle is non-null before calling the read/write callback to pass the data on to the caller.

If the session handle is not valid the packet is directly cleared and dropped to the GC, this is safe to do as packets can easily be created if a pool has none available, it's just more efficient to re-use them

This was found using the reproduction in #659 but it does not fix that issue.

/cc @cheenamalhotra

[cheenamalhotra]

@Wraith2
I understand your point of view, and it's true these methods did fail with _sessionHandle being null before, we also handled it by putting null checks (?) there. I will add the else block to release packet in my code to continue debugging and get rid of unreleased packets but it sure is not a solution to the core problem.

The core problem we need to solve in this case is "why" the "_sessionHandle" would go null when we try to release the packet? I have traced that Dispose() is called before this release gets called, but that should just not happen! And if dispose is called, can we unlink and release all packets right there such that no further packets can be consumed/released once handle is disposed?

Let's say we handle _sessionHandle being null, it seems we are still unsure about any other exception that might occur when we call packet.Release() - hence those empty catch blocks? TBH we need to stay away from empty catch blocks - that's just rule no. 1, coz we can never get rid of them anytime in future! They could just swallow so much unknowingly leaving us to always doubt and cause breaking changes. So that's certainly not going in, we shall continue to look into why would we ever "expect" an exception on packet.Release(), can packet ever be invalid at that point? Either way let's remove the try-catch and see what we get.

[Wraith2]

I'm entirely happy with removing the try-catch if you prefer, the only thing that happens in them is packet cleanup and won't throw. I just put them there for caution, if they're problematic they can go.

[Wraith2]

error handling removed and i added in an assert in the packet where the packet can be identified as dropping to the GC without breaking in the GC thread.

As i said above this doesn't resolve any of the other problems in this area it just cleans up the packet handling, everything else is still under investigation.

[Wraith2]

I removed the debug assert because it fires for every packet in mars mode. I'm working through mars and will push any changes for that in a separate PR.

[cheenamalhotra]

Just FYI @Wraith2

I have a PR coming for Managed SNI tracing improvements, so you may hold on to further changes for that one.
I don't have any more changes for Managed SNI, just for once!

(I'll create after this and #889 get merged)

[cheenamalhotra]

FYI, I still get this error, repro used is 659_2 (second repro)

[Wraith2]

Yes, you will. This one only fixes non-mars cases. The mars fix is much more complex and includes the demuxer state machine rewrite. I chose not to try and post it until your tracing changes were merged.