notes

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

@@ -16,8 +16,10 @@ namespace System.Net.Quic.Implementations.MsQuic
  internal sealed class MsQuicStream : QuicStreamProvider
  {
  // Delegate that wraps the static function that will be called when receiving an event.
+ // CR: callback calls for a single instance of a stream are serialized by msquic, happpen on a msquic thread and shouldn't take too long to not to block msquic.
  internal static readonly StreamCallbackDelegate s_streamDelegate = new StreamCallbackDelegate(NativeCallbackHandler);
 
+ // CR: passed to msquic and returned in msquic callback.
  private readonly State _state = new State();
 
  private readonly bool _canRead;
@@ -30,7 +32,11 @@ internal sealed class MsQuicStream : QuicStreamProvider
 
  private sealed class State
  {
+ // 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.
  public SafeMsQuicStreamHandle Handle = null!; // set in ctor.
+ // CR: roots the state in GC and it won't get collected while this exist.
+ // CR: must be kept alive until we receive SHUTDOWN_COMPLETE event
  public GCHandle StateGCHandle;
 
  public MsQuicStream? Stream; // roots the stream in the pinned state to prevent GC during an async read I/O.
@@ -50,6 +56,7 @@ private sealed class State
  // set when ReadState.PendingRead:
  public Memory<byte> ReceiveUserBuffer;
  public CancellationTokenRegistration ReceiveCancellationRegistration;
+ // 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.
  // Resettable completions to be used for multiple calls to receive.
  public readonly ResettableCompletionSource<int> ReceiveResettableCompletionSource = new ResettableCompletionSource<int>();
 
@@ -65,12 +72,16 @@ private sealed class State
  // Resettable completions to be used for multiple calls to send.
  public readonly ResettableCompletionSource<uint> SendResettableCompletionSource = new ResettableCompletionSource<uint>();
 
+ // CR: We should get rid off it, we're not interested in it.
  public ShutdownWriteState ShutdownWriteState;
 
  // Set once writes have been shutdown.
+ // CR: We should get rid of this
  public readonly TaskCompletionSource ShutdownWriteCompletionSource = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
 
+ // CR: Shutdown should be merged into CloseAsync.
  public ShutdownState ShutdownState;
+ // CR: To make sure that we release the handles only once.
  public int ShutdownDone;
 
  // Set once stream have been shutdown.
@@ -113,16 +124,21 @@ internal MsQuicStream(MsQuicConnection.State connectionState, SafeMsQuicStreamHa
  }
  catch
  {
+ // CR: we should release the handle here as well as we do in outbound. Does int interfere with this being run on msquic thread? Ask NICK.
+ // Should we return a specific error code in MsQuicConnection to clean the handle for us?
  _state.StateGCHandle.Free();
  throw;
  }
 
+ // CR: We should move this before callback handler registration.
  if (!connectionState.TryAddStream(this))
  {
  _state.StateGCHandle.Free();
  throw new ObjectDisposedException(nameof(QuicConnection));
  }
 
+ // CR: We should set it before SetCallbackHandlerDelegate since we can get CONNETCION_CLOSED event and that needs this.
+ // If we want to prevent decrementing stream count, we should explicitely unset here.
  _state.ConnectionState = connectionState;
 
  _state.TraceId = MsQuicTraceHelper.GetTraceId(_state.Handle);
@@ -170,6 +186,7 @@ internal MsQuicStream(MsQuicConnection.State connectionState, QUIC_STREAM_OPEN_F
  throw;
  }
 
+ // CR: The same as with the inbound.
  if (!connectionState.TryAddStream(this))
  {
  _state.Handle?.Dispose();
@@ -191,6 +208,7 @@ internal MsQuicStream(MsQuicConnection.State connectionState, QUIC_STREAM_OPEN_F
 
  internal override bool CanRead => _disposed == 0 && _canRead;
 
+ // CR: Might be useful to introduce Is(Uni|Bi)Directional to distinguish the type of the stream instead of using Can*.
  internal override bool CanWrite => _disposed == 0 && _canWrite;
 
  internal override long StreamId
@@ -220,6 +238,7 @@ internal override ValueTask WriteAsync(ReadOnlySequence<byte> buffers, Cancellat
 
  internal override async ValueTask WriteAsync(ReadOnlySequence<byte> buffers, bool endStream, CancellationToken cancellationToken = default)
  {
+ // CR: we should propagate exceptions up here for write state.
  ThrowIfDisposed();
 
  using CancellationTokenRegistration registration = HandleWriteStartState(cancellationToken);
@@ -258,6 +277,8 @@ internal override async ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, bool e
 
  private CancellationTokenRegistration HandleWriteStartState(CancellationToken cancellationToken)
  {
+ // CR: we should make the flow as we do in Read, where we check the state and make transition in a lock as a first thing
+ // and throw exceptions afterwards.
  if (_state.SendState == SendState.Closed)
  {
  throw new InvalidOperationException(SR.net_quic_writing_notallowed);
@@ -286,6 +307,8 @@ private CancellationTokenRegistration HandleWriteStartState(CancellationToken ca
  }
 
  // if token was already cancelled, this would execute synchronously
+ // CR: we need to track the registration and dispose it even in case of exception thrown outside of this method.
+ // https://github.com/dotnet/runtime/issues/55437
  CancellationTokenRegistration registration = cancellationToken.UnsafeRegister(static (s, token) =>
  {
  var state = (State)s!;
@@ -318,6 +341,7 @@ private CancellationTokenRegistration HandleWriteStartState(CancellationToken ca
  throw new QuicStreamAbortedException(_state.SendErrorCode);
  }
 
+ // CR: this should be some other exception, we should throw OCE unless CT has been fired.
  throw new OperationCanceledException(SR.net_quic_sending_aborted);
  }
  else if (_state.SendState == SendState.ConnectionClosed)
@@ -555,6 +579,7 @@ private void StartShutdown(QUIC_STREAM_SHUTDOWN_FLAGS flags, long errorCode)
  QuicExceptionHelpers.ThrowIfFailed(status, "StreamShutdown failed.");
  }
 
+ // CR: We should get rid of this
  internal override async ValueTask ShutdownWriteCompleted(CancellationToken cancellationToken = default)
  {
  ThrowIfDisposed();
@@ -1146,6 +1171,8 @@ private static void CleanupSendState(State state)
  }
 
  // TODO prevent overlapping sends or consider supporting it.
+ // CR: Why do we have 3 version of sending? We should unite as much as possible.
+ // CR: are we even testing all of them?
  private unsafe ValueTask SendReadOnlyMemoryAsync(
  ReadOnlyMemory<byte> buffer,
  QUIC_SEND_FLAGS flags)
@@ -1200,6 +1227,7 @@ private unsafe ValueTask SendReadOnlyMemoryAsync(
  return _state.SendResettableCompletionSource.GetTypelessValueTask();
  }
 
+ // CR: for kestrel
  private unsafe ValueTask SendReadOnlySequenceAsync(
  ReadOnlySequence<byte> buffers,
  QUIC_SEND_FLAGS flags)