From 6029ef1dfc04584d77fb8554cdde51fd8f36f8bd Mon Sep 17 00:00:00 2001 From: scoder Date: Fri, 31 May 2024 15:24:25 +0200 Subject: [PATCH] Prevent deadlock when deallocating Python objects in threads (GH-255) Previously, the deallocation would try to acquire the runtime lock and wait indefinitely because it's held by another thread that runs the Lua code. Failing to acquire the lock will now move the Lua reference to a "to be freed later" list that gets processed before returning from Lua back to Python. Closes https://github.com/scoder/lupa/issues/250 --- lupa/_lupa.pyx | 63 +++++++++++++++++++++++++++++++++++----------- lupa/tests/test.py | 18 +++++++++++++ 2 files changed, 67 insertions(+), 14 deletions(-) diff --git a/lupa/_lupa.pyx b/lupa/_lupa.pyx index 9f100d0..2b07da4 100644 --- a/lupa/_lupa.pyx +++ b/lupa/_lupa.pyx @@ -260,6 +260,7 @@ cdef class LuaRuntime: cdef FastRLock _lock cdef dict _pyrefs_in_lua cdef tuple _raised_exception + cdef list _pending_unrefs cdef bytes _encoding cdef bytes _source_encoding cdef object _attribute_filter @@ -324,6 +325,28 @@ cdef class LuaRuntime: if self._memory_status.limit == -1: self._memory_status.limit -= 1 + @cython.final + cdef void add_pending_unref(self, int ref) noexcept: + pyval: object = ref + if self._pending_unrefs is None: + self._pending_unrefs = [pyval] + else: + self._pending_unrefs.append(pyval) + + @cython.final + cdef int clean_up_pending_unrefs(self) except -1: + if self._pending_unrefs is None or self._state is NULL: + return 0 + + pending_unrefs = self._pending_unrefs + self._pending_unrefs = None + + cdef int ref + L = self._state + for ref in pending_unrefs: + lua.luaL_unref(L, lua.LUA_REGISTRYINDEX, ref) + return 0 + def __dealloc__(self): if self._state is not NULL: lua.lua_close(self._state) @@ -848,8 +871,8 @@ cdef tuple _fix_args_kwargs(tuple args): ################################################################################ # fast, re-entrant runtime locking -cdef inline bint lock_runtime(LuaRuntime runtime) noexcept with gil: - return lock_lock(runtime._lock, pythread.PyThread_get_thread_ident(), True) +cdef inline bint lock_runtime(LuaRuntime runtime, bint blocking=True) noexcept with gil: + return lock_lock(runtime._lock, pythread.PyThread_get_thread_ident(), blocking=blocking) cdef inline void unlock_runtime(LuaRuntime runtime) noexcept nogil: unlock_lock(runtime._lock) @@ -877,15 +900,20 @@ cdef class _LuaObject: def __dealloc__(self): if self._runtime is None: return + runtime = self._runtime + self._runtime = None + ref = self._ref + if ref == lua.LUA_NOREF: + return + self._ref = lua.LUA_NOREF cdef lua_State* L = self._state - if L is not NULL and self._ref != lua.LUA_NOREF: - locked = lock_runtime(self._runtime) - lua.luaL_unref(L, lua.LUA_REGISTRYINDEX, self._ref) - self._ref = lua.LUA_NOREF - runtime = self._runtime - self._runtime = None + if L is not NULL: + locked = lock_runtime(runtime, blocking=False) if locked: + lua.luaL_unref(L, lua.LUA_REGISTRYINDEX, ref) unlock_runtime(runtime) + return + runtime.add_pending_unref(ref) @cython.final cdef inline int push_lua_object(self, lua_State* L) except -1: @@ -1343,15 +1371,20 @@ cdef class _LuaIter: def __dealloc__(self): if self._runtime is None: return + runtime = self._runtime + self._runtime = None + ref = self._refiter + if ref == lua.LUA_NOREF: + return + self._refiter = lua.LUA_NOREF cdef lua_State* L = self._state - if L is not NULL and self._refiter != lua.LUA_NOREF: - locked = lock_runtime(self._runtime) - lua.luaL_unref(L, lua.LUA_REGISTRYINDEX, self._refiter) - self._refiter = lua.LUA_NOREF - runtime = self._runtime - self._runtime = None + if L is not NULL: + locked = lock_runtime(runtime, blocking=False) if locked: + lua.luaL_unref(L, lua.LUA_REGISTRYINDEX, ref) unlock_runtime(runtime) + return + runtime.add_pending_unref(ref) def __repr__(self): return u"LuaIter(%r)" % (self._obj) @@ -1900,6 +1933,7 @@ cdef object execute_lua_call(LuaRuntime runtime, lua_State *L, Py_ssize_t nargs) result_status = lua.lua_pcall(L, nargs, lua.LUA_MULTRET, has_lua_traceback_func) if has_lua_traceback_func: lua.lua_remove(L, 1) + runtime.clean_up_pending_unrefs() results = unpack_lua_results(runtime, L) if result_status: if isinstance(results, BaseException): @@ -2099,6 +2133,7 @@ cdef bint call_python(LuaRuntime runtime, lua_State *L, py_object* py_obj) excep lua.lua_settop(L, 0) # FIXME result = f(*args, **kwargs) + runtime.clean_up_pending_unrefs() return py_function_result_to_lua(runtime, L, result) cdef int py_call_with_gil(lua_State* L, py_object *py_obj) noexcept with gil: diff --git a/lupa/tests/test.py b/lupa/tests/test.py index 3c8f608..dc8b0a2 100644 --- a/lupa/tests/test.py +++ b/lupa/tests/test.py @@ -55,6 +55,7 @@ def _run_gc_test(self, run_test, off_by_one=False): run_test() del i gc.collect() + new_count = len(gc.get_objects()) if off_by_one and old_count == new_count + 1: # FIXME: This happens in test_attrgetter_refcycle - need to investigate why! @@ -2104,6 +2105,23 @@ def mandelbrot(i, lua_func): ## image = Image.fromstring('1', (image_size, image_size), result_bytes) ## image.show() + def test_lua_gc_deadlock(self): + # Delete a Lua reference from a thread while the LuaRuntime is running. + lua = self.lupa.LuaRuntime() + ref = [lua.eval("{}")] + + def trigger_gc(ref): + del ref[0] + + thread = threading.Thread(target=trigger_gc, args=[ref]) + + lua.execute( + "start, join = ...; start(); join()", + thread.start, + thread.join, + ) + assert not thread.is_alive(), "thread didn't finish - deadlock?" + class TestDontUnpackTuples(LupaTestCase): def setUp(self):