diff --git a/src/dbzero/bindings/python/PyAPI.cpp b/src/dbzero/bindings/python/PyAPI.cpp index ed2366cb..ae083662 100644 --- a/src/dbzero/bindings/python/PyAPI.cpp +++ b/src/dbzero/bindings/python/PyAPI.cpp @@ -38,6 +38,7 @@ #include #include #include +#include namespace db0::python @@ -1146,7 +1147,7 @@ namespace db0::python PyObject *tryWait(const char *prefix, long state, long timeout) { - std::unique_lock api_lock; + db0::SafeRLock api_lock; { // GIL have to be released to safely lock API db0::python::WithGIL_Unlocked no_gil; diff --git a/src/dbzero/bindings/python/PyLocks.cpp b/src/dbzero/bindings/python/PyLocks.cpp index 520a01b1..77e123f0 100644 --- a/src/dbzero/bindings/python/PyLocks.cpp +++ b/src/dbzero/bindings/python/PyLocks.cpp @@ -4,6 +4,7 @@ #include "PyLocks.hpp" namespace db0::python + { GIL_Lock::GIL_Lock() diff --git a/src/dbzero/bindings/python/PyLocks.hpp b/src/dbzero/bindings/python/PyLocks.hpp index 5add30bc..49e0b205 100644 --- a/src/dbzero/bindings/python/PyLocks.hpp +++ b/src/dbzero/bindings/python/PyLocks.hpp @@ -5,9 +5,7 @@ #include -#define PY_API_FUNC PyThreadState *__save = PyEval_SaveThread(); \ -auto __api_lock = db0::python::PyToolkit::lockApi(); \ -PyEval_RestoreThread(__save); +#define PY_API_FUNC auto __api_lock = db0::python::PyToolkit::lockPyApi(); namespace db0::python diff --git a/src/dbzero/bindings/python/PyToolkit.cpp b/src/dbzero/bindings/python/PyToolkit.cpp index 70f6e42a..e4d4b5bc 100644 --- a/src/dbzero/bindings/python/PyToolkit.cpp +++ b/src/dbzero/bindings/python/PyToolkit.cpp @@ -37,7 +37,7 @@ namespace db0::python PyToolkit::PyWorkspace PyToolkit::m_py_workspace; - std::recursive_mutex PyToolkit::m_api_mutex; + SafeRMutex PyToolkit::m_api_mutex; std::string PyToolkit::getTypeName(ObjectPtr py_object) { return getTypeName(Py_TYPE(py_object)); @@ -709,10 +709,25 @@ namespace db0::python return PyClassObject_Check(py_object); } - std::unique_lock PyToolkit::lockApi() { - return std::unique_lock(m_api_mutex); + SafeRLock PyToolkit::lockApi() { + return { m_api_mutex }; } + SafeRLock PyToolkit::lockPyApi() + { + if (m_api_mutex.isOwnedByThisThread()) { + // already locked by this thread + return {}; + } + + // unlock GIL while waiting for the API mutex + PyThreadState *__save = PyEval_SaveThread(); + auto result = SafeRLock(m_api_mutex); + // restore GIL + PyEval_RestoreThread(__save); + return result; + } + PyToolkit::TypeObjectPtr PyToolkit::getBaseType(TypeObjectPtr py_object) { return py_object->tp_base; } diff --git a/src/dbzero/bindings/python/PyToolkit.hpp b/src/dbzero/bindings/python/PyToolkit.hpp index ec647af3..c7a891cc 100644 --- a/src/dbzero/bindings/python/PyToolkit.hpp +++ b/src/dbzero/bindings/python/PyToolkit.hpp @@ -13,10 +13,7 @@ #include "PyLocks.hpp" #include #include - -#define PY_API_FUNC PyThreadState *__save = PyEval_SaveThread(); \ -auto __api_lock = db0::python::PyToolkit::lockApi(); \ -PyEval_RestoreThread(__save); +#include namespace db0 @@ -240,8 +237,10 @@ namespace db0::python static std::optional getBool(ObjectPtr py_object, const std::string &key); static std::optional getString(ObjectPtr py_object, const std::string &key); - // block until lock acquired - static std::unique_lock lockApi(); + // Blocks until lock acquired + static SafeRLock lockApi(); + // locks API from a Python context (releases GIL while waiting for the lock) + static SafeRLock lockPyApi(); // return base type of TypeObject static TypeObjectPtr getBaseType(TypeObjectPtr py_object); @@ -259,10 +258,10 @@ namespace db0::python // decRef operation for memo objects // @return true if reference count was decremented to zero (!hasRefs) static bool decRefMemo(bool is_tag, ObjectPtr py_object); - + private: static PyWorkspace m_py_workspace; - static std::recursive_mutex m_api_mutex; + static SafeRMutex m_api_mutex; }; } \ No newline at end of file diff --git a/src/dbzero/core/threading/SafeRMutex.cpp b/src/dbzero/core/threading/SafeRMutex.cpp new file mode 100644 index 00000000..df27b400 --- /dev/null +++ b/src/dbzero/core/threading/SafeRMutex.cpp @@ -0,0 +1,35 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +// Copyright (c) 2025 DBZero Software sp. z o.o. + +#include "SafeRMutex.hpp" + +namespace db0 + +{ + + void SafeRMutex::lock() + { + std::thread::id this_id = std::this_thread::get_id(); + // We can use relaxed ordering because we only care if WE wrote it previously. + if (m_owner.load(std::memory_order_relaxed) == this_id) { + ++m_recursion_count; + return; + } + + m_mutex.lock(); + m_owner.store(this_id, std::memory_order_relaxed); + m_recursion_count = 1; + } + + void SafeRMutex::unlock() + { + --m_recursion_count; + + if (m_recursion_count == 0) { + // Clear ownership BEFORE unlocking to avoid race conditions with future lockers + m_owner.store(std::thread::id(), std::memory_order_relaxed); + m_mutex.unlock(); + } + } + +} \ No newline at end of file diff --git a/src/dbzero/core/threading/SafeRMutex.hpp b/src/dbzero/core/threading/SafeRMutex.hpp new file mode 100644 index 00000000..b80b5e0c --- /dev/null +++ b/src/dbzero/core/threading/SafeRMutex.hpp @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +// Copyright (c) 2025 DBZero Software sp. z o.o. + +#pragma once + +#include +#include +#include +#include + +namespace db0 + +{ + + // Safe recursive mutex with thread tracking + // allowing additional checks (e.g. for proper integration with Python GIL) + class SafeRMutex + { + std::mutex m_mutex; + std::atomic m_owner = {}; + int m_recursion_count = 0; + + public: + bool isOwnedByThisThread() const { + return m_owner.load(std::memory_order_relaxed) == std::this_thread::get_id(); + } + + void lock(); + void unlock(); + }; + + class SafeRLock + { + public: + SafeRLock() = default; + SafeRLock(const SafeRLock &) = delete; + + SafeRLock(SafeRLock &&other) noexcept + : m_mutex_ptr(other.m_mutex_ptr) + { + other.m_mutex_ptr = nullptr; + } + + SafeRLock(SafeRMutex &mutex) + : m_mutex_ptr(&mutex) + { + m_mutex_ptr->lock(); + } + + ~SafeRLock() { + unlock(); + } + + void unlock() + { + if (m_mutex_ptr) { + m_mutex_ptr->unlock(); + m_mutex_ptr = nullptr; + } + } + + SafeRLock &operator=(const SafeRLock &) = delete; + + SafeRLock &operator=(SafeRLock &&other) noexcept + { + if (this != &other) { + m_mutex_ptr = other.m_mutex_ptr; + other.m_mutex_ptr = nullptr; + } + return *this; + } + + private: + SafeRMutex *m_mutex_ptr = nullptr; + }; + +} diff --git a/src/dbzero/core/threading/ThreadTracker.cpp b/src/dbzero/core/threading/ThreadTracker.cpp deleted file mode 100644 index 6582dbd0..00000000 --- a/src/dbzero/core/threading/ThreadTracker.cpp +++ /dev/null @@ -1,30 +0,0 @@ -// SPDX-License-Identifier: AGPL-3.0-or-later -// Copyright (c) 2025 DBZero Software sp. z o.o. - -#include "ThreadTracker.hpp" -#include -#include - -namespace db0 - -{ - - std::thread::id ThreadTracker::m_tracked_id; - - void ThreadTracker::beginUnique() { - m_tracked_id = std::this_thread::get_id(); - } - - void ThreadTracker::checkUnique() - { - assert(m_tracked_id == std::thread::id() || m_tracked_id == std::this_thread::get_id()); - if (m_tracked_id != std::thread::id() && m_tracked_id != std::this_thread::get_id()) { - throw std::runtime_error("ThreadTracker: unexpected thread access"); - } - } - - void ThreadTracker::end() { - m_tracked_id = std::thread::id(); - } - -} \ No newline at end of file diff --git a/src/dbzero/core/threading/ThreadTracker.hpp b/src/dbzero/core/threading/ThreadTracker.hpp deleted file mode 100644 index d04717fd..00000000 --- a/src/dbzero/core/threading/ThreadTracker.hpp +++ /dev/null @@ -1,24 +0,0 @@ -// SPDX-License-Identifier: AGPL-3.0-or-later -// Copyright (c) 2025 DBZero Software sp. z o.o. - -#pragma once - -#include - -namespace db0 - -{ - - // A simple tool for debugging treading issues - class ThreadTracker - { - public: - static std::thread::id m_tracked_id; - // begin tracking a specific thread (for which we expect unique access) - static void beginUnique(); - // check if the current thread is a tracked unique one - static void checkUnique(); - static void end(); - }; - -} diff --git a/src/dbzero/workspace/FixtureThreads.cpp b/src/dbzero/workspace/FixtureThreads.cpp index 5ae3c5db..fdb42fc6 100644 --- a/src/dbzero/workspace/FixtureThreads.cpp +++ b/src/dbzero/workspace/FixtureThreads.cpp @@ -4,7 +4,6 @@ #include "FixtureThreads.hpp" #include #include -#include #include "AtomicContext.hpp" #include "LockedContext.hpp" @@ -206,25 +205,19 @@ namespace db0 void AutoCommitThread::onUpdate(Fixture &fixture) { using LangToolkit = db0::object_model::LangConfig::LangToolkit; - + // need to lock the language API first // otherwise it may deadlock on trying to invoke API calls from auto-commit // (e.g. instance destruction triggered by LangCache::clear) auto __api_lock = LangToolkit::lockApi(); - // NOTE: since this a separate thread, we must acuire the language interpreter's lock (where required) + // // NOTE: since this a separate thread, we must acuire the language interpreter's lock (where required) auto lang_lock = LangToolkit::ensureLocked(); -#ifndef NDEBUG - ThreadTracker::beginUnique(); -#endif auto callbacks = fixture.onAutoCommit(); if (!callbacks.empty()) { assert(m_context && "AutoSaveContext must exist here!"); // These callbacks have to be executed when 'everything' is unlocked. Otherwise we are risking a deadlock. m_context->appendCallbacks(std::move(callbacks)); } -#ifndef NDEBUG - ThreadTracker::end(); -#endif } void AutoCommitThread::prepareContext()