Skip to content

Commit

Permalink
[QUIC] API changes follow up (dotnet#72366)
Browse files Browse the repository at this point in the history 
* Comments

* CanRead and CanWrite take into account whether the side is closed or not.

* PeerCertificate test

* Reverted CanRead/CanWrite; comments; tests for QuicStream task properties; fixed behavior of ResettableValueTaskSource

* Log if certificate is not selected
  • Loading branch information
@ManickaP
ManickaP authored Jul 18, 2022
1 parent 32aa0e1 commit 94a444a
Show file tree
Hide file tree
Showing 10 changed files with 558 additions and 124 deletions.
14 changes: 14 additions & 0 deletions src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,13 @@ public static MsQuicSafeHandle Create(QuicClientConnectionOptions options)
{
certificate = selectedCertificate;
}
else
{
if (NetEventSource.Log.IsEnabled())
{
NetEventSource.Info(options, $"'{certificate}' not selected because it doesn't have a private key.");
}
}
}
else if (authenticationOptions.ClientCertificates != null)
{
Expand All @@ -52,6 +59,13 @@ public static MsQuicSafeHandle Create(QuicClientConnectionOptions options)
certificate = clientCertificate;
break;
}
else
{
if (NetEventSource.Log.IsEnabled())
{
NetEventSource.Info(options, $"'{certificate}' not selected because it doesn't have a private key.");
}
}
}
}

