From 2b6620b811150ffe71309282c17ecb20d8a29227 Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Wed, 20 Mar 2019 00:28:08 -0400 Subject: [PATCH] Fix memory leak related to call_later() and call_at() The crux of the problem is that TimerHandle did not clean up a strong reference from the event loop to `self`. This typically isn't a problem unless there's another strong reference to the loop from the callback or from its arguments (such as a Future). A few new unit tests should ensure this kind of bugs won't happen again. Fixes: #239. --- tests/test_base.py | 36 +++++++++++++++++++++++++++++- uvloop/cbhandles.pxd | 6 ++--- uvloop/cbhandles.pyx | 52 +++++++++++++++++++++++++++++++------------- 3 files changed, 75 insertions(+), 19 deletions(-) diff --git a/tests/test_base.py b/tests/test_base.py index a0b1ecfe..6fbb2e4f 100644 --- a/tests/test_base.py +++ b/tests/test_base.py @@ -36,7 +36,7 @@ def test_handle_weakref(self): h = self.loop.call_soon(lambda: None) wd['h'] = h # Would fail without __weakref__ slot. - def test_call_soon(self): + def test_call_soon_1(self): calls = [] def cb(inc): @@ -56,6 +56,22 @@ def cb(inc): self.assertEqual(calls, [10, 1]) + def test_call_soon_2(self): + waiter = self.loop.create_future() + waiter_r = weakref.ref(waiter) + self.loop.call_soon(lambda f: f.set_result(None), waiter) + self.loop.run_until_complete(waiter) + del waiter + self.assertIsNone(waiter_r()) + + def test_call_soon_3(self): + waiter = self.loop.create_future() + waiter_r = weakref.ref(waiter) + self.loop.call_soon(lambda f=waiter: f.set_result(None)) + self.loop.run_until_complete(waiter) + del waiter + self.assertIsNone(waiter_r()) + def test_call_soon_base_exc(self): def cb(): raise KeyboardInterrupt() @@ -160,6 +176,24 @@ async def main(): delta = time.monotonic() - started self.assertGreater(delta, 0.019) + def test_call_later_3(self): + # a memory leak regression test + waiter = self.loop.create_future() + waiter_r = weakref.ref(waiter) + self.loop.call_later(0.01, lambda f: f.set_result(None), waiter) + self.loop.run_until_complete(waiter) + del waiter + self.assertIsNone(waiter_r()) + + def test_call_later_4(self): + # a memory leak regression test + waiter = self.loop.create_future() + waiter_r = weakref.ref(waiter) + self.loop.call_later(0.01, lambda f=waiter: f.set_result(None)) + self.loop.run_until_complete(waiter) + del waiter + self.assertIsNone(waiter_r()) + def test_call_later_negative(self): calls = [] diff --git a/uvloop/cbhandles.pxd b/uvloop/cbhandles.pxd index 80978b98..ba65d9d0 100644 --- a/uvloop/cbhandles.pxd +++ b/uvloop/cbhandles.pxd @@ -24,15 +24,15 @@ cdef class Handle: cdef class TimerHandle: cdef: - object callback, args + object callback + tuple args bint _cancelled UVTimer timer Loop loop object context + tuple _debug_info object __weakref__ - readonly _source_traceback - cdef _run(self) cdef _cancel(self) cdef inline _clear(self) diff --git a/uvloop/cbhandles.pyx b/uvloop/cbhandles.pyx index ef06161a..dd37ce43 100644 --- a/uvloop/cbhandles.pyx +++ b/uvloop/cbhandles.pyx @@ -194,7 +194,12 @@ cdef class TimerHandle: self.context = None if loop._debug: - self._source_traceback = extract_stack() + self._debug_info = ( + format_callback_name(callback), + extract_stack() + ) + else: + self._debug_info = None self.timer = UVTimer.new( loop, self._run, self, delay) @@ -204,6 +209,11 @@ cdef class TimerHandle: # Only add to loop._timers when `self.timer` is successfully created loop._timers.add(self) + property _source_traceback: + def __get__(self): + if self._debug_info is not None: + return self._debug_info[1] + def __dealloc__(self): if UVLOOP_DEBUG: self.loop._debug_cb_timer_handles_count -= 1 @@ -227,7 +237,7 @@ cdef class TimerHandle: self.loop._timers.remove(self) finally: self.timer._close() - self.timer = None # let it die asap + self.timer = None # let the UVTimer handle GC cdef _run(self): if self._cancelled == 1: @@ -260,8 +270,8 @@ cdef class TimerHandle: 'handle': self, } - if self._source_traceback is not None: - context['source_traceback'] = self._source_traceback + if self._debug_info is not None: + context['source_traceback'] = self._debug_info[1] self.loop.call_exception_handler(context) else: @@ -276,6 +286,7 @@ cdef class TimerHandle: Py_DECREF(self) if PY37: Context_Exit(context) + self._clear() # Public API @@ -285,19 +296,20 @@ cdef class TimerHandle: if self._cancelled: info.append('cancelled') - if self.callback is not None: - func = self.callback - if hasattr(func, '__qualname__'): - cb_name = getattr(func, '__qualname__') - elif hasattr(func, '__name__'): - cb_name = getattr(func, '__name__') - else: - cb_name = repr(func) + if self._debug_info is not None: + callback_name = self._debug_info[0] + source_traceback = self._debug_info[1] + else: + callback_name = None + source_traceback = None - info.append(cb_name) + if callback_name is not None: + info.append(callback_name) + elif self.callback is not None: + info.append(format_callback_name(self.callback)) - if self._source_traceback is not None: - frame = self._source_traceback[-1] + if source_traceback is not None: + frame = source_traceback[-1] info.append('created at {}:{}'.format(frame[0], frame[1])) return '<' + ' '.join(info) + '>' @@ -309,6 +321,16 @@ cdef class TimerHandle: self._cancel() +cdef format_callback_name(func): + if hasattr(func, '__qualname__'): + cb_name = getattr(func, '__qualname__') + elif hasattr(func, '__name__'): + cb_name = getattr(func, '__name__') + else: + cb_name = repr(func) + return cb_name + + cdef new_Handle(Loop loop, object callback, object args, object context): cdef Handle handle handle = Handle.__new__(Handle)