[All - Fix] Improved threading safety, notably on macOS #130
Conversation
Kodikuu
added
the
2. Review requested
Kodikuu
added
1. Coding
Kodikuu
changed the title
Kodikuu
added
2. Review requested
| @@ -270,6 +291,9 @@ static void *thread_pool_func(void *opaque) | |||
|
|
|||
| uint32_t MTY_ThreadPoolDispatch(MTY_ThreadPool *ctx, MTY_AnonFunc func, void *opaque) | |||
| { | |||
| if (!ctx) | |||
There was a problem hiding this comment.
should we either emit an MTY_Log warning specific to this new error (NULL context being passed) or emit the same MTY_Log warning we do when we can't find an available index?
src/windows/threadw.c
Outdated
| EnterCriticalSection(&ctx->mutex); | ||
| } | ||
|
|
||
| bool MTY_MutexTryLock(MTY_Mutex *ctx) | ||
| { | ||
| if (!ctx) | ||
| return false;; |
There was a problem hiding this comment.
eventually, we would want to emit a log warning for these extraordinary cases, where the code expects to do a thing through a pointer, and the pointer is unexpectedly NULL. handy if we had a WARNING macro that logged the source file name & line number, then it wouldn't take much creativity to come up with a unique log string
…tion (not just a macro) for cross-platform code to use a memory barrier, there must be a function (not just a macro)
Namaneo
added
3. 2nd review stage
|
A follow-up PR from @bmcnett changes the memory barrier to be a function instead of a macro, depending on when this PR is intended to be merged you might want to update it to reflect the upcoming changes. |
Kodikuu
force-pushed
the
thread-safety
branch
from
0954ef9 to
cf54262
Compare
|
Rebased onto new branch and adjusted to use new function. No further changes |
Kodikuu
added
2. Review requested
src/thread.c
Outdated
| @@ -55,17 +55,21 @@ void MTY_RWLockDestroy(MTY_RWLock **rwlock) | |||
| return; | |||
|
|
|||
| MTY_RWLock *ctx = *rwlock; | |||
| *rwlock = NULL; | |||
| MTY_MemoryBarrier(); | |||
There was a problem hiding this comment.
Just as a wild thought, it would be neat to have an MTY_SecureNull(...) function, which would have a this definition:
void *MTY_SecureNull(void **ptr)
{
void *old_ptr = *ptr;
*ptr = NULL;
MTY_MemoryBarrier();
return old_ptr;
}And taking that block as an example, you'd simply do instead: MTY_RWLock *ctx = MTY_SecureNull(rwlock);
Disclaimer: I don't know how memory barriers work internally, i.e. if they're scoped to the executing function or not. If they are, my suggestion just make no sense, please ignore.
There was a problem hiding this comment.
What you're actually after here is an atomic pointer swap, which Bryan does have code for. But I just want to get this work in, because it covers the main issue. Atomically swapping the pointer is less of an improvement (but still one)
See https://github.com/parsec-cloud/libmatoya/commits/thread-safety-barrier/
This reverts commit cf54262.
not yet, for this thing
remnants of the thing we don't want here
and, the spacing
RonaldH-Parsec
added
4. Impl. Reviewed
When destroying threading-related contexts, the NULL assignment is moved to be first.
When calling other threading-related functions, the context is NULL-checked first.
This should help prevent some rare and specific race conditions around the calling of threading functions during or after the destruction of their contexts.