[QUIC] Stream refactorings

[ManickaP]

Based on our group code-review:

• We should make sure that we never return from a final state (Abort etc.) back to writing.
• Also we're missing final state for writing (we have one for reading).
• Do not use Handle... for private helpers that are not msquic callbacks, e.g.:

  runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs

  Line 383 in 4d41173

  private void HandleWriteCompletedState()

• Consolidate the state machine transitions from so many small helpers, i.e.: Handle... helpers. Blocked by having 3 different Write methods.
• NIT: shouldComplete (all vars) naming is not ideal
• Should we go back to success state after failed attempt to call msquic send or should we go into aborted state?

  runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs

  Lines 394 to 403 in 4d41173

  	private void HandleWriteFailedState()
  	{
  	lock (_state)
  	{
  	if (_state.SendState == SendState.Pending)
  	{
  	_state.SendState = SendState.Finished;
  	}
  	}
  	}

• Should we be issuing abortive shutdown (immediate) in cancellation of ShutdownCompleted? Beware of shutdown event handler releasing memory and having memory leaks.

  runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs

  Lines 628 to 646 in 4d41173

  	using CancellationTokenRegistration registration = cancellationToken.UnsafeRegister(static (s, token) =>
  	{
  	var state = (State)s!;
  	bool shouldComplete = false;
  	lock (state)
  	{
  	if (state.ShutdownState == ShutdownState.None)
  	{
  	state.ShutdownState = ShutdownState.Canceled;
  	shouldComplete = true;
  	}
  	}
  	if (shouldComplete)
  	{
  	state.ShutdownCompletionSource.SetException(
  	ExceptionDispatchInfo.SetCurrentStackTrace(new OperationCanceledException("Wait for shutdown was canceled", token)));
  	}
  	}, _state);

• NIT: Naming of Shutdown which closes the writing side of the stream:

  runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs

  Line 668 in 4d41173

  internal override void Shutdown()

• Use constants for _state.ShutdownDone

  runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs

  Line 808 in 4d41173

  releaseHandles = Interlocked.Exchange(ref _state.ShutdownDone, 1) == 2;

• Use constant for 0xffffffff

  runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs

  Line 828 in 4d41173

  StartShutdown(QUIC_STREAM_SHUTDOWN_FLAGS.ABORT_RECEIVE, 0xffffffff);

• Merge NativeCallbackHandler with HandleEvent in MsQuicStream. Why is this a seperate function? We should be consistent with other msquic objects like connection and listener. - both connection and listener have only one method.
• Rename HandleEventRecv and HandleEventPeerRecvAborted to both caontain full Receive word as do the msquic event names.
• Add high-level comments for HandleEventRecv and how it works together ReadAsync. The comments are there, just spread out across the code so it's hard to follow the logic.
• HandleEventSendComplete: the cancelled flag will be set in 2 known cases: connection is getting aborted/closed or we issued Send with FIN flag quickly followed by another Send (the second will be cancelled). Since there might be race with connection close event we might consider ignore this SEND_COMPLETE in case of Cancelled==1.

  runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs

  Line 1213 in 4d41173

  bool canceled = sendCompleteEvent.Canceled != 0;

• CleanupReadStateAndCheckPending could use a nicer name, but no one has any good suggestions.
• Add comments for the other state enums and their transitions as we do for ReadState.

[ghost]

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

Issue Details

---

Based on our group code-review:

• We should make sure that we never return from a final state (Abort etc.) back to writing.
• Also we're missing final state for writing (we have one for reading).
• Do not use Handle... for private helpers that are not msquic callbacks, e.g.:

  runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs

  Line 383 in 4d41173

  private void HandleWriteCompletedState()

• Consolidate the state machine transitions from so many small helpers, i.e.: Handle... helpers. Blocked by having 3 different Write methods.
• NIT: shouldComplete (all vars) naming is not ideal
• [ ]

Author:	ManickaP
Assignees:	-
Labels:	untriaged, area-System.Net.Quic
Milestone:	-

[ManickaP]

Closed by #71969