-
Notifications
You must be signed in to change notification settings - Fork 115
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.
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
Add misc. x509 un/lock and set1 functions #1449
Conversation
1fd1843
to
1f8a98e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1449 +/- ##
==========================================
- Coverage 76.95% 76.94% -0.02%
==========================================
Files 425 425
Lines 71587 71616 +29
==========================================
+ Hits 55093 55104 +11
- Misses 16494 16512 +18 ☔ View full report in Codecov by Sentry. |
1f8a98e
to
f8f98e4
Compare
4e5a86e
to
bfd778c
Compare
8c1291f
to
032c87f
Compare
std::vector<std::thread> threads; | ||
for (size_t i = 0; i < kNumThreads/2; i++) { | ||
threads.emplace_back([&] { | ||
// Sleep with some jitter to offset thread execution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the jitter? Wouldn't we want them to all start at the same time? If there was enough time during the sleep for other threads to finish would that defeat the point of this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my worry with jitter-less execution is that the adders/getters might be fast enough as to take/release locks while the subsequent threads are spawning, with the threads not getting a chance to contend. maybe this isn't a viable concern; i don't know what the overhead for spawning a thread is like... maybe this is overkill and thread spawn overhead is small enough that we can be confident that contention would occur without any sleeps?
the way it's currently written is that half the threads sleep for some random jitter before adding a cert (which takes then releases a lock internally) while the other half of the threads take a lock immediately, then sleep, then do a lookup and release the lock. my hope is that with the getters all taking locks immediately then sleeping (while holding the lock), it would cause contention with the adders who sleep for a bit before attempting to take a lock.
we can ensure that the two sets of threads always entirely overlap by making the getter lock-holders always sleep for longer than the adders.
282eb9e
to
bdff637
Compare
5ce80d5
to
b0d77b5
Compare
The set1 functions are cribbed from [OpenSSL's implementation][1], but notably differ in that we don't free the `X509_OBJECT`. This is pursuant to OpenSSL's [own convention][2] around "set1" functions: > In the 1 version the ownership of the object is not passed to or > retained by the parent object. Instead a copy or "up ref" of the > object is performed. OpenSSL [frees][3] the input `X509_OBJECT`; we do not. [1]: https://github.com/openssl/openssl/blob/4a6f70c03182b421d326831532edca32bcdb3fb1/crypto/x509/x509_lu.c#L506 [2]: https://www.openssl.org/docs/man3.2/man7/ossl-guide-libraries-introduction.html [3]: https://github.com/openssl/openssl/blob/4a6f70c03182b421d326831532edca32bcdb3fb1/crypto/x509/x509_lu.c#L511
This commit was made because the MacOS CI jobs [seem to have taken issue][1] with how we were doing things previously. [1]: https://github.com/aws/aws-lc/actions/runs/8014000000/job/21891957419?pr=1449
Co-authored-by: Andrew Hopkins <[email protected]>
b0d77b5
to
6568311
Compare
Issues:
n/a
Description of changes:
The set1 functions are mostly cribbed from OpenSSL's implementation.
Call-outs:
n/a
Testing:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.