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

avoid NRE in MsQuicConnection ConnectAsync #56283

Merged
merged 7 commits into from
Jul 29, 2021
Merged

avoid NRE in MsQuicConnection ConnectAsync #56283

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f9e3284
avoid NRE in MsQuicConnection ConnectAsync
wfurt Jul 26, 2021
dbc0830
Merge branch 'main' of https://github.com/dotnet/runtime into nre_56263
wfurt Jul 27, 2021
6f3901f
enable disabled test
wfurt Jul 27, 2021
dccf730
one more place where we set ConnectTcs to null
wfurt Jul 27, 2021
4c5e6de
use local variable
wfurt Jul 27, 2021
1d906fb
Merge branch 'main' of https://github.com/dotnet/runtime into nre_56263
wfurt Jul 29, 2021
9426974
feedback from review
wfurt Jul 29, 2021
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
10 changes: 6 additions & 4 deletions src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ private static uint HandleEventConnected(State state, ref ConnectionEvent connec

state.Connected = true;
state.ConnectTcs!.SetResult(MsQuicStatusCodes.Success);
state.ConnectTcs = null;
}

return MsQuicStatusCodes.Success;
Expand Down Expand Up @@ -576,7 +577,7 @@ internal override ValueTask ConnectAsync(CancellationToken cancellationToken = d
throw new Exception($"Unsupported remote endpoint type '{_remoteEndPoint.GetType()}'.");
}

_state.ConnectTcs = new TaskCompletionSource<uint>(TaskCreationOptions.RunContinuationsAsynchronously);
var tcs = _state.ConnectTcs = new TaskCompletionSource<uint>(TaskCreationOptions.RunContinuationsAsynchronously);
wfurt marked this conversation as resolved.
Show resolved Hide resolved

try
{
Expand All @@ -600,7 +601,7 @@ internal override ValueTask ConnectAsync(CancellationToken cancellationToken = d
throw;
}

return new ValueTask(_state.ConnectTcs.Task);
return new ValueTask(tcs.Task);
}

private ValueTask ShutdownAsync(
Expand Down Expand Up @@ -683,9 +684,10 @@ private static uint NativeCallbackHandler(

if (state.ConnectTcs != null)
{
state.ConnectTcs.SetException(ex);
state.ConnectTcs = null;
// This is opportunistic if we get exception and have ability to propagate it to caller.
state.ConnectTcs.TrySetException(ex);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the change I was hinting at when asking about the race condition.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I can do better fix. Instead of checking state of the task I can simply store TaskCompletionSource in ConnectAsync to local variable and return that. There is really no reason to drag it past Connect event.

state.Connection = null;
state.ConnectTcs = null;
}
else
{
Expand Down
1 change: 0 additions & 1 deletion src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,6 @@ public async Task CertificateCallbackThrowPropagates()
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/56263")]
public async Task ConnectWithCertificateCallback()
{
X509Certificate2 c1 = System.Net.Test.Common.Configuration.Certificates.GetServerCertificate();
Expand Down