Browse Source

pythonTask: fix refcount leak of non-panda Future::done

A missing Py_DECREF on the future's "done" method
caused both the bound method and the underlying
self instance of the future to be leaked when
awaiting non-panda futures (such as _asyncio.Future).

This change includes a simple new test addition
to catch this in the future.
John C. Allwein 1 year ago
parent
commit
21b39da65d
2 changed files with 28 additions and 3 deletions
  1. 1 0
      panda/src/event/pythonTask.cxx
  2. 27 3
      tests/event/test_futures.py

+ 1 - 0
panda/src/event/pythonTask.cxx

@@ -795,6 +795,7 @@ do_python_task() {
             << *this << " is now polling " << PyUnicode_AsUTF8(str) << ".done()\n";
           Py_DECREF(str);
         }
+        Py_DECREF(fut_done);
         _fut_waiter = result;
         return DS_cont;
       }

+ 27 - 3
tests/event/test_futures.py

@@ -1,9 +1,9 @@
 from panda3d import core
+from asyncio.exceptions import TimeoutError, CancelledError
 import pytest
 import time
 import sys
-from asyncio.exceptions import TimeoutError, CancelledError
-
+import weakref
 
 class MockFuture:
     _asyncio_future_blocking = False
@@ -31,7 +31,6 @@ class MockFuture:
 
         return self._result
 
-
 def test_future_cancelled():
     fut = core.AsyncFuture()
 
@@ -681,3 +680,28 @@ def test_event_future_cancel2():
     assert not fut2.done()
     assert not fut2.cancelled()
 
+def test_task_manager_cleanup_non_panda_future():
+    future = MockFuture()
+    # Create a weakref so we can verify the future was cleaned up.
+    future_ref = weakref.ref(future)
+
+    async def coro_main():
+        return await future
+
+    task = core.PythonTask(coro_main(), 'coro_main')
+    task_mgr = core.AsyncTaskManager.get_global_ptr()
+    task_mgr.add(task)
+    # Poll the task_mgr so the PythonTask starts polling on future.done()
+    task_mgr.poll()
+    future._result = 'Done123'
+    future._state = 'DONE'
+    # Drop our reference to the future, `task` should hold the only reference
+    del future
+    # Recognize the future has completed
+    task_mgr.poll()
+
+    # Verify the task was completed.
+    assert task.result() == 'Done123'
+    del coro_main # this should break the last strong reference to the mock future
+
+    assert future_ref() is None, "MockFuture was not cleaned up!"