diff --git a/src/tsutil/DbgCtl.cc b/src/tsutil/DbgCtl.cc index 99f35a0b1ac..0263c31139c 100644 --- a/src/tsutil/DbgCtl.cc +++ b/src/tsutil/DbgCtl.cc @@ -150,6 +150,8 @@ DbgCtl::_new_reference(char const *tag) DebugInterface *p = DebugInterface::get_instance(); debug_assert(tag != nullptr); + _TagData *new_tag_data{nullptr}; + // DbgCtl instances may be declared as static objects in the destructors of objects not destroyed till program exit. // So, we must handle the case where the construction of such instances of DbgCtl overlaps with the destruction of // other instances of DbgCtl. That is why it is important to make sure the reference count is non-zero before @@ -157,42 +159,46 @@ DbgCtl::_new_reference(char const *tag) // the Registry, the new Registry will not be destroyed before the mutex in the new Registry is locked. ++_RegistryAccessor::registry_reference_count; + { + _RegistryAccessor ra; - // There is a mutex in the C/C++ runtime that both dlopen() and _cxa_thread_atexit() lock while running. - // Creating a _RegistryAccessor instance locks the registry mutex. If the subsequent code in this function triggers - // the construction of a thread_local variable (with a non-trivial destructor), the following deadlock scenario is - // possible: - // 1. Thread 1 calls a DbgCtl constructor, which locks the registry mutex, but then is suspended. - // 2. Thread 2 calls dlopen() for a plugin, locking the runtime mutex. It then executes the constructor for a - // statically allocated DbgCtl object, which blocks on locking the registry mutex. - // 3. Thread 1 resumes, and calls member functions of the derived class of DebugInterface. If this causes the - // the construction of a thread_local variable with a non-trivial destructor, _cxa_thread_atexit() will be called - // to set up a call of the variable's destructor at thread exit. The call to _cxa_thread_atexit() will block on - // the runtime mutex (held by Thread 2). So Thread 1 holds the registry mutex and is blocked waiting for the - // runtime mutex. And Thread 2 holds the runtime mutex and is blocked waiting for the registry mutex. Deadlock. - // - // This deadlock is avoided by having the thread_local variable register its destruction in a non-thread_local class. - _RegistryAccessor ra; + auto &d{ra.data()}; - auto &d{ra.data()}; + if (auto it = d.map.find(tag); it != d.map.end()) { + return &*it; + } - if (auto it = d.map.find(tag); it != d.map.end()) { - return &*it; - } + auto sz = std::strlen(tag); - auto sz = std::strlen(tag); + debug_assert(sz > 0); - debug_assert(sz > 0); + char *t = new char[sz + 1]; // Deleted by ~Registry(). + std::memcpy(t, tag, sz + 1); + _TagData new_elem{t, false}; - char *t = new char[sz + 1]; // Deleted by ~Registry(). - std::memcpy(t, tag, sz + 1); - _TagData new_elem{t, p && p->debug_tag_activated(tag)}; + auto res = d.map.insert(new_elem); - auto res = d.map.insert(new_elem); + debug_assert(res.second); + + new_tag_data = &*res.first; + } + new_tag_data->second = p && p->debug_tag_activated(tag); - debug_assert(res.second); + // It is important that debug_tag_activated() is NOT called while the ra object exists, and the registry mutex is + // locked. There is a mutex in the C/C++ runtime that both dlopen() and _cxa_thread_atexit() lock while running. + // Creating a _RegistryAccessor instance locks the registry mutex. If the subsequent code in this function triggers + // the construction of a thread_local variable (with a non-trivial destructor), with the registry mutex locked, the + // following deadlock scenario is possible: + // 1. Thread 1 calls a DbgCtl constructor, which locks the registry mutex, but then is suspended. + // 2. Thread 2 calls dlopen() for a plugin, locking the runtime mutex. It then executes the constructor for a + // statically allocated DbgCtl object, which blocks on locking the registry mutex. + // 3. Thread 1 resumes, and calls a function that causes the the construction of a thread_local variable with a + // non-trivial destructor. This causes a call to _cxa_thread_atexit(), to set up a call of the variable's + // destructor at thread exit. The call to _cxa_thread_atexit() will block on the runtime mutex (held by Thread 2). + // So Thread 1 holds the registry mutex and is blocked waiting for the runtime mutex. And Thread 2 holds the + // runtime mutex and is blocked waiting for the registry mutex. Deadlock. - return &*res.first; + return new_tag_data; } void diff --git a/src/tsutil/Regex.cc b/src/tsutil/Regex.cc index c5beb88e5ce..7dd02b6fe58 100644 --- a/src/tsutil/Regex.cc +++ b/src/tsutil/Regex.cc @@ -51,19 +51,6 @@ my_free(void *ptr, void * /*caller*/) free(ptr); } -class RegexContext; // defined below -class RegexContextCleanup -{ -public: - void push_back(RegexContext *ctx); - ~RegexContextCleanup(); - -private: - std::vector _contexts; - std::mutex _mutex; -}; -RegexContextCleanup regex_context_cleanup; - //---------------------------------------------------------------------------- class RegexContext { @@ -71,20 +58,11 @@ class RegexContext static RegexContext * get_instance() { - if (_shutdown == true) { - return nullptr; - } - - if (_regex_context == nullptr) { - _regex_context = new RegexContext(); - regex_context_cleanup.push_back(_regex_context); - } - return _regex_context; + thread_local RegexContext ctx; + return &ctx; } ~RegexContext() { - _shutdown = true; - if (_general_context != nullptr) { pcre2_general_context_free(_general_context); } @@ -123,33 +101,12 @@ class RegexContext _jit_stack = pcre2_jit_stack_create(4096, 1024 * 1024, nullptr); // 1 page min and 1MB max pcre2_jit_stack_assign(_match_context, nullptr, _jit_stack); } - pcre2_general_context *_general_context = nullptr; - pcre2_compile_context *_compile_context = nullptr; - pcre2_match_context *_match_context = nullptr; - pcre2_jit_stack *_jit_stack = nullptr; - thread_local static RegexContext *_regex_context; - static bool _shutdown; // flag to indicate destructor was called, so no new instances can be created + pcre2_general_context *_general_context = nullptr; + pcre2_compile_context *_compile_context = nullptr; + pcre2_match_context *_match_context = nullptr; + pcre2_jit_stack *_jit_stack = nullptr; }; -thread_local RegexContext *RegexContext::_regex_context = nullptr; -bool RegexContext::_shutdown = false; - -//---------------------------------------------------------------------------- - -RegexContextCleanup::~RegexContextCleanup() -{ - std::lock_guard guard(_mutex); - for (auto ctx : _contexts) { - delete ctx; - } -} -void -RegexContextCleanup::push_back(RegexContext *ctx) -{ - std::lock_guard guard(_mutex); - _contexts.push_back(ctx); -} - } // namespace //----------------------------------------------------------------------------