From: Ben Darnell <ben@bendarnell.com>
Date: Tue, 22 Aug 2023 03:03:39 +0000 (-0400)
Subject: ioloop,concurrent: Fix reference cycles
X-Git-Tag: v6.4.0b1~12^2~2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=1d5fc98018bd1d742b1608634bfb57f6e3d990f0;p=thirdparty%2Ftornado.git

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 6e05346b5..f0bbf6231 100644
--- 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 cdfb4a630..3fb1359aa 100644
--- 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 2502c5fba..3908588c5 100644
--- 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())