20241108-WOLFSSL_CLEANUP_THREADSAFE #8169
Conversation
src/ssl.c
Outdated
| /* WOLFSSL_CLEANUP_THREADSAFE depends on a wolfSSL_Atomic_Int that can | ||
| * be statically initialized. | ||
| */ | ||
| static wolfSSL_Atomic_Int inits_count_mutex_valid2 = 0; |
There was a problem hiding this comment.
wolfSSL_Atomic_Int_Init is not being called anywhere (and that would also defeat the purpose of this PR). While this works for *nix I would consider how we handle this carefully. Maybe a new WOLFSSL_ATOMIC_INITIALIZER is needed.
There was a problem hiding this comment.
Also please use a more descriptive name. I know the context and I had a hard time to figure out what is going on.
There was a problem hiding this comment.
WOLFSSL_ATOMIC_INITIALIZER is a great idea -- then this whole solution can be gated on defined(WOLFSSL_ATOMIC_INITIALIZER).
will do.
There was a problem hiding this comment.
fixed -- inits_count_mutex_atomic_initing_flag.
src/ssl.c
Outdated
| while ((current_inits_count_mutex_valid = inits_count_mutex_valid) == 0); | ||
| if (current_inits_count_mutex_valid < 0) { | ||
| (void)wolfSSL_Atomic_Int_FetchSub(&inits_count_mutex_valid2, 1); | ||
| return BAD_MUTEX_E; | ||
| } |
There was a problem hiding this comment.
I'm not convinced a spin lock is a good idea to hide in the init function. Since this should be a very rare occurrence, I think this should be treated the same as failing to lock a mutex and returning BAD_MUTEX_E.
There was a problem hiding this comment.
that was indeed the other solution I considered. I agree that might be better, but we can't use BAD_MUTEX_E for the error, because the caller needs to be told that the call is retryable. (we already return BAD_MUTEX_E if wc_InitMutex() fails, and that's not retryable.)
with your nudge, I'll switch it to the other solution, and pick or add an appropriate error code that makes it clear by its name that the call should be retried by the application. effectively the spin locking will move to be the application's responsibility, which is better.
…olfSSL_Cleanup().
* add WOLFSSL_ATOMIC_INITIALIZER() to wc_port.h; * rename feature macro to WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS for clarity; * remove spin lock logic in wolfSSL_Init() and instead return DEADLOCK_AVERTED_E on contended initialization; * unless WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS is user-defined to 0, automatically enable it when appropriate.
douzzer
force-pushed
the
20241108-WOLFSSL_CLEANUP_THREADSAFE
branch
6 times, most recently
from
a0706d5
to
28430c5
Compare
| #ifndef WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS | ||
| #if !defined(WOLFSSL_MUTEX_INITIALIZER) && !defined(SINGLE_THREADED) && \ | ||
| defined(WOLFSSL_ATOMIC_OPS) && defined(WOLFSSL_ATOMIC_INITIALIZER) | ||
| #define WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS 1 | ||
| #else | ||
| #define WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS 0 | ||
| #endif | ||
| #elif defined(WOLFSSL_MUTEX_INITIALIZER) || defined(SINGLE_THREADED) | ||
| #undef WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS | ||
| #define WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS 0 | ||
| #endif |
There was a problem hiding this comment.
Why undef & define and just define in the other?
There was a problem hiding this comment.
to force the feature off we define WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS to 0. the user can also force it off by defining to 0.
the top half is followed if WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS has no definition either way, in which case the default setup is used, following the config settings.
the bottom half is followed if the user config activated WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS, but it needs to be forced off because it's not compatible with other config settings.
douzzer
dismissed stale reviews from dgarske and JacobBarthelmeh
via
0ebd86d
douzzer
force-pushed
the
20241108-WOLFSSL_CLEANUP_THREADSAFE
branch
from
28430c5
to
0ebd86d
Compare
src/ssl.c: implementWOLFSSL_CLEANUP_THREADSAFEinwolfSSL_Init()/wolfSSL_Cleanup().ZD#18836
tested with
./configure --enable-all CFLAGS='-DWOLFSSL_TEST_NO_MUTEX_INITIALIZER -DWOLFSSL_CLEANUP_THREADSAFE'andmake check, plus pro formawolfssl-multi-test.sh ... check-source-text