Merge pull request tornadoweb#2348 from bdarnell/release-502

release 5.0.2

docs/releases.rst

@@ -4,6 +4,7 @@ Release notes
 .. toctree::
    :maxdepth: 2
 
+   releases/v5.0.2
    releases/v5.0.1
    releases/v5.0.0
    releases/v4.5.3

docs/releases/v5.0.2.rst

@@ -0,0 +1,18 @@
+What's new in Tornado 5.0.1
+===========================
+
+Apr 7, 2018
+-----------
+
+Bug fixes
+~~~~~~~~~
+
+- Fixed a memory leak when `.IOLoop` objects are created and destroyed.
+- If `.AsyncTestCase.get_new_ioloop` returns a reference to a
+  preexisting event loop (typically when it has been overridden to
+  return `.IOLoop.current()`), the test's ``tearDown`` method will not
+  close this loop.
+- Fixed a confusing error message when the synchronous `.HTTPClient`
+  fails to initialize because an event loop is already running.
+- `.PeriodicCallback` no longer executes twice in a row due to
+  backwards clock adjustments.

setup.py

@@ -103,7 +103,7 @@ def build_extension(self, ext):
 
 kwargs = {}
 
-version = "5.0.1"
+version = "5.0.2"
 
 with open('README.rst') as f:
     kwargs['long_description'] = f.read()

tornado/__init__.py

@@ -24,5 +24,5 @@
 # is zero for an official release, positive for a development branch,
 # or negative for a release candidate or beta (after the base version
 # number has been incremented)
-version = "5.0.1"
-version_info = (5, 0, 1, 0)
+version = "5.0.2"
+version_info = (5, 0, 2, 0)

tornado/httpclient.py

@@ -54,8 +54,10 @@
 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

tornado/ioloop.py

@@ -47,7 +47,6 @@
 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 @@ def connection_ready(sock, fd, events):
     _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):
@@ -1214,11 +1213,31 @@ def _run(self):
 
     def _schedule_next(self):
         if self._running:
-            current_time = self.io_loop.time()
-
-            if self._next_timeout <= current_time:
-                callback_time_sec = self.callback_time / 1000.0
-                self._next_timeout += (math.floor((current_time - self._next_timeout) /
-                                                  callback_time_sec) + 1) * callback_time_sec
-
+            self._update_next(self.io_loop.time())
             self._timeout = self.io_loop.add_timeout(self._next_timeout, self._run)
+
+    def _update_next(self, current_time):
+        callback_time_sec = self.callback_time / 1000.0
+        if self._next_timeout <= current_time:
+            # The period should be measured from the start of one call
+            # to the start of the next. If one call takes too long,
+            # skip cycles to get back to a multiple of the original
+            # schedule.
+            self._next_timeout += (math.floor((current_time - self._next_timeout) /
+                                              callback_time_sec) + 1) * callback_time_sec
+        else:
+            # If the clock moved backwards, ensure we advance the next
+            # timeout instead of recomputing the same value again.
+            # This may result in long gaps between callbacks if the
+            # clock jumps backwards by a lot, but the far more common
+            # scenario is a small NTP adjustment that should just be
+            # ignored.
+            #
+            # Note that on some systems if time.time() runs slower
+            # than time.monotonic() (most common on windows), we
+            # effectively experience a small backwards time jump on
+            # every iteration because PeriodicCallback uses
+            # time.time() while asyncio schedules callbacks using
+            # time.monotonic().
+            # https://github.com/tornadoweb/tornado/issues/2333
+            self._next_timeout += callback_time_sec

tornado/platform/asyncio.py

@@ -38,6 +38,20 @@ def initialize(self, asyncio_loop, **kwargs):
         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 @@ def close(self, all_fds=False):
             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)

tornado/test/asyncio_test.py

@@ -122,6 +122,44 @@ async def native_coroutine_with_adapter2():
             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):

tornado/test/ioloop_test.py

@@ -785,6 +785,62 @@ def cb():
         io_loop.close()
 
 
+class TestPeriodicCallbackMath(unittest.TestCase):
+    def simulate_calls(self, pc, durations):
+        """Simulate a series of calls to the PeriodicCallback.
+
+        Pass a list of call durations in seconds (negative values
+        work to simulate clock adjustments during the call, or more or
+        less equivalently, between calls). This method returns the
+        times at which each call would be made.
+        """
+        calls = []
+        now = 1000
+        pc._next_timeout = now
+        for d in durations:
+            pc._update_next(now)
+            calls.append(pc._next_timeout)
+            now = pc._next_timeout + d
+        return calls
+
+    def test_basic(self):
+        pc = PeriodicCallback(None, 10000)
+        self.assertEqual(self.simulate_calls(pc, [0] * 5),
+                         [1010, 1020, 1030, 1040, 1050])
+
+    def test_overrun(self):
+        # If a call runs for too long, we skip entire cycles to get
+        # back on schedule.
+        call_durations = [9, 9, 10, 11, 20, 20, 35, 35, 0, 0, 0]
+        expected = [
+            1010, 1020, 1030,  # first 3 calls on schedule
+            1050, 1070,  # next 2 delayed one cycle
+            1100, 1130,  # next 2 delayed 2 cycles
+            1170, 1210,  # next 2 delayed 3 cycles
+            1220, 1230,  # then back on schedule.
+        ]
+
+        pc = PeriodicCallback(None, 10000)
+        self.assertEqual(self.simulate_calls(pc, call_durations),
+                         expected)
+
+    def test_clock_backwards(self):
+        pc = PeriodicCallback(None, 10000)
+        # Backwards jumps are ignored, potentially resulting in a
+        # slightly slow schedule (although we assume that when
+        # time.time() and time.monotonic() are different, time.time()
+        # is getting adjusted by NTP and is therefore more accurate)
+        self.assertEqual(self.simulate_calls(pc, [-2, -1, -3, -2, 0]),
+                         [1010, 1020, 1030, 1040, 1050])
+
+        # For big jumps, we should perhaps alter the schedule, but we
+        # don't currently. This trace shows that we run callbacks
+        # every 10s of time.time(), but the first and second calls are
+        # 110s of real time apart because the backwards jump is
+        # ignored.
+        self.assertEqual(self.simulate_calls(pc, [-100, 0, 0]),
+                         [1010, 1020, 1030])
+
 class TestIOLoopConfiguration(unittest.TestCase):
     def run_python(self, *statements):
         statements = [

tornado/test/testing_test.py

@@ -11,6 +11,11 @@
 import traceback
 import warnings
 
+try:
+    import asyncio
+except ImportError:
+    asyncio = None
+
 
 @contextlib.contextmanager
 def set_environ(name, value):
@@ -310,5 +315,30 @@ async def test(self):
             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()

tornado/testing.py

@@ -74,6 +74,12 @@
         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.
 
@@ -216,11 +222,12 @@ def tearDown(self):
         # Clean up Subprocess, so it can be used again with a new ioloop.
         Subprocess.uninitialize()
         self.io_loop.clear_current()
-        # 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)
+        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)
         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.
@@ -229,9 +236,15 @@ def tearDown(self):
         self.__rethrow()
 
     def get_new_ioloop(self):
-        """Creates a new `.IOLoop` for this test.  May be overridden in
-        subclasses for tests that require a specific `.IOLoop` (usually
-        the singleton `.IOLoop.instance()`).
+        """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``).
         """
         return IOLoop()