Expand Down
175 changes: 84 additions & 91 deletions src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ResettableValueTaskSource.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ internal sealed class ResettableValueTaskSource : IValueTaskSource
{
// None -> [TryGetValueTask] -> Awaiting -> [TrySetResult|TrySetException(final: false)] -> Ready -> [GetResult] -> None
// None -> [TrySetResult|TrySetException(final: false)] -> Ready -> [TryGetValueTask] -> [GetResult] -> None
// None|Awaiting -> [TrySetResult|TrySetException(final: true)] -> Final(never leaves this state)
// None|Awaiting -> [TrySetResult|TrySetException(final: true)] -> Completed(never leaves this state)
// Ready -> [GetResult: TrySet*(final: true) was called] -> Completed(never leaves this state)
private enum State
{
None,
Expand All @@ -30,7 +31,7 @@ private enum State
private Action<object?>? _cancellationAction;
private GCHandle _keepAlive;

private FinalTaskSource _finalTaskSource;
private TaskCompletionSource _finalTaskSource;

public ResettableValueTaskSource(bool runContinuationsAsynchronously = true)
{
Expand All @@ -39,7 +40,8 @@ public ResettableValueTaskSource(bool runContinuationsAsynchronously = true)
_cancellationRegistration = default;
_keepAlive = default;

_finalTaskSource = new FinalTaskSource(runContinuationsAsynchronously);
// TODO: defer instantiation only after Task is retrieved
_finalTaskSource = new TaskCompletionSource(runContinuationsAsynchronously ? TaskCreationOptions.RunContinuationsAsynchronously : TaskCreationOptions.None);
}

/// <summary>
Expand All @@ -49,11 +51,21 @@ public ResettableValueTaskSource(bool runContinuationsAsynchronously = true)
public Action<object?> CancellationAction { init { _cancellationAction = value; } }

/// <summary>
/// Returns <c>true</c> is this task source has entered its final state, i.e. <see cref="TryComplete(Exception?, bool)"/> or <see cref="TrySetException(Exception, bool)"/>
/// Returns <c>true</c> is this task source has entered its final state, i.e. <see cref="TrySetResult(bool)"/> or <see cref="TrySetException(Exception, bool)"/>
/// was called with <c>final</c> set to <c>true</c> and the result was propagated.
/// </summary>
public bool IsCompleted => (State)Volatile.Read(ref Unsafe.As<State, byte>(ref _state)) == State.Completed;

/// <summary>
/// Tries to get a value task representing this task source. If this task source is <see cref="State.None"/>, it'll also transition it into <see cref="State.Awaiting"/> state.
/// It prevents concurrent operations from being invoked since it'll return <c>false</c> if the task source was already in <see cref="State.Awaiting"/> state.
/// In other states, it'll return a value task representing this task source without any other work. So to determine whether to invoke a P/Invoke operation or not,
/// the state of <paramref name="valueTask"/> must also be checked.
/// </summary>
/// <param name="valueTask">A value task representing the result. Only meaningful in case this method returns <c>true</c>. Might already be completed.</param>
/// <param name="keepAlive">An object to hold during a P/Invoke call. It'll get release with setting the result/exception.</param>
/// <param name="cancellationToken">A cancellation token which might cancel the value task.</param>
/// <returns><c>true</c> if this is not an overlapping call (task source transitioned or was already set); otherwise, <c>false</c>.</returns>
public bool TryGetValueTask(out ValueTask valueTask, object? keepAlive = null, CancellationToken cancellationToken = default)
{
lock (this)
Expand All @@ -66,10 +78,11 @@ public bool TryGetValueTask(out ValueTask valueTask, object? keepAlive = null, C
{
_cancellationRegistration = cancellationToken.UnsafeRegister(static (obj, cancellationToken) =>
{
(ResettableValueTaskSource parent, object? target) = ((ResettableValueTaskSource, object?))obj!;
if (parent.TrySetException(new OperationCanceledException(cancellationToken)))
(ResettableValueTaskSource thisRef, object? target) = ((ResettableValueTaskSource, object?))obj!;
// This will transition the state to Ready.
if (thisRef.TrySetException(new OperationCanceledException(cancellationToken)))
{
parent._cancellationAction?.Invoke(target);
thisRef._cancellationAction?.Invoke(target);
}
}, (this, keepAlive));
}
Expand Down Expand Up @@ -118,45 +131,54 @@ private bool TryComplete(Exception? exception, bool final)
{
State state = _state;

// None,Awaiting: clean up and finish the task source.
if (state == State.Awaiting ||
state == State.None)
// Completed: nothing to do.
if (state == State.Completed)
{
return false;
}

// If the _valueTaskSource has already been set, we don't want to lose the result by overwriting it.
// So keep it as is and store the result in _finalTaskSource.
if (state == State.None ||
state == State.Awaiting)
{
_state = final ? State.Completed : State.Ready;
}

// Swap the cancellation registration so the one that's been registered gets eventually Disposed.
// Ideally, we would dispose it here, but if the callbacks kicks in, it tries to take the lock held by this thread leading to deadlock.
cancellationRegistration = _cancellationRegistration;
_cancellationRegistration = default;
// Swap the cancellation registration so the one that's been registered gets eventually Disposed.
// Ideally, we would dispose it here, but if the callbacks kicks in, it tries to take the lock held by this thread leading to deadlock.
cancellationRegistration = _cancellationRegistration;
_cancellationRegistration = default;

// Unblock the current task source and in case of a final also the final task source.
if (exception is not null)
// Unblock the current task source and in case of a final also the final task source.
if (exception is not null)
{
// Set up the exception stack strace for the caller.
exception = exception.StackTrace is null ? ExceptionDispatchInfo.SetCurrentStackTrace(exception) : exception;
if (state == State.None ||
state == State.Awaiting)
{
// Set up the exception stack strace for the caller.
exception = exception.StackTrace is null ? ExceptionDispatchInfo.SetCurrentStackTrace(exception) : exception;
_valueTaskSource.SetException(exception);
}
else
if (final)
{
return _finalTaskSource.TrySetException(exception);
}
return state != State.Ready;
}
else
{
if (state == State.None ||
state == State.Awaiting)
{
_valueTaskSource.SetResult(final);
}

if (final)
{
_finalTaskSource.TryComplete(exception);
_finalTaskSource.TrySignal(out _);
return _finalTaskSource.TrySetResult();
}

return true;
}

// Final: remember the first final result to set it once the current non-final result gets retrieved.
if (final)
{
return _finalTaskSource.TryComplete(exception);
return state != State.Ready;
}

return false;
}
finally
{
Expand All @@ -176,11 +198,24 @@ private bool TryComplete(Exception? exception, bool final)
}
}

/// <summary>
/// Tries to transition from <see cref="State.Awaiting"/> to either <see cref="State.Ready"/> or <see cref="State.Completed"/>, depending on the value of <paramref name="final"/>.
/// Only the first call (with either value for <paramref name="final"/>) is able to do that. I.e.: <c>TrySetResult()</c> followed by <c>TrySetResult(true)</c> will both return <c>true</c>.
/// </summary>
/// <param name="final">Whether this is the final transition to <see cref="State.Completed" /> or just a transition into <see cref="State.Ready"/> from which the task source can be reset back to <see cref="State.None"/>.</param>
/// <returns><c>true</c> if this is the first call that set the result; otherwise, <c>false</c>.</returns>
public bool TrySetResult(bool final = false)
{
return TryComplete(null, final);
}

/// <summary>
/// Tries to transition from <see cref="State.Awaiting"/> to either <see cref="State.Ready"/> or <see cref="State.Completed"/>, depending on the value of <paramref name="final"/>.
/// Only the first call is able to do that with the exception of <c>TrySetResult()</c> followed by <c>TrySetResult(true)</c>, which will both return <c>true</c>.
/// </summary>
/// <param name="final">Whether this is the final transition to <see cref="State.Completed" /> or just a transition into <see cref="State.Ready"/> from which the task source can be reset back to <see cref="State.None"/>.</param>
/// <param name="exception">The exception to set as a result of the value task.</param>
/// <returns><c>true</c> if this is the first call that set the result; otherwise, <c>false</c>.</returns>
public bool TrySetException(Exception exception, bool final = false)
{
return TryComplete(exception, final);
Expand All @@ -194,9 +229,11 @@ void IValueTaskSource.OnCompleted(Action<object?> continuation, object? state, s

void IValueTaskSource.GetResult(short token)
{
bool successful = false;
try
{
_valueTaskSource.GetResult(token);
successful = true;
}
finally
{
Expand All @@ -207,75 +244,31 @@ void IValueTaskSource.GetResult(short token)
if (state == State.Ready)
{
_valueTaskSource.Reset();
if (_finalTaskSource.TrySignal(out Exception? exception))
_state = State.None;

// Propagate the _finalTaskSource result into _valueTaskSource if completed.
if (_finalTaskSource.Task.IsCompleted)
{
_state = State.Completed;

if (exception is not null)
if (_finalTaskSource.Task.IsCompletedSuccessfully)
{
_valueTaskSource.SetException(exception);
_valueTaskSource.SetResult(true);
}
else
{
_valueTaskSource.SetResult(true);
// We know it's always going to be a single exception since we're the ones setting it.
_valueTaskSource.SetException(_finalTaskSource.Task.Exception?.InnerException!);
}

// In case the _valueTaskSource was successful, we want the potential error from _finalTaskSource to surface immediately.
// In other words, if _valueTaskSource was set with success while final exception arrived, this will throw that exception right away.
if (successful)
{
_valueTaskSource.GetResult(_valueTaskSource.Version);
}
}
else
{
_state = State.None;
}
}
}
}
}

private struct FinalTaskSource
{
private TaskCompletionSource _finalTaskSource;
private bool _isCompleted;
private Exception? _exception;

public FinalTaskSource(bool runContinuationsAsynchronously = true)
{
// TODO: defer instantiation only after Task is retrieved
_finalTaskSource = new TaskCompletionSource(runContinuationsAsynchronously ? TaskCreationOptions.RunContinuationsAsynchronously : TaskCreationOptions.None);
_isCompleted = false;
_exception = null;
}

public Task Task => _finalTaskSource.Task;

public bool TryComplete(Exception? exception = null)
{
if (_isCompleted)
{
return false;
}

_exception = exception;
_isCompleted = true;
return true;
}

public bool TrySignal(out Exception? exception)
{
if (!_isCompleted)
{
exception = default;
return false;
}

if (_exception is not null)
{
_finalTaskSource.SetException(_exception);
}
else
{
_finalTaskSource.SetResult();
}

exception = _exception;
return true;
}
}
}
26 changes: 24 additions & 2 deletions src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/ValueTaskSource.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,22 @@ public ValueTaskSource(bool runContinuationsAsynchronously = true)
_keepAlive = default;
}

/// <summary>
/// Returns <c>true</c> is this task source was completed, i.e. <see cref="TrySetResult"/> or <see cref="TrySetException(Exception)"/> was called.
/// </summary>
public bool IsCompleted => (State)Volatile.Read(ref Unsafe.As<State, byte>(ref _state)) == State.Completed;
/// <summary>
/// Returns <c>true</c> is this task source was completed successfully, i.e. <see cref="TrySetResult"/> was called and set the result.
/// </summary>
public bool IsCompletedSuccessfully => IsCompleted && _valueTaskSource.GetStatus(_valueTaskSource.Version) == ValueTaskSourceStatus.Succeeded;

/// <summary>
/// Tries to transition from <see cref="State.None"/> to <see cref="State.Awaiting"/>. Only the first call is able to do that so the result can be used to determine whether to invoke an operation or not.
/// </summary>
/// <param name="valueTask">A value task representing the result. In case this method returns <c>false</c>, it'll still contain a value task that'll get set with the original operation.</param>
/// <param name="keepAlive">An object to hold during a P/Invoke call. It'll get release with setting the result/exception.</param>
/// <param name="cancellationToken">A cancellation token which might cancel the value task.</param>
/// <returns><c>true</c> if this is the first call; otherwise, <c>false</c>.</returns>
public bool TryInitialize(out ValueTask valueTask, object? keepAlive = null, CancellationToken cancellationToken = default)
{
lock (this)
Expand All @@ -53,8 +66,8 @@ public bool TryInitialize(out ValueTask valueTask, object? keepAlive = null, Can
{
_cancellationRegistration = cancellationToken.UnsafeRegister(static (obj, cancellationToken) =>
{
ValueTaskSource parent = (ValueTaskSource)obj!;
parent.TrySetException(new OperationCanceledException(cancellationToken));
ValueTaskSource thisRef = (ValueTaskSource)obj!;
thisRef.TrySetException(new OperationCanceledException(cancellationToken));
}, this);
}
}
Expand Down Expand Up @@ -134,11 +147,20 @@ private bool TryComplete(Exception? exception)
}
}

/// <summary>
/// Tries to transition from <see cref="State.Awaiting"/> to <see cref="State.Completed"/>. Only the first call is able to do that.
/// </summary>
/// <returns><c>true</c> if this is the first call that set the result; otherwise, <c>false</c>.</returns>
public bool TrySetResult()
{
return TryComplete(null);
}

/// <summary>
/// Tries to transition from <see cref="State.Awaiting"/> to <see cref="State.Completed"/>. Only the first call is able to do that.
/// </summary>
/// <param name="exception">The exception to set as a result of the value task.</param>
/// <returns><c>true</c> if this is the first call that set the result; otherwise, <c>false</c>.</returns>
public bool TrySetException(Exception exception)
{
return TryComplete(exception);
Expand Down
4 changes: 1 addition & 3 deletions src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,6 @@ namespace System.Net.Quic;
///
/// Each connection can then open outbound stream: <see cref="QuicConnection.OpenOutboundStreamAsync(QuicStreamType, CancellationToken)" />,
/// or accept an inbound stream: <see cref="QuicConnection.AcceptInboundStreamAsync(CancellationToken)" />.
///
/// After all the streams have been finished, connection should be properly closed with an application code: <see cref="CloseAsync(long, CancellationToken)" />.
/// If not, the connection will not send the peer information about being closed and the peer's connection will have to wait on its idle timeout.
/// </remarks>
public sealed partial class QuicConnection : IAsyncDisposable
{
Expand Down Expand Up @@ -177,6 +174,7 @@ public X509Certificate? RemoteCertificate
/// </summary>
public SslApplicationProtocol NegotiatedApplicationProtocol => _negotiatedApplicationProtocol;

/// <inheritdoc cref="ToString"/>
public override string ToString() => _handle.ToString();

/// <summary>
Expand Down
Loading

0 comments on commit 94a444a

Please sign in to comment.