Fix handling of new connection in MsQuicListener

[wfurt]

I'm still testing but this is somewhat invasive change so I would like to get any early feedback.
As noted in #57246, part of the problem is that now we hand out QuicConnection before it is ready.
To fix that, this change adds another holding container, and it waits to either get CONNECTED event and then it jumps on old path to add connection to the Channel and that wakes the AcceptConnectionAsync.
If the connection does not get connected wit would get SHUTDOEN_COMPLETED and we would dispose and remove connection in the handler.

Since the vents are coming on the Connection we need relation back to the listener. So until one of the events come, we would hold reference to the ListenerState - similar to the Connection - Stream relation.

The second part is the validation handling. While on client we want to throw and propagate details to the caller there is nowhere to pass the exception on server. I updated the code to simple return failure instead of throwing. (that will clean the connection)

Added new test as well updated few existing one as failing handshake will no longer produce connection.

fixes #57246

[ghost]

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

Issue Details

---

I'm still testing but this is somewhat invasive change so I would like to get any early feedback.
As noted in #57246, part of the problem is that now we hand out QuicConnection before it is ready.
To fix that, this change adds another holding container, and it waits to either get CONNECTED event and then it jumps on old path to add connection to the Channel and that wakes the AcceptConnectionAsync.
If the connection does not get connected wit would get SHUTDOEN_COMPLETED and we would dispose and remove connection in the handler.

Since the vents are coming on the Connection we need relation back to the listener. So until one of the events come, we would hold reference to the ListenerState - similar to the Connection - Stream relation.

The second part is the validation handling. While on client we want to throw and propagate details to the caller there is nowhere to pass the exception on server. I updated the code to simple return failure instead of throwing. (that will clean the connection)

Added new test as well updated few existing one as failing handshake will no longer produce connection.

fixes #57246

Author:	wfurt
Assignees:	wfurt
Labels:	area-System.Net.Quic
Milestone:	-

[ManickaP]

Looks good!
I have just a few question to help me understand the change.

Thanks!

[ManickaP]

Could we inverse the condition here to reduce the nesting? Since we now have much deeper nesting.

if (state.Connected)
{
    return MsQuicStatusCodes.Success;
}
// The rest as is now.

[ManickaP]

I assume this part corresponds to the PR description:

I updated the code to simple return failure instead of throwing. (that will clean the connection)

Could you explain exactly what "clean the connection" means in this context? We have methods like Shutdown (finish using) and Close (free memory). Does it mean we don't need to call one/both of them? Won't it lead to double free since we're calling connection.Dispose() few lines above?

I'm just trying to understand what this exactly mean, I'm not saying it's wrong.

[wfurt]

From what I know, when the event callback returns something else than success code, MsQuic will notify peer, cleanup the connection internally and we will get "SHUTDOWN_COMPLETED" event. That should release the GC handle and that will start final description of the connection object.
We may or may not call the Dispose, depending how we will get to the end. If we do that should close the safe handle and tell MsQuic we are done. Attempt to make call on the connection would cause ODE from the SafeHandle.
That does not mean MsQuic would immediately free it. AFAIK they grab references when they need to and the object should be actually freed when squid is all done.
cc: @nibanks to confirm

[ManickaP]

So it substitutes Shutdown but not Close (free), which we still need to call sometime later.

[ManickaP]

NIT:

Suggested change

	// [InlineData(false)] ActiveIssue("https://github.com/dotnet/runtime/issues/57308")
	// [InlineData(false)] [ActiveIssue("https://github.com/dotnet/runtime/issues/57308")]

[ManickaP]

We're not cleaning the dictionary in case the listener gets disposed. I see that we're not cleaning AcceptConnectionQueue either, so I guess it's not an issue. I'm just making sure it's intentional.

[wfurt]

I was going back & forth on this one. With Connection & Stream we have FlushAcceptQueue so when the Connection is disposed, we would actively nuke all the stream.

I think for Listener everything would decompose natural since we are not playing tricks and we would release the _stateHandle. We could certainly speed it up by cleaning both. But I'm not sure if that matters as much as unlike connection, I would expect Listener would last long so this would be rare operation
I think it would be easy to add. adding @stephentoub and @geoffkizer in case they have suggestions.

[CarnaViire]

With Connection & Stream we had to make sure that we release Connection's SafeHandle only after all Stream handles are released. I don't think Connection has the same relation to the Listener, so the order of finalization doesn't matter...

[CarnaViire]

LGTM

[wfurt]

wasm & osx test failures are unrelated.