Fix Wrong exception for Open*Stream after connection is closed

[rzikm]

I considered adding guards into individual Open*Stream methods, but that still leaves a data race between the stream-creating thread and incoming connection aborts, so it seems best to me to use the status code from MsQuicStreamOpen to detect connection abort/close.

Fixes #56133

[ghost]

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

Issue Details

---

I considered adding guards into individual Open*Stream methods, but that still leaves a data race between the stream-creating thread and incoming connection aborts, so I devided it would be best to use the status code from MsQuicStreamOpen.

Fixes #56133

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

[CarnaViire]

I believe we should throw QuicConnectionAbortedException only if status == MsQuicStatusCodes.Aborted. If it's an invalid state, it's a sign that there's a bug left either in our code or in msquic -- we don't want to hide that. And connectionState.AbortErrorCode would be set only if connection was actually aborted.

[rzikm]

I think that the InvalidState happens when the connection is aborted locally (i.e. first close the connection and then immediately try opening a stream), so to cover that case without checking for InvalidState here, we would probably have to put another guard somewhere else (like Open*Stream function).

[CarnaViire]

If the connection is aborted locally, it should be different exception (QuicOperationAbortedException) -- this is how user understands who was the initiator...
UPD: Ok, ThrowHelper already does that check

[CarnaViire]

Have you seen INVALID_STATE happening on local close?

[CarnaViire]

ok I see
https://github.com/microsoft/msquic/blob/0fffdc076abd52a61a2d4d8d699cf6a5f80e56e5/src/core/stream.c#L221-L223

        Status = 
            (ClosedLocally || Stream->Flags.Started) ?
            QUIC_STATUS_INVALID_STATE :
            QUIC_STATUS_ABORTED;

but it means INVALID_STATE is not only used if ClosedLocally; and it doesn't prevent stream_open from returning it later for some different reason....

On one hand, it will be better if we were able to tell the difference and not just translate INVALID_STATE into OperationAbortedException. This exception now bears the meaning of "you do something that results in a local operation being aborted, e.g. AbortRead (or close Stream or close Connection) with a pending Read"; if we use it for something else it will become unhelpful... in the same way "INVALID_STATE" sounds unhelpful -- something is wrong but you're not sure what.

On the other hand, tracking all we do locally and adding the guards you mention might be too big and complex change, and if it would involve locking, it may significantly impact perf.

I was initially inclined towards not translating INVALID_STATE into OperationAbortedException, but I'm not so sure now.

@ManickaP what are your thoughts?

[ManickaP]

In case of INVALID_STATE we're doing something wrong (we = user of System.Net.Quic), 2 cases here:

• closing connection locally and then opening stream
• opening stream twice

In both cases, we're fully responsible for the wrong combination of operations.

==> I think we should keep the original exception for INVALID_STATE and change it only for ABORTED.

Note that we might revisit this in the future (APIs, customer feedback, ...), so no need to obsess about it.

[CarnaViire]

Thanks!