potential fix for #2238 #2243
potential fix for #2238 #2243
Conversation
|
While this avoids destroying the underlying |
|
This is tricky. The protected objects, which is two std::maps, are both swapped for empty maps during "destruction". The swap is not protected and may not be atomic, so this is far from perfect. But protecting it by taking the lock risks causing a deadlock in the destruction sequence, so we don't. wrt to the mutex itself, this is more of a theoretical point, isn't it? Are you proposing that we build a heap allocated singleton containing the mutex and what it protects and accesses it through a pointer in a global variable? |
Accessing an object after its destructor has run results in undefined behavior. While it may happen to work, it's impossible to ensure that it works reliably on all platforms we support. I think we should avoid relying on such things without exceptionally good cause.
From what I can see an approach along those lines would address all of the problems in this area.
Note that a singleton isn't strictly necessary as the variables in question could be independently heap-allocated, for instance. I don't think the heap allocation is strictly necessary either, but it's certainly the simplest approach. |
|
Please check your coverage here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2243/2/Diff_Coverage |
…/fix_mutex_destruction_race
…a/fix_mutex_destruction_race
|
Please check your coverage here: https://ci.realm.io/job/realm/job/realm-core/job/PR-2243/5/Diff_Coverage |
| std::map<std::string, std::weak_ptr<SlabAlloc::MappedFile>> all_files; | ||
| util::Mutex all_files_mutex; | ||
| // prevent destruction at exit (which can lead to races if other threads are still running) | ||
| std::map<std::string, std::weak_ptr<SlabAlloc::MappedFile>>& all_files = |
There was a problem hiding this comment.
These declarations could be good places to use auto as it would avoid repeating the type name.
| // prevent destruction at exit (which can lead to races if other threads are still running) | ||
| util::Mutex& mapping_mutex = *new Mutex; | ||
| std::vector<mapping_and_addr>& mappings_by_addr = *new std::vector<mapping_and_addr>; | ||
| std::vector<mappings_for_file>& mappings_by_file = *new std::vector<mappings_for_file>; |
There was a problem hiding this comment.
will this cause valgrind leak reports?
There was a problem hiding this comment.
I've verified it doesn't - same goes for the address sanitizer. Would be logical if it did, though.
| } | ||
| } at_exit; | ||
| // prevent destruction at exit (which can lead to races if other threads are still running) | ||
| util::Mutex& mapping_mutex = *new Mutex; |
There was a problem hiding this comment.
why not just remove pthread_mutex_destroy() in ~Mutex()?
There was a problem hiding this comment.
that should follow from the discussions above.
|
seems to do the job.. 👍 |
This PR fixes a race between destruction of a global Mutex as part of main thread exit and attempt to lock it on a background thread. (fix for issues #2238, #2137 and #2009)
Note to reviewer: Initial approach was to conditionally disable full destruction of Mutex, but Mark had a better idea which I implemented instead.