From 3b0cac77eadda611ec144f788b581dbcad81b5e3 Mon Sep 17 00:00:00 2001 From: Wojtek Date: Fri, 12 Dec 2025 17:47:44 +0100 Subject: [PATCH 1/2] safer locks --- src/dbzero/bindings/python/PyAPI.cpp | 3 +- src/dbzero/bindings/python/PyLocks.cpp | 1 + src/dbzero/bindings/python/PyLocks.hpp | 7 ++ src/dbzero/bindings/python/PyToolkit.cpp | 21 +++++- src/dbzero/bindings/python/PyToolkit.hpp | 15 ++-- src/dbzero/core/threading/SafeRMutex.cpp | 35 ++++++++++ src/dbzero/core/threading/SafeRMutex.hpp | 77 +++++++++++++++++++++ src/dbzero/core/threading/ThreadTracker.cpp | 30 -------- src/dbzero/core/threading/ThreadTracker.hpp | 24 ------- src/dbzero/workspace/FixtureThreads.cpp | 11 +-- 10 files changed, 149 insertions(+), 75 deletions(-) create mode 100644 src/dbzero/core/threading/SafeRMutex.cpp create mode 100644 src/dbzero/core/threading/SafeRMutex.hpp delete mode 100644 src/dbzero/core/threading/ThreadTracker.cpp delete mode 100644 src/dbzero/core/threading/ThreadTracker.hpp 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..cab2d658 100644 --- a/src/dbzero/bindings/python/PyLocks.hpp +++ b/src/dbzero/bindings/python/PyLocks.hpp @@ -5,9 +5,16 @@ #include + +/* #define PY_API_FUNC PyThreadState *__save = PyEval_SaveThread(); \ auto __api_lock = db0::python::PyToolkit::lockApi(); \ PyEval_RestoreThread(__save); +*/ + +// FIXME: log +#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() From d521b3c4deb9fd18938d0acd688965d2ea8a5bc9 Mon Sep 17 00:00:00 2001 From: Wojtek Date: Fri, 12 Dec 2025 17:48:56 +0100 Subject: [PATCH 2/2] cleanup --- src/dbzero/bindings/python/PyLocks.hpp | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/dbzero/bindings/python/PyLocks.hpp b/src/dbzero/bindings/python/PyLocks.hpp index cab2d658..49e0b205 100644 --- a/src/dbzero/bindings/python/PyLocks.hpp +++ b/src/dbzero/bindings/python/PyLocks.hpp @@ -5,17 +5,8 @@ #include - -/* -#define PY_API_FUNC PyThreadState *__save = PyEval_SaveThread(); \ -auto __api_lock = db0::python::PyToolkit::lockApi(); \ -PyEval_RestoreThread(__save); -*/ - -// FIXME: log #define PY_API_FUNC auto __api_lock = db0::python::PyToolkit::lockPyApi(); - namespace db0::python {