Stop using mutex_destroy(), when multiple threads can still access the mutex
Part of #31614 (moved), alternative to #31735 (moved).
If we can't join all the threads before destroying a mutex (#31735 (moved)), and we can't otherwise prevent multiple thread access, we should stop destroying that mutex. (Because destroying a locked thread invokes undefined behaviour.)
There may be some other pattern that helps us destroy all but one mutex. But that involves a "mutex-destruction" mutex. Which is terribly complex.
Activity
changed milestone to %Tor: 0.4.1.x-final
Trac:
Parent Ticket: #31614 (moved)added 041-backport-maybe 042-should BugSmashFund actualpoints::0.5 component::core tor/tor consider-backport-after-042-stable diagnostics milestone::Tor: 0.4.1.x-final owner::teor parent::31614 points::0.2 priority::medium regression resolution::fixed reviewer::nickm severity::normal status::closed type::defect version::tor 0.3.5.1-alpha labels
I spoke with nickm on IRC, and he agreed:
- we will stop calling mutex destroy on static pthread mutexes
- need to check the windows rules
- we'll have to make sure asan and valgrind don't think it's a leak
- we should leave a comment on the functions that still need to call mutex destroy, and the places where we remove the mutex destroys
- we will stop calling mutex destroy on static pthread mutexes
Here is a draft pull request, I'd like a quick review before continuing:
Here are my questions:
- Are the comments correct?
- Have I missed any calls?
- I skipped some calls that looked local, am I correct?
I still need to:
- write a changes file
- split into multiple commits
- check for any extra uses of these functions after 0.3.5:
- pthread_mutex_destroy()
- tor_mutex_uninit() - non-Windows
- tor_mutex_free() - non-Windows
- atomic_counter_destroy() - non-Windows, but with our atomic counter compat implementation
Trac:
Status: new to needs_review
Cc: N/A to nickm, dgoulet
Hi! This all looks plausible to me at first glance. Here's another question to check on, however:
- Have you thought about what happens on re-initialization?
(Ideally, if Tor finishes, and then is re-initialized in the same process, we would reuse the existing mutexes rather than creating new ones. Is that achievable?)
Trac:
Owner: N/A to teor
Status: needs_review to assignedReplying to nickm:
Hi! This all looks plausible to me at first glance. Here's another question to check on, however:
- Have you thought about what happens on re-initialization?
We must destroy the mutexes, or re-initialising them triggers undefined behaviour. (On both pthreads and Windows threads.) And there is no way to determine if the mutex has already been initialised.
Therefore, we should continue to use the old code, but:
- avoid freeing some locked mutexes by acquiring and releasing the lock in tor_mutex_uninit(), and
- add comments explaining the issue, so we can diagnose any crashes or deadlocks more easily.
Here is my PR:
I think we should leave restart debugging for #31735 (moved), because I am out of time on this issue.
Trac:
Status: assigned to needs_review
Actualpoints: 0.2 to 0.4We should keep this ticket open and consider-backport-if-needed.
I think #31735 (moved) covers all the follow-up we need to do, please feel free to open another ticket if it doesn't.
I found another fix that might be much safer: using a sentinel variable to only initialise global mutexes once, regardless of the number of times that tor is re-initialised. (We already do this with the logs, so that we can keep seeing logs during shutdown.)
I updated the usage comments in this branch, and added a note to #31735 (moved).
Trac:
Status: merge_ready to needs_review
Actualpoints: 0.4 to 0.5-
I like the sentinel-variable fix; this branch seems okay to merge at this point. I'll comment more in #31735 (moved).
Trac:
Status: needs_review to merge_ready This ticket can be merged independently of its parent.
Trac:
Parent: #31614 (moved) to N/A
