ioloop,concurrent: Fix reference cycles

In a few places we were referring to a future via a closure instead
of using the reference passed as an argument to the callback.  This
sometimes causes a reference cycle that can slow GC. This commit
adds a test which covers two of the cases (chain_future and the
concurrent.future branch of add_future) while the third was found by
inspecting other calls to add_done_callback for obvious instances of
this pattern.

Fixes #2620

diff --git a/tornado/concurrent.py b/tornado/concurrent.py
index 6e05346b56d00048910752201dc110edd7df9aae..f0bbf6231763bb8beb957ee6c3d82e9c4ce88be2 100644 (file)
--- a/tornado/concurrent.py
+++ b/tornado/concurrent.py
@@ -150,8 +150,7 @@ def chain_future(a: "Future[_T]", b: "Future[_T]") -> None:
 
     """
 
-    def copy(future: "Future[_T]") -> None:
-        assert future is a
+    def copy(a: "Future[_T]") -> None:
         if b.done():
             return
         if hasattr(a, "exc_info") and a.exc_info() is not None:  # type: ignore

diff --git a/tornado/ioloop.py b/tornado/ioloop.py
index cdfb4a630fd063f61b2818ea5d5f9dd1b38636fd..3fb1359aae174162e6d4aa8b12e877c0582d9e0e 100644 (file)
--- a/tornado/ioloop.py
+++ b/tornado/ioloop.py
@@ -696,15 +696,13 @@ class IOLoop(Configurable):
             # the error logging (i.e. it goes to tornado.log.app_log
             # instead of asyncio's log).
             future.add_done_callback(
-                lambda f: self._run_callback(functools.partial(callback, future))
+                lambda f: self._run_callback(functools.partial(callback, f))
             )
         else:
             assert is_future(future)
             # For concurrent futures, we use self.add_callback, so
             # it's fine if future_add_done_callback inlines that call.
-            future_add_done_callback(
-                future, lambda f: self.add_callback(callback, future)
-            )
+            future_add_done_callback(future, lambda f: self.add_callback(callback, f))
 
     def run_in_executor(
         self,

diff --git a/tornado/test/circlerefs_test.py b/tornado/test/circlerefs_test.py
index 2502c5fbaccd7720633e47a3e44d1faa40ca9649..3908588c596032c249143bc918bf9462a6a24011 100644 (file)
--- a/tornado/test/circlerefs_test.py
+++ b/tornado/test/circlerefs_test.py
@@ -181,3 +181,29 @@ class CircleRefsTest(unittest.TestCase):
                 self.write("ok\n")
 
         asyncio.run(self.run_handler(Handler))
+
+    def test_run_on_executor(self):
+        # From https://github.com/tornadoweb/tornado/issues/2620
+        #
+        # When this test was introduced it found cycles in IOLoop.add_future
+        # and tornado.concurrent.chain_future.
+        import concurrent.futures
+
+        thread_pool = concurrent.futures.ThreadPoolExecutor(1)
+
+        class Factory(object):
+            executor = thread_pool
+
+            @tornado.concurrent.run_on_executor
+            def run(self):
+                return None
+
+        factory = Factory()
+
+        async def main():
+            # The cycle is not reported on the first call. It's not clear why.
+            for i in range(2):
+                await factory.run()
+
+        with assert_no_cycle_garbage():
+            asyncio.run(main())