Skip to content
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.

Sign up for GitHub

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

Jump to bottom

QUIC: Remove waiting on start event in send path #55442

Merged
merged 1 commit into from
Jul 12, 2021
Merged

QUIC: Remove waiting on start event in send path #55442

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 21 additions & 44 deletions src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);

Expand All @@ -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);

Expand All @@ -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)
{
Expand All @@ -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
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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}");
Copy link
Member

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.

Copy link
Contributor Author

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.


return MsQuicStatusCodes.InternalError;
}
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The 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?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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;
}
Expand Down Expand Up @@ -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"/>.
Expand Down Expand Up @@ -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,
Expand All @@ -1374,10 +1349,12 @@ private enum ShutdownState

private enum SendState
{
None,
None = 0,
Pending,
Aborted,
Finished,

// Terminal states
Aborted,
ConnectionClosed,
Closed
}
Expand Down