Properly unlock locked mutexes on thread cleanup. #14262
Conversation
This comment has been minimized.
This comment has been minimized.
3a2fb18 to
4c2c911
Compare
thread_sync.c
Outdated
| { | ||
| rb_mutex_t *mutex = ptr; | ||
| if (mutex->fiber) { | ||
| rb_gc_mark_movable(rb_fiberptr_self(mutex->fiber)); |
There was a problem hiding this comment.
You use rb_gc_mark_movable but mutex_data_type doesn't register a dcompact method.
There was a problem hiding this comment.
👋 Doesn't this just mark the fiber without pinning it? Oh right, then we would need our own way to update its references.
Edit: John just mentioned to me that we would also probably need write barriers for fiber. John and I will discuss later this week.
There was a problem hiding this comment.
Yes precisely. And if you don't have a dcompact callback, by not pinning it, you allow it to move, if that does happen, the ref might be corrupted.
Now in this case it turns out it's alright because the ref is inside a struct you don't own, and the Fiber will update its own reference. But I'd either add a very clear comment mentioning this assumption, or just implement dcompact here too.
Mutexes were being improperly unlocked on thread cleanup. This bug was introduced in 050a895. We must keep a reference from the mutex to the thread, because if the fiber is collected before mutex is, then we cannot unlink it from the thread in `mutex_free`. If it's not unlinked from the the thread when it's freed, it causes bugs in `rb_thread_unlock_all_locking_mutexes`. Fixes [Bug #21342]
4c2c911 to
150b16a
Compare
Mutexes were being improperly unlocked on thread cleanup. This bug was introduced in
050a895.
We must keep a reference from the mutex to the thread, because if the fiber
is collected before the mutex is, then we cannot unlink it from the thread
in
mutex_free. If it's not unlinked from the the thread when it's freed, itcauses bugs in
rb_thread_unlock_all_locking_mutexes.Fixes [Bug #21342]