Uh oh!
There was an error while loading. Please reload this page.
Don't call user callbacks on MsQuic worker thread. - #98361
Conversation
ghost
commented
Feb 13, 2024
Tagging subscribers to this area: @dotnet/ncl |
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) |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
wfurt
commented
Feb 15, 2024
Would we need to bump minimal expected msquic version? If not how would we expect it to work with current versions? |
rzikm
commented
Feb 15, 2024
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
left a comment
There was a problem hiding this comment.
LGTM, small nits and one last question.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
rzikm
commented
Feb 20, 2024
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
commented
Feb 21, 2024
👍 |
ManickaP
commented
Feb 21, 2024
There are some CI failures that might be relevant though. |
rzikm
commented
Feb 21, 2024
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
commented
Feb 21, 2024
@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>
52cc33b to
9fdb790Comparerzikm
commented
Feb 26, 2024
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
commented
Feb 26, 2024
|
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
commented
Feb 27, 2024
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. |
| 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; | ||
| } |
There was a problem hiding this comment.
once we update to 2.4, we can remove the if and change the StartAsyncCertificateValidation to async void.
| } | ||
| status = QUIC_STATUS_USER_CANCELED; | ||
| result = QUIC_TLS_ALERT_CODES.BAD_CERTIFICATE; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
commented
Feb 28, 2024
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
rzikm
commented
Feb 28, 2024
CI Failures are known and unrelated |
Closes#98039.
This should also help with #55979, as previously the callbacks were delaying MsQuic threads which led to MsQuic thinking the workers were overloaded.