Don't call user callbacks on MsQuic worker thread. #98361

rzikm · 2024-02-13T15:52:01Z

This should also help with #55979, as previously the callbacks were delaying MsQuic threads which led to MsQuic thinking the workers were overloaded.

ghost · 2024-02-13T15:52:13Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Closes #98039.

Author:	rzikm
Assignees:	-
Labels:	`area-System.Net.Quic`
Milestone:	-

rzikm · 2024-02-14T12:17:48Z

For now, one test keeps failing due to microsoft/msquic#4132. We'll have to wait until that issue is fixed and new MsQuic bits flow to all CI images (or disable the test meanwhile)

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.NativeMethods.cs

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs

wfurt · 2024-02-15T05:43:34Z

For now, one test keeps failing due to microsoft/msquic#4132. We'll have to wait until that issue is fixed and new MsQuic bits flow to all CI images (or disable the test meanwhile)

Would we need to bump minimal expected msquic version? If not how would we expect it to work with current versions?

rzikm · 2024-02-15T09:21:56Z

Would we need to bump minimal expected msquic version? If not how would we expect it to work with current versions?

I would prefer to bump the minimum required version. But we can alternatively do a version check and do the work inline as we did until now.

CarnaViire · 2024-02-19T12:23:23Z

I would prefer to bump the minimum required version. But we can alternatively do a version check and do the work inline as we did until now.

Just a reminder, that Alpine Arm32 is pinned on an older msquic version due to a regression microsoft/msquic#3958

ManickaP

LGTM, small nits and one last question.

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs

rzikm · 2024-02-20T19:20:38Z

I think the PR is ready for the final round of review. However, we need to figure out how to deal with microsoft/msquic#4132. Given that some images are pinned on an older version due to microsoft/msquic#3958, I think we should add a version check and do the async cert validation inline on older version (with minimal code changes), and later bump minimum required MsQuic version and remove the version check.

ManickaP · 2024-02-21T12:44:40Z

I think we should add a version check and do the async cert validation inline on older version (with minimal code changes), and later bump minimum required MsQuic version and remove the version check.

👍

ManickaP

LGTM, thanks!

ManickaP · 2024-02-21T13:14:38Z

There are some CI failures that might be relevant though.

rzikm · 2024-02-21T13:19:24Z

There are some CI failures that might be relevant though.

This is the failure caused by microsoft/msquic#4132. Once microsoft/msquic#4145 is merged I can add a version check which will make the test pass again.

System.Net.Quic.Tests.MsQuicTests.CertificateCallbackThrowPropagates [FAIL]
Assert.Throws() Failure: Exception type was not an exact match
Expected: typeof(System.Security.Authentication.AuthenticationException)
Actual:   typeof(System.Net.Quic.QuicException)
---- System.Net.Quic.QuicException : Connection handshake was canceled due to the configured timeout of 00:00:10 seconds elapsing.
-------- System.OperationCanceledException : The operation was canceled.

CarnaViire · 2024-02-21T15:50:02Z

This is the failure caused by microsoft/msquic#4132. Once microsoft/msquic#4145 is merged I can add a version check which will make the test pass again.

