Fix SslStreamCertificateContext OCSP test failures #100644
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bartonjs
reviewed
Apr 4, 2024
src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs
Outdated
Show resolved
Hide resolved
bartonjs
reviewed
Apr 4, 2024
Comment on lines
+255
to
+259
| TaskCompletionSource<byte[]?> completionSource = new TaskCompletionSource<byte[]?>(); | ||
|
|
||
| ArraySegment<char> rentedChars = UrlBase64Encoding.RentEncode(encoded); | ||
| byte[]? ret = null; | ||
| _pendingDownload = completionSource.Task; | ||
| FetchOcspAsyncCore(completionSource); | ||
| return completionSource.Task; |
There was a problem hiding this comment.
Does anything bad happen if something starts waiting on _pendingDownload while the TCS's task is still PendingActivation?
There was a problem hiding this comment.
I can't think of anything bad happening, if the TCS wasn't completed yet, then the behavior should be equivalent to waiting for a regular Task returned from an async method (rest of the calling async method being as continuation etc.)
bartonjs
reviewed
Apr 4, 2024
src/libraries/System.Net.Security/tests/UnitTests/SslStreamCertificateContextOcspLinuxTests.cs
Outdated
Show resolved
Hide resolved
This was referenced Apr 4, 2024
|
/azp run runtime-libraries-coreclr outerloop |
|
/azp list |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-coreclr jitstress |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-coreclr jitstressregs |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
CI looks good the reenabled tests did not fail again and other failures are unrelated |
bartonjs
approved these changes
Apr 8, 2024
matouskozak
pushed a commit
to matouskozak/runtime
that referenced
this pull request
Apr 30, 2024
* Fix SslStreamCertificateContext OCSP test failures * Fix code style * Remove unwanted change * fixup! Fix code style * fixup! Remove unwanted change
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #97836.
Fixes #97779.
This PR fixes two data races and adds some internal logging (leftover from debugging, but I figured they can become useful in case we need to debug issues in this area in the future).
First data race was because of following pattern
Both assignments to
_pendingTaskare racing and some of the test failures in the past were due to_pendingTask = nullassignment finishing first.Second data race was test-only. Consider the function
The test assumed that if
GetOcspResponseAsyncdoes not finish synchronously (e.g. no cached OCSP response and new one is being downloaded), the method will always return null. However, it is possible that the task finishes between returning fromGetOcspResponseAsyncand checkingtask.IsCompletedSuccessfully. The tests were adjusted to not depend on this.