Repository: tornadoweb/tornado
Pull request #2326 (2018-03-25 15:21:41)

asyncio: Fix a leak when event loops are created and destroyed


Patch:
diff --git a/tornado/httpclient.py b/tornado/httpclient.py
index 3547631d..9c438d15 100644
--- a/tornado/httpclient.py
+++ b/tornado/httpclient.py
@@ -54,8 +54,10 @@ from tornado.util import Configurable
 class HTTPClient(object):
     """A blocking HTTP client.
 
-    This interface is provided for convenience and testing; most applications
-    that are running an IOLoop will want to use `AsyncHTTPClient` instead.
+    This interface is provided to make it easier to share code between
+    synchronous and asynchronous applications. Applications that are
+    running an `.IOLoop` must use `AsyncHTTPClient` instead.
+
     Typical usage looks like this::
 
         http_client = httpclient.HTTPClient()
@@ -70,8 +72,19 @@ class HTTPClient(object):
             # Other errors are possible, such as IOError.
             print("Error: " + str(e))
         http_client.close()
+
+    .. versionchanged:: 5.0
+
+       Due to limitations in `asyncio`, it is no longer possible to
+       use the synchronous ``HTTPClient`` while an `.IOLoop` is running.
+       Use `AsyncHTTPClient` instead.
+
     """
     def __init__(self, async_client_class=None, **kwargs):
+        # Initialize self._closed at the beginning of the constructor
+        # so that an exception raised here doesn't lead to confusing
+        # failures in __del__.
+        self._closed = True
         self._io_loop = IOLoop(make_current=False)
         if async_client_class is None:
             async_client_class = AsyncHTTPClient
diff --git a/tornado/ioloop.py b/tornado/ioloop.py
index 839e7ee5..f6ec177b 100644
--- a/tornado/ioloop.py
+++ b/tornado/ioloop.py
@@ -47,7 +47,6 @@ import threading
 import time
 import traceback
 import math
-import weakref
 
 from tornado.concurrent import Future, is_future, chain_future, future_set_exc_info, future_add_done_callback  # noqa: E501
 from tornado.log import app_log, gen_log
@@ -185,7 +184,7 @@ class IOLoop(Configurable):
     _current = threading.local()
 
     # In Python 3, _ioloop_for_asyncio maps from asyncio loops to IOLoops.
-    _ioloop_for_asyncio = weakref.WeakKeyDictionary()
+    _ioloop_for_asyncio = dict()
 
     @classmethod
     def configure(cls, impl, **kwargs):
diff --git a/tornado/platform/asyncio.py b/tornado/platform/asyncio.py
index b2ad9fe6..b6a490af 100644
--- a/tornado/platform/asyncio.py
+++ b/tornado/platform/asyncio.py
@@ -38,6 +38,20 @@ class BaseAsyncIOLoop(IOLoop):
         self.readers = set()
         self.writers = set()
         self.closing = False
+        # If an asyncio loop was closed through an asyncio interface
+        # instead of IOLoop.close(), we'd never hear about it and may
+        # have left a dangling reference in our map. In case an
+        # application (or, more likely, a test suite) creates and
+        # destroys a lot of event loops in this way, check here to
+        # ensure that we don't have a lot of dead loops building up in
+        # the map.
+        #
+        # TODO(bdarnell): consider making self.asyncio_loop a weakref
+        # for AsyncIOMainLoop and make _ioloop_for_asyncio a
+        # WeakKeyDictionary.
+        for loop in list(IOLoop._ioloop_for_asyncio):
+            if loop.is_closed():
+                del IOLoop._ioloop_for_asyncio[loop]
         IOLoop._ioloop_for_asyncio[asyncio_loop] = self
         super(BaseAsyncIOLoop, self).initialize(**kwargs)
 
@@ -49,6 +63,7 @@ class BaseAsyncIOLoop(IOLoop):
             if all_fds:
                 self.close_fd(fileobj)
         self.asyncio_loop.close()
+        del IOLoop._ioloop_for_asyncio[self.asyncio_loop]
 
     def add_handler(self, fd, handler, events):
         fd, fileobj = self.split_fd(fd)


Test patch:
diff --git a/tornado/test/asyncio_test.py b/tornado/test/asyncio_test.py
index 37f2f6e1..a7c75649 100644
--- a/tornado/test/asyncio_test.py
+++ b/tornado/test/asyncio_test.py
@@ -122,6 +122,44 @@ class AsyncIOLoopTest(AsyncTestCase):
             42)
 
 