Then we need to either create an issue on updating the version check after MsQuic is updated (I think it's not just a matter of merging the PR, we need a new release, right?), and disable the test; or we need to put this PR into draft and "do-not-merge" to block until we can address it here.

UPD: Ah, I see that "no merge" is already here; but the label is not checked as part of CI, and it's not easy to notice it 🙈

CarnaViire · 2024-02-21T16:11:55Z

@rzikm I'll go ahead and mark it as draft while it's blocked -- so that it will not appear in the stale PRs list (and will not get accidentally merged)

Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com>

rzikm · 2024-02-26T15:05:16Z

microsoft/msquic#4132 was fixed, targeting 2.4 release, so I added version check to do the asynchronous validation only on 2.4 and higher. We can remove the version check once we update minimum required MsQuic version.

wfurt · 2024-02-26T16:38:21Z

MsQuicTests.CertificateCallbackThrowPropagates failure seems related.

rzikm · 2024-02-27T13:26:30Z

Interesting, the test should not be failing because it should be still running inline. I can't even reproduce the failures on my machine with the same msquic versions as in the test runs

rzikm · 2024-02-27T16:22:40Z

Looks like the failure condition is still there even if we invoke the callback synchronously, so we need to return the result directly from the handler function.

rzikm · 2024-02-27T18:30:58Z

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs

+        var task = _sslConnectionOptions.StartAsyncCertificateValidation((IntPtr)data.Certificate, (IntPtr)data.Chain);
+        if (task.IsCompletedSuccessfully)
        {
-            return _sslConnectionOptions.ValidateCertificate((QUIC_BUFFER*)data.Certificate, (QUIC_BUFFER*)data.Chain, out _remoteCertificate);
-        }
-        catch (Exception ex)
-        {
-            _connectedTcs.TrySetException(ex);
-            return QUIC_STATUS_HANDSHAKE_FAILURE;
+            return task.Result ? QUIC_STATUS_SUCCESS : QUIC_STATUS_BAD_CERTIFICATE;
        }


once we update to 2.4, we can remove the if and change the StartAsyncCertificateValidation to async void.

ManickaP · 2024-02-28T08:31:05Z

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs

                    {
                        wrapException = false;
                        if (_isClient)
                        {
                            throw new AuthenticationException(SR.net_quic_cert_custom_validation);
                        }

-                        status = QUIC_STATUS_USER_CANCELED;
+                        result = QUIC_TLS_ALERT_CODES.BAD_CERTIFICATE;


Are we losing now the specific errors we were returning? Like discerning user_cancelled? Does it matter (affects the other side and what info they get), or not?

MsQuic does not use the specific error code value, it only checks for PENDING for async validation, SUCCESS for accept, and any error simply means reject.

With the async validation, we will actually be able to select the right TLS alert code which will go out over the wire, If the user validation callback throws, this will default to USER_CANCELLED as before, but if the callback returns false then it becomes BAD_CERTIFICATE. We can perhaps be more specific when no custom validation is present (there are alerts for stuff like UNTRUSTED_CERTIFICATE and similar) but I don't think we do that even in SslStream on some platforms.

rzikm · 2024-02-28T12:56:20Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2024-02-28T12:56:33Z

Azure Pipelines successfully started running 1 pipeline(s).

rzikm · 2024-02-28T14:51:38Z

CI Failures are known and unrelated

dotnet-issue-labeler bot added the area-System.Net.Quic label Feb 13, 2024

ghost assigned rzikm Feb 13, 2024

rzikm changed the title ~~98039-QuicConnection-Validate-certificates-asynchronously~~ Don't call user callbacks on MsQuic worker thread. Feb 13, 2024

rzikm marked this pull request as ready for review February 14, 2024 12:16

rzikm requested a review from a team February 14, 2024 12:18

build-analysis bot mentioned this pull request Feb 14, 2024

Segmentation fault in System.Text.RegularExpressions.Tests #93206

Closed

ManickaP reviewed Feb 14, 2024

View reviewed changes

ManickaP mentioned this pull request Feb 16, 2024

Privately expose MsQuic performance counters #98422

Merged

rzikm requested review from stephentoub and ManickaP February 19, 2024 14:49

ManickaP reviewed Feb 20, 2024

View reviewed changes

rzikm requested a review from ManickaP February 20, 2024 19:16

rzikm added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 20, 2024

ManickaP approved these changes Feb 21, 2024

View reviewed changes

CarnaViire changed the title ~~Don't call user callbacks on MsQuic worker thread.~~ [NO-MERGE] Don't call user callbacks on MsQuic worker thread. Feb 21, 2024

rzikm and others added 9 commits February 26, 2024 16:03

Don't call callbacks from MsQuic threads

5b3d46b

Remove unintentional changes

08cf807

Offload parsing to threadpool as well

702ea94

Customize TLS ALERT code

17de07f

Code review feedback

3b23b49

Apply suggestions from code review

fce941c

Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com>

use ConfigureAwaitOptions.ForceYielding

c595b05

Version check to work around microsoft/msquic#4132

6b9142f

Use configure await to yield to threadpool

9fdb790

rzikm force-pushed the 98039-QuicConnection-Validate-certificates-asynchronously branch from 52cc33b to 9fdb790 Compare February 26, 2024 15:03

rzikm marked this pull request as ready for review February 26, 2024 15:05

rzikm removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 26, 2024

rzikm changed the title ~~[NO-MERGE] Don't call user callbacks on MsQuic worker thread.~~ Don't call user callbacks on MsQuic worker thread. Feb 27, 2024

Fix functionality on older msquic versions

2579291

rzikm commented Feb 27, 2024

View reviewed changes

This was referenced Feb 27, 2024

Tracking issue for CI build timeouts #76454

Closed

[mono][tvos] OOM in System.IO.Tests.MemoryStreamTests #92467

Closed

CarnaViire mentioned this pull request Feb 27, 2024

Disable QUIC tests on alpine arm32 #99020

Merged

ManickaP reviewed Feb 28, 2024

View reviewed changes

rzikm merged commit 56af107 into dotnet:main Feb 28, 2024
105 of 119 checks passed

rzikm mentioned this pull request Feb 28, 2024

QUIC: MsQuicListener can randomly fail to accept new connection #55979

Closed

github-actions bot locked and limited conversation to collaborators Mar 30, 2024

karelz added this to the 9.0.0 milestone May 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't call user callbacks on MsQuic worker thread. #98361

Don't call user callbacks on MsQuic worker thread. #98361

rzikm commented Feb 13, 2024 •

edited

Loading

ghost commented Feb 13, 2024

rzikm commented Feb 14, 2024 •

edited

Loading

wfurt commented Feb 15, 2024

rzikm commented Feb 15, 2024

CarnaViire commented Feb 19, 2024 •

edited

Loading

ManickaP left a comment

rzikm commented Feb 20, 2024

ManickaP commented Feb 21, 2024

ManickaP left a comment

ManickaP commented Feb 21, 2024

rzikm commented Feb 21, 2024

CarnaViire commented Feb 21, 2024 •

edited

Loading

CarnaViire commented Feb 21, 2024

rzikm commented Feb 26, 2024

wfurt commented Feb 26, 2024

rzikm commented Feb 27, 2024 •

edited

Loading

rzikm commented Feb 27, 2024

rzikm Feb 27, 2024 •

edited

Loading

ManickaP Feb 28, 2024

rzikm Feb 28, 2024

rzikm commented Feb 28, 2024

azure-pipelines bot commented Feb 28, 2024

rzikm commented Feb 28, 2024

Don't call user callbacks on MsQuic worker thread. #98361

Don't call user callbacks on MsQuic worker thread. #98361

Conversation

rzikm commented Feb 13, 2024 • edited Loading

ghost commented Feb 13, 2024

rzikm commented Feb 14, 2024 • edited Loading

wfurt commented Feb 15, 2024

rzikm commented Feb 15, 2024

CarnaViire commented Feb 19, 2024 • edited Loading

ManickaP left a comment

Choose a reason for hiding this comment

rzikm commented Feb 20, 2024

ManickaP commented Feb 21, 2024

ManickaP left a comment

Choose a reason for hiding this comment

ManickaP commented Feb 21, 2024

rzikm commented Feb 21, 2024

CarnaViire commented Feb 21, 2024 • edited Loading

CarnaViire commented Feb 21, 2024

rzikm commented Feb 26, 2024

wfurt commented Feb 26, 2024

rzikm commented Feb 27, 2024 • edited Loading

rzikm commented Feb 27, 2024

rzikm Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

ManickaP Feb 28, 2024

Choose a reason for hiding this comment

rzikm Feb 28, 2024

Choose a reason for hiding this comment

rzikm commented Feb 28, 2024

azure-pipelines bot commented Feb 28, 2024

rzikm commented Feb 28, 2024

rzikm commented Feb 13, 2024 •

edited

Loading

rzikm commented Feb 14, 2024 •

edited

Loading

CarnaViire commented Feb 19, 2024 •

edited

Loading

CarnaViire commented Feb 21, 2024 •

edited

Loading

rzikm commented Feb 27, 2024 •

edited

Loading

rzikm Feb 27, 2024 •

edited

Loading