From a0c8c1d20a98fe2ba25d3e8dae0fe3b489de3c12 Mon Sep 17 00:00:00 2001 From: Stefan Behnel Date: Sun, 28 Jan 2024 13:28:13 +0100 Subject: [PATCH 1/7] Try to solve deadlock when deallocating Python objects in threads. Closes https://github.com/scoder/lupa/issues/250 --- lupa/_lupa.pyx | 63 +++++++++++++++++++++++++++++++++++----------- lupa/tests/test.py | 22 ++++++++++++++++ 2 files changed, 71 insertions(+), 14 deletions(-) diff --git a/lupa/_lupa.pyx b/lupa/_lupa.pyx index 66b10e49..c396f136 100644 --- a/lupa/_lupa.pyx +++ b/lupa/_lupa.pyx @@ -255,6 +255,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 @@ -319,6 +320,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 + + cdef int ref + L = self._state + pending_unrefs = self._pending_unrefs + self._pending_unrefs = None + for ref in pending_unrefs: + lua.luaL_unref(L, lua.LUA_REGISTRYINDEX, ref) + print(f"Cleaned up {len(pending_unrefs)} Lua refs") # TODO: remove + return 0 + def __dealloc__(self): if self._state is not NULL: lua.lua_close(self._state) @@ -793,8 +816,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) @@ -822,15 +845,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: @@ -1288,15 +1316,20 @@ cdef class _LuaIter: def __dealloc__(self): if self._runtime is None: return + runtime = self._runtime + self._runtime = None + ref = self._refiter + if self._refiter == 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) @@ -1829,6 +1862,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): @@ -2028,6 +2062,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 cbb13580..62038e5d 100644 --- a/lupa/tests/test.py +++ b/lupa/tests/test.py @@ -102,6 +102,28 @@ def get_attr(obj, name): # Seems related to running the test twice in the same Lupa module? self._run_gc_test(make_refcycle, off_by_one=True) + def test_lupa_gc_deadlock(self): + lua = self.lupa.LuaRuntime() + + def assert_no_deadlock(thread): + thread.start() + thread.join(1) + assert not thread.is_alive(), "thread didn't finish - deadlock?" + + def delete_table_reference_in_thread(): + ref = [lua.eval("{}")] + + def trigger_gc(ref): + del ref[0] + + lua.execute( + "f,x=...; f(x)", + assert_no_deadlock, + threading.Thread(target=trigger_gc, args=[ref]), + ) + + self._run_gc_test(delete_table_reference_in_thread) + class TestLuaRuntime(SetupLuaRuntimeMixin, LupaTestCase): def test_lua_version(self): From c0889a516b1db87b03a2b89659b170439a01a575 Mon Sep 17 00:00:00 2001 From: Stefan Behnel Date: Wed, 29 May 2024 11:48:16 +0200 Subject: [PATCH 2/7] Add methods LuaRuntime.gccollect() and LuaRuntime.nogc() to control the Lua garbage collector. --- CHANGES.rst | 2 ++ lupa/_lupa.pyx | 48 ++++++++++++++++++++++++++++++++++++++++++++++ lupa/tests/test.py | 14 ++++++++++++++ 3 files changed, 64 insertions(+) diff --git a/CHANGES.rst b/CHANGES.rst index 9a942b16..f6f503c2 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -16,6 +16,8 @@ Lupa change log explicitly at need. (original patch by Richard Connon) +* A new method ``LuaRuntime.gc()`` was added to control the Lua garbage collector. + * The bundled Lua 5.1 was updated to 5.1.5 and Lua 5.2 to 5.2.4. (patch by xxyzz) diff --git a/lupa/_lupa.pyx b/lupa/_lupa.pyx index c396f136..eaaa62c9 100644 --- a/lupa/_lupa.pyx +++ b/lupa/_lupa.pyx @@ -561,6 +561,23 @@ cdef class LuaRuntime: finally: unlock_runtime(self) + def nogc(self): + """ + Return a context manager that temporarily disables the Lua garbage collector. + """ + return _LuaNoGC(self) + + def gccollect(self): + """ + Run a full pass of the Lua garbage collector. + """ + assert self._state is not NULL + cdef lua_State *L = self._state + lock_runtime(self) + # Pass third argument for compatibility with Lua 5.[123]. + lua.lua_gc(L, lua.LUA_GCCOLLECT, 0) + unlock_runtime(self) + def set_max_memory(self, size_t max_memory, total=False): """Set maximum allowed memory for this LuaRuntime. @@ -660,6 +677,37 @@ cdef class LuaRuntime: return 0 # nothing left to return on the stack +@cython.internal +cdef class _LuaNoGC: + """ + A context manager that temporarily disables the Lua garbage collector. + """ + cdef LuaRuntime _runtime + + def __cinit__(self, LuaRuntime runtime not None): + self._runtime = runtime + + def __enter__(self): + if self._runtime is None: + return # e.g. system teardown + assert self._runtime._state is not NULL + cdef lua_State *L = self._runtime._state + lock_runtime(self._runtime) + # Pass third argument for compatibility with Lua 5.[123]. + lua.lua_gc(L, lua.LUA_GCSTOP, 0) + unlock_runtime(self._runtime) + + def __exit__(self, *exc): + if self._runtime is None: + return # e.g. system teardown + assert self._runtime._state is not NULL + cdef lua_State *L = self._runtime._state + lock_runtime(self._runtime) + # Pass third argument for compatibility with Lua 5.[123]. + lua.lua_gc(L, lua.LUA_GCRESTART, 0) + unlock_runtime(self._runtime) + + ################################################################################ # decorators for calling Python functions with keyword (named) arguments # from Lua scripts diff --git a/lupa/tests/test.py b/lupa/tests/test.py index 62038e5d..9be86901 100644 --- a/lupa/tests/test.py +++ b/lupa/tests/test.py @@ -139,6 +139,20 @@ def test_lua_implementation(self): self.assertTrue(lua_implementation.startswith("Lua"), lua_implementation) self.assertTrue(lua_implementation.split()[0] in ("Lua", "LuaJIT"), lua_implementation) + def test_lua_gccollect(self): + self.lua.gccollect() + + def test_lua_nogc(self): + if self.lua.lua_version >= (5,2): + self.assertTrue(self.lua.eval('collectgarbage("isrunning")')) + + with self.lua.nogc(): + if self.lua.lua_version >= (5,2): + self.assertFalse(self.lua.eval('collectgarbage("isrunning")')) + + if self.lua.lua_version >= (5,2): + self.assertTrue(self.lua.eval('collectgarbage("isrunning")')) + def test_eval(self): self.assertEqual(2, self.lua.eval('1+1')) From c10e400af43a637ee3cec19219956b15f55bf957 Mon Sep 17 00:00:00 2001 From: Stefan Behnel Date: Wed, 29 May 2024 11:57:58 +0200 Subject: [PATCH 3/7] Minor code cleanups. --- lupa/_lupa.pyx | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lupa/_lupa.pyx b/lupa/_lupa.pyx index eaaa62c9..e84afa00 100644 --- a/lupa/_lupa.pyx +++ b/lupa/_lupa.pyx @@ -333,10 +333,11 @@ cdef class LuaRuntime: if self._pending_unrefs is None or self._state is NULL: return 0 - cdef int ref - L = self._state 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) print(f"Cleaned up {len(pending_unrefs)} Lua refs") # TODO: remove @@ -1367,7 +1368,7 @@ cdef class _LuaIter: runtime = self._runtime self._runtime = None ref = self._refiter - if self._refiter == lua.LUA_NOREF: + if ref == lua.LUA_NOREF: return self._refiter = lua.LUA_NOREF cdef lua_State* L = self._state From e97ee192b8ca62da5e29b064db24f95779e4e486 Mon Sep 17 00:00:00 2001 From: Stefan Behnel Date: Wed, 29 May 2024 11:58:45 +0200 Subject: [PATCH 4/7] Make sure Lua cleans up old references in refcounting test. --- lupa/tests/test.py | 1 + 1 file changed, 1 insertion(+) diff --git a/lupa/tests/test.py b/lupa/tests/test.py index 9be86901..272cac41 100644 --- a/lupa/tests/test.py +++ b/lupa/tests/test.py @@ -121,6 +121,7 @@ def trigger_gc(ref): assert_no_deadlock, threading.Thread(target=trigger_gc, args=[ref]), ) + lua.gccollect() self._run_gc_test(delete_table_reference_in_thread) From 7b649d2bd5fc72b1d8e4d4976be4eb0b2d4a7d3d Mon Sep 17 00:00:00 2001 From: Stefan Behnel Date: Wed, 29 May 2024 21:43:31 +0200 Subject: [PATCH 5/7] Undo accidental merge artefact. --- CHANGES.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES.rst b/CHANGES.rst index 2341ceb0..3760df0d 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -26,7 +26,7 @@ Lupa change log explicitly at need. (original patch by Richard Connon) -* The bundled Lua 5.1 was updated to 5.1.5 and Lua 5.2 to 5.2.4. +* GH#234: The bundled Lua 5.1 was updated to 5.1.5 and Lua 5.2 to 5.2.4. (patch by xxyzz) * The bundled Lua 5.4 was updated to 5.4.6. From 2e2c2038c88c5dccf0496eacde506338f12b6859 Mon Sep 17 00:00:00 2001 From: Stefan Behnel Date: Thu, 30 May 2024 17:47:30 +0200 Subject: [PATCH 6/7] Avoid one-time threading initialisation from interfering with refcounting test. --- lupa/tests/test.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/lupa/tests/test.py b/lupa/tests/test.py index 486887c1..ca4ecb2d 100644 --- a/lupa/tests/test.py +++ b/lupa/tests/test.py @@ -103,8 +103,6 @@ def get_attr(obj, name): self._run_gc_test(make_refcycle, off_by_one=True) def test_lupa_gc_deadlock(self): - lua = self.lupa.LuaRuntime() - def assert_no_deadlock(thread): thread.start() thread.join(1) @@ -123,6 +121,14 @@ def trigger_gc(ref): ) lua.gccollect() + # Pre-initialise threading outside of the refcount checks. + lua = self.lupa.LuaRuntime() + assert_no_deadlock(threading.Thread()) + delete_table_reference_in_thread() + gc.collect() + + # Run test. + lua = self.lupa.LuaRuntime() self._run_gc_test(delete_table_reference_in_thread) From 866469b80cf5f5282a09259387ab82df6c02aa01 Mon Sep 17 00:00:00 2001 From: Stefan Behnel Date: Thu, 30 May 2024 18:11:24 +0200 Subject: [PATCH 7/7] Simplify test, move it to a better place and remove debug code. --- lupa/_lupa.pyx | 1 - lupa/tests/test.py | 47 ++++++++++++++++++---------------------------- 2 files changed, 18 insertions(+), 30 deletions(-) diff --git a/lupa/_lupa.pyx b/lupa/_lupa.pyx index 15df87c8..2b07da44 100644 --- a/lupa/_lupa.pyx +++ b/lupa/_lupa.pyx @@ -345,7 +345,6 @@ cdef class LuaRuntime: L = self._state for ref in pending_unrefs: lua.luaL_unref(L, lua.LUA_REGISTRYINDEX, ref) - print(f"Cleaned up {len(pending_unrefs)} Lua refs") # TODO: remove return 0 def __dealloc__(self): diff --git a/lupa/tests/test.py b/lupa/tests/test.py index ca4ecb2d..dc8b0a2b 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! @@ -102,35 +103,6 @@ def get_attr(obj, name): # Seems related to running the test twice in the same Lupa module? self._run_gc_test(make_refcycle, off_by_one=True) - def test_lupa_gc_deadlock(self): - def assert_no_deadlock(thread): - thread.start() - thread.join(1) - assert not thread.is_alive(), "thread didn't finish - deadlock?" - - def delete_table_reference_in_thread(): - ref = [lua.eval("{}")] - - def trigger_gc(ref): - del ref[0] - - lua.execute( - "f,x=...; f(x)", - assert_no_deadlock, - threading.Thread(target=trigger_gc, args=[ref]), - ) - lua.gccollect() - - # Pre-initialise threading outside of the refcount checks. - lua = self.lupa.LuaRuntime() - assert_no_deadlock(threading.Thread()) - delete_table_reference_in_thread() - gc.collect() - - # Run test. - lua = self.lupa.LuaRuntime() - self._run_gc_test(delete_table_reference_in_thread) - class TestLuaRuntime(SetupLuaRuntimeMixin, LupaTestCase): def assertLuaResult(self, lua_expression, result): @@ -2133,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):