+@unittest.skipIf(asyncio is None, "asyncio module not present")
+class LeakTest(unittest.TestCase):
+    def setUp(self):
+        # Trigger a cleanup of the mapping so we start with a clean slate.
+        AsyncIOLoop().close()
+        # If we don't clean up after ourselves other tests may fail on
+        # py34.
+        self.orig_policy = asyncio.get_event_loop_policy()
+        asyncio.set_event_loop_policy(asyncio.DefaultEventLoopPolicy())
+
+    def tearDown(self):
+        asyncio.get_event_loop().close()
+        asyncio.set_event_loop_policy(self.orig_policy)
+
+    def test_ioloop_close_leak(self):
+        orig_count = len(IOLoop._ioloop_for_asyncio)
+        for i in range(10):
+            # Create and close an AsyncIOLoop using Tornado interfaces.
+            loop = AsyncIOLoop()
+            loop.close()
+        new_count = len(IOLoop._ioloop_for_asyncio) - orig_count
+        self.assertEqual(new_count, 0)
+
+    def test_asyncio_close_leak(self):
+        orig_count = len(IOLoop._ioloop_for_asyncio)
+        for i in range(10):
+            # Create and close an AsyncIOMainLoop using asyncio interfaces.
+            loop = asyncio.new_event_loop()
+            loop.call_soon(IOLoop.current)
+            loop.call_soon(loop.stop)
+            loop.run_forever()
+            loop.close()
+        new_count = len(IOLoop._ioloop_for_asyncio) - orig_count
+        # Because the cleanup is run on new loop creation, we have one
+        # dangling entry in the map (but only one).
+        self.assertEqual(new_count, 1)
+
+
 @unittest.skipIf(asyncio is None, "asyncio module not present")
 class AnyThreadEventLoopPolicyTest(unittest.TestCase):
     def setUp(self):
diff --git a/tornado/test/testing_test.py b/tornado/test/testing_test.py
index 37449466..9905c927 100644
--- a/tornado/test/testing_test.py
+++ b/tornado/test/testing_test.py
@@ -11,11 +11,6 @@ import platform
 import traceback
 import warnings
 
-try:
-    import asyncio
-except ImportError:
-    asyncio = None
-
 
 @contextlib.contextmanager
 def set_environ(name, value):
@@ -312,30 +307,5 @@ class GenTest(AsyncTestCase):
             self.finished = True
 
 
-@unittest.skipIf(asyncio is None, "asyncio module not present")
-class GetNewIOLoopTest(AsyncTestCase):
-    def get_new_ioloop(self):
-        # Use the current loop instead of creating a new one here.
-        return ioloop.IOLoop.current()
-
-    def setUp(self):
-        # This simulates the effect of an asyncio test harness like
-        # pytest-asyncio.
-        self.orig_loop = asyncio.get_event_loop()
-        self.new_loop = asyncio.new_event_loop()
-        asyncio.set_event_loop(self.new_loop)
-        super(GetNewIOLoopTest, self).setUp()
-
-    def tearDown(self):
-        super(GetNewIOLoopTest, self).tearDown()
-        # AsyncTestCase must not affect the existing asyncio loop.
-        self.assertFalse(asyncio.get_event_loop().is_closed())
-        asyncio.set_event_loop(self.orig_loop)
-        self.new_loop.close()
-
-    def test_loop(self):
-        self.assertIs(self.io_loop.asyncio_loop, self.new_loop)
-
-
 if __name__ == '__main__':
     unittest.main()
diff --git a/tornado/testing.py b/tornado/testing.py
index 19bc5a94..400dd3e4 100644
--- a/tornado/testing.py
+++ b/tornado/testing.py
@@ -74,12 +74,6 @@ else:
         import unittest  # type: ignore
 
 
-if asyncio is None:
-    _NON_OWNED_IOLOOPS = ()
-else:
-    import tornado.platform.asyncio
-    _NON_OWNED_IOLOOPS = tornado.platform.asyncio.AsyncIOMainLoop
-
 def bind_unused_port(reuse_port=False):
     """Binds a server socket to an available port on localhost.
 
@@ -222,12 +216,11 @@ class AsyncTestCase(unittest.TestCase):
         # Clean up Subprocess, so it can be used again with a new ioloop.
         Subprocess.uninitialize()
         self.io_loop.clear_current()
-        if not isinstance(self.io_loop, _NON_OWNED_IOLOOPS):
-            # Try to clean up any file descriptors left open in the ioloop.
-            # This avoids leaks, especially when tests are run repeatedly
-            # in the same process with autoreload (because curl does not
-            # set FD_CLOEXEC on its file descriptors)
-            self.io_loop.close(all_fds=True)
+        # Try to clean up any file descriptors left open in the ioloop.
+        # This avoids leaks, especially when tests are run repeatedly
+        # in the same process with autoreload (because curl does not
+        # set FD_CLOEXEC on its file descriptors)
+        self.io_loop.close(all_fds=True)
         super(AsyncTestCase, self).tearDown()
         # In case an exception escaped or the StackContext caught an exception
         # when there wasn't a wait() to re-raise it, do so here.
@@ -236,15 +229,9 @@ class AsyncTestCase(unittest.TestCase):
         self.__rethrow()
 
     def get_new_ioloop(self):
-        """Returns the `.IOLoop` to use for this test.
-
-        By default, a new `.IOLoop` is created for each test.
-        Subclasses may override this method to return
-        `.IOLoop.current()` if it is not appropriate to use a new
-        `.IOLoop` in each tests (for example, if there are global
-        singletons using the default `.IOLoop`) or if a per-test event
-        loop is being provided by another system (such as
-        ``pytest-asyncio``).
+        """Creates a new `.IOLoop` for this test.  May be overridden in
+        subclasses for tests that require a specific `.IOLoop` (usually
+        the singleton `.IOLoop.instance()`).
         """
         return IOLoop()