-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
QUIC: Remove waiting on start event in send path #55442
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,9 +26,6 @@ internal sealed class MsQuicStream : QuicStreamProvider | |
// Backing for StreamId | ||
private long _streamId = -1; | ||
|
||
// Used to check if StartAsync has been called. | ||
private bool _started; | ||
|
||
private int _disposed; | ||
|
||
private sealed class State | ||
|
@@ -53,7 +50,7 @@ private sealed class State | |
public int SendBufferMaxCount; | ||
public int SendBufferCount; | ||
|
||
// Resettable completions to be used for multiple calls to send, start, and shutdown. | ||
// Resettable completions to be used for multiple calls to send. | ||
public readonly ResettableCompletionSource<uint> SendResettableCompletionSource = new ResettableCompletionSource<uint>(); | ||
|
||
public ShutdownWriteState ShutdownWriteState; | ||
|
@@ -85,7 +82,6 @@ internal MsQuicStream(MsQuicConnection.State connectionState, SafeMsQuicStreamHa | |
_state.Handle = streamHandle; | ||
_canRead = true; | ||
_canWrite = !flags.HasFlag(QUIC_STREAM_OPEN_FLAGS.UNIDIRECTIONAL); | ||
_started = true; | ||
if (!_canWrite) | ||
{ | ||
_state.SendState = SendState.Closed; | ||
|
@@ -208,7 +204,7 @@ internal override async ValueTask WriteAsync(ReadOnlySequence<byte> buffers, boo | |
{ | ||
ThrowIfDisposed(); | ||
|
||
using CancellationTokenRegistration registration = await HandleWriteStartState(cancellationToken).ConfigureAwait(false); | ||
using CancellationTokenRegistration registration = HandleWriteStartState(cancellationToken); | ||
|
||
await SendReadOnlySequenceAsync(buffers, endStream ? QUIC_SEND_FLAGS.FIN : QUIC_SEND_FLAGS.NONE).ConfigureAwait(false); | ||
|
||
|
@@ -224,7 +220,7 @@ internal override async ValueTask WriteAsync(ReadOnlyMemory<ReadOnlyMemory<byte> | |
{ | ||
ThrowIfDisposed(); | ||
|
||
using CancellationTokenRegistration registration = await HandleWriteStartState(cancellationToken).ConfigureAwait(false); | ||
using CancellationTokenRegistration registration = HandleWriteStartState(cancellationToken); | ||
|
||
await SendReadOnlyMemoryListAsync(buffers, endStream ? QUIC_SEND_FLAGS.FIN : QUIC_SEND_FLAGS.NONE).ConfigureAwait(false); | ||
|
||
|
@@ -235,14 +231,14 @@ internal override async ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, bool e | |
{ | ||
ThrowIfDisposed(); | ||
|
||
using CancellationTokenRegistration registration = await HandleWriteStartState(cancellationToken).ConfigureAwait(false); | ||
using CancellationTokenRegistration registration = HandleWriteStartState(cancellationToken); | ||
|
||
await SendReadOnlyMemoryAsync(buffer, endStream ? QUIC_SEND_FLAGS.FIN : QUIC_SEND_FLAGS.NONE).ConfigureAwait(false); | ||
|
||
HandleWriteCompletedState(); | ||
} | ||
|
||
private async ValueTask<CancellationTokenRegistration> HandleWriteStartState(CancellationToken cancellationToken) | ||
private CancellationTokenRegistration HandleWriteStartState(CancellationToken cancellationToken) | ||
{ | ||
if (_state.SendState == SendState.Closed) | ||
{ | ||
|
@@ -268,14 +264,7 @@ private async ValueTask<CancellationTokenRegistration> HandleWriteStartState(Can | |
} | ||
} | ||
|
||
throw new System.OperationCanceledException(cancellationToken); | ||
} | ||
|
||
// Make sure start has completed | ||
if (!_started) | ||
{ | ||
await _state.SendResettableCompletionSource.GetTypelessValueTask().ConfigureAwait(false); | ||
_started = true; | ||
throw new OperationCanceledException(cancellationToken); | ||
} | ||
|
||
// if token was already cancelled, this would execute synchronously | ||
|
@@ -363,7 +352,7 @@ internal override async ValueTask<int> ReadAsync(Memory<byte> destination, Cance | |
} | ||
} | ||
|
||
throw new System.OperationCanceledException(cancellationToken); | ||
throw new OperationCanceledException(cancellationToken); | ||
} | ||
|
||
if (NetEventSource.Log.IsEnabled()) | ||
|
@@ -731,12 +720,12 @@ private static uint HandleEvent(State state, ref StreamEvent evt) | |
|
||
try | ||
{ | ||
switch ((QUIC_STREAM_EVENT_TYPE)evt.Type) | ||
switch (evt.Type) | ||
{ | ||
// Stream has started. | ||
// Will only be done for outbound streams (inbound streams have already started) | ||
case QUIC_STREAM_EVENT_TYPE.START_COMPLETE: | ||
return HandleEventStartComplete(state); | ||
return HandleEventStartComplete(state, ref evt); | ||
// Received data on the stream | ||
case QUIC_STREAM_EVENT_TYPE.RECEIVE: | ||
return HandleEventRecv(state, ref evt); | ||
|
@@ -769,12 +758,10 @@ private static uint HandleEvent(State state, ref StreamEvent evt) | |
{ | ||
if (NetEventSource.Log.IsEnabled()) | ||
{ | ||
NetEventSource.Error(state, $"[Stream#{state.GetHashCode()}] Exception occurred during handling {(QUIC_STREAM_EVENT_TYPE)evt.Type} event: {ex}"); | ||
NetEventSource.Error(state, $"[Stream#{state.GetHashCode()}] Exception occurred during handling {evt.Type} event: {ex}"); | ||
} | ||
|
||
// This is getting hit currently | ||
// See https://github.com/dotnet/runtime/issues/55302 | ||
//Debug.Fail($"[Stream#{state.GetHashCode()}] Exception occurred during handling {(QUIC_STREAM_EVENT_TYPE)evt.Type} event: {ex}"); | ||
Debug.Fail($"[Stream#{state.GetHashCode()}] Exception occurred during handling {evt.Type} event: {ex}"); | ||
|
||
return MsQuicStatusCodes.InternalError; | ||
} | ||
|
@@ -831,22 +818,10 @@ private static uint HandleEventPeerRecvAborted(State state, ref StreamEvent evt) | |
return MsQuicStatusCodes.Success; | ||
} | ||
|
||
private static uint HandleEventStartComplete(State state) | ||
private static uint HandleEventStartComplete(State state, ref StreamEvent evt) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to pass in the event if we are not using it? (now?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need to. I added it because I thought I was going to use it, but since we don't have a definition for the START_COMPLETE event data, we can't right now. I don't think there's any harm in having it, though. |
||
{ | ||
bool shouldComplete = false; | ||
lock (state) | ||
{ | ||
// Check send state before completing as send cancellation is shared between start and send. | ||
if (state.SendState == SendState.None) | ||
{ | ||
shouldComplete = true; | ||
} | ||
} | ||
|
||
if (shouldComplete) | ||
{ | ||
state.SendResettableCompletionSource.Complete(MsQuicStatusCodes.Success); | ||
} | ||
// TODO: We should probably check for a failure as indicated by the event data (or at least assert no failure if we aren't expecting it). | ||
// However, since there is no definition for START_COMPLETE event data currently, we can't do this right now. | ||
|
||
return MsQuicStatusCodes.Success; | ||
} | ||
|
@@ -1327,7 +1302,7 @@ private enum ReadState | |
/// <summary> | ||
/// The stream is open, but there is no data available. | ||
/// </summary> | ||
None, | ||
None = 0, | ||
|
||
/// <summary> | ||
/// Data is available in <see cref="State.ReceiveQuicBuffers"/>. | ||
|
@@ -1357,15 +1332,15 @@ private enum ReadState | |
|
||
private enum ShutdownWriteState | ||
{ | ||
None, | ||
None = 0, | ||
Canceled, | ||
Finished, | ||
ConnectionClosed | ||
} | ||
|
||
private enum ShutdownState | ||
{ | ||
None, | ||
None = 0, | ||
Canceled, | ||
Pending, | ||
Finished, | ||
|
@@ -1374,10 +1349,12 @@ private enum ShutdownState | |
|
||
private enum SendState | ||
{ | ||
None, | ||
None = 0, | ||
Pending, | ||
Aborted, | ||
Finished, | ||
|
||
// Terminal states | ||
Aborted, | ||
ConnectionClosed, | ||
Closed | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should find better way how to propagate exceptions to the caller. This is going to be hard IMHO to debug in CI and does nothing to diagnose filed issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, it's a bit of a pain to deal with this.
That said, it's not obvious to me how to propagate this to the caller and/or make it easier to debug. Suggestions welcome.
Hopefully this is not a common occurrence -- an exception during a callback means something is deeply, deeply wrong.