QUIC: Should not need finalization on MsQuicStream/Connection/Listener

[geoffkizer]

Currently we have a chain of objects for each of Stream/Connection/Listener that looks like this:

MsQuicStream (finalizable) -> stream State object (rooted by GCHandle) -> SafeMsQuicStreamHandle (finalizable)

We shouldn't need MsQuicStream to be finalizable; we should be able to rely on the SafeHandle finalizer -- after all that's what it is for.

The reason we need this today is that msquic holds a GCHandle to the State object, which roots it until the msquic stream handle is closed and all events are processed. But the State object holds a ref to the SafeHandle, which means the SafeHandle itself will never be finalized. We use the finalizer on MsQuicStream to break this loop.

I think we can solve this by removing the reference from State to the SafeHandle. In general we shouldn't need this; the MsQuicStream can pass the handle into appropriate methods on State when necessary.

So:

MsQuicStream -> stream State object (rooted by GCHandle)
MsQuicStream -> SafeMsQuicStreamHandle (finalizable)

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Currently we have a chain of objects for each of Stream/Connection/Listener that looks like this:

MsQuicStream (finalizable) -> stream State object (rooted by GCHandle) -> SafeMsQuicStreamHandle (finalizable)

We shouldn't need MsQuicStream to be finalizable; we should be able to rely on the SafeHandle finalizer -- after all that's what it is for.

The reason we need this today is that msquic holds a GCHandle to the State object, which roots it until the msquic stream handle is closed and all events are processed. But the State object holds a ref to the SafeHandle, which means the SafeHandle itself will never be finalized. We use the finalizer on MsQuicStream to break this loop.

I think we can solve this by removing the reference from State to the SafeHandle. In general we shouldn't need this; the MsQuicStream can pass the handle into appropriate methods on State when necessary.

So:

MsQuicStream -> stream State object (rooted by GCHandle)
MsQuicStream -> SafeMsQuicStreamHandle (finalizable)

Author:	geoffkizer
Assignees:	-
Labels:	area-System.Net.Quic
Milestone:	6.0.0

[ManickaP]

AFAIU MsQuicStream doesn't need to be finalizable per se, but than its State would have to be, because we need to, in all cases, clear number of active streams from connection, in order to properly close that connection eventually.

This could be solved differently as well. We had a chat with @wfurt about SafeHandle.DangerousAddRef to do the counting for us (and ignore msquic callbacks if we already disposed the relevant object), thus not needed to hold the link between connection and stream.

[CarnaViire]

But if I understand correctly, changing to DangerousAddRef counting wouldn't free you from holding the link - because you would need to call DangerousRelease in the end...

[geoffkizer]

To be explicit: the issue re refcounting streams on a connection is that all msquic stream handles must be closed before the connection handle is closed.

I think it would be best to handle this by having the stream SafeHandle take a DangerousAddRef on the connection SafeHandle. This should enable us to remove the refcounting that exists on MsQuicConnection today.

That said, this is not a priority relative to other issues we have at the moment.

[ManickaP]

CR: We could laverage DangerousAddRef to handle relation between connection and stream and properly order calls to close.
CR: SHUTDOWN event on connection will not happen while there are still any streams associated, we could laverage this instead of SafeHandle ref counting.
CR: Potentially we could get rid of back reference to the parent object since we don't care about keeping them alive while async msquic call is happening.

[CarnaViire]

SHUTDOWN event on connection will not happen while there are still any streams associated, we could laverage this instead of SafeHandle ref counting.

That is not true - I've checked it now. If I hold a reference to a stream (prevent Close on stream's SafeHandle) and then call Shutdown on the connection (CloseAsync() in S.N.Quic API), I do receive SHUTDOWN_COMPLETE event on that connection, even though the stream handle is "alive", meaning it is still not safe to close connection's SafeHandle.

[wfurt]

can you please comment on this @nibanks? That seems to contradict what we talk about.

[karelz]

Triage: This is sensitive area which already destabilized us before. Right now we are stable based on our test runs.
We want this long-term, but we should not prioritize it for 6.0 unless we have bug report, test failure, or something.

Moving to 7.0.

[karelz]

Triage: We should do it early in 7.0 to have time to stabilize. We have decent test coverage now, so we are not too much afraid to touch it.
It will help with maintenance.

[ManickaP]

Fixed in #71969 #71783 #71579