Merge pull request tornadoweb#2361 from bdarnell/ioloop-deprecation

iostream: Deprecate callback-based interfaces

Author: bdarnell

tornado/ioloop.py

@@ -441,7 +441,8 @@ def set_blocking_signal_threshold(self, seconds, action):
         .. deprecated:: 5.0
 
            Not implemented on the `asyncio` event loop. Use the environment
-           variable ``PYTHONASYNCIODEBUG=1`` instead.
+           variable ``PYTHONASYNCIODEBUG=1`` instead. This method will be
+           removed in Tornado 6.0.
         """
         raise NotImplementedError()
 
@@ -455,14 +456,19 @@ def set_blocking_log_threshold(self, seconds):
         .. deprecated:: 5.0
 
            Not implemented on the `asyncio` event loop. Use the environment
-           variable ``PYTHONASYNCIODEBUG=1`` instead.
+           variable ``PYTHONASYNCIODEBUG=1`` instead. This method will be
+           removed in Tornado 6.0.
         """
         self.set_blocking_signal_threshold(seconds, self.log_stack)
 
     def log_stack(self, signal, frame):
         """Signal handler to log the stack trace of the current thread.
 
         For use with `set_blocking_signal_threshold`.
+
+        .. deprecated:: 5.1
+
+           This method will be removed in Tornado 6.0.
         """
         gen_log.warning('IOLoop blocked for %f seconds in\n%s',
                         self._blocking_signal_threshold,
@@ -789,6 +795,16 @@ def handle_callback_exception(self, callback):
 
         The exception itself is not passed explicitly, but is available
         in `sys.exc_info`.
+
+        .. versionchanged:: 5.0
+
+           When the `asyncio` event loop is used (which is now the
+           default on Python 3), some callback errors will be handled by
+           `asyncio` instead of this method.
+
+        .. deprecated: 5.1
+
+           Support for this method will be removed in Tornado 6.0.
         """
         app_log.error("Exception in callback %r", callback, exc_info=True)
 

tornado/iostream.py

@@ -33,6 +33,7 @@
 import socket
 import sys
 import re
+import warnings
 
 from tornado.concurrent import Future
 from tornado import ioloop
@@ -342,6 +343,12 @@ def read_until_regex(self, regex, callback=None, max_bytes=None):
         .. versionchanged:: 4.0
             Added the ``max_bytes`` argument.  The ``callback`` argument is
             now optional and a `.Future` will be returned if it is omitted.
+
+        .. deprecated:: 5.1
+
+           The ``callback`` argument is deprecated and will be removed
+           in Tornado 6.0. Use the returned `.Future` instead.
+
         """
         future = self._set_read_callback(callback)
         self._read_regex = re.compile(regex)
@@ -375,6 +382,11 @@ def read_until(self, delimiter, callback=None, max_bytes=None):
         .. versionchanged:: 4.0
             Added the ``max_bytes`` argument.  The ``callback`` argument is
             now optional and a `.Future` will be returned if it is omitted.
+
+        .. deprecated:: 5.1
+
+           The ``callback`` argument is deprecated and will be removed
+           in Tornado 6.0. Use the returned `.Future` instead.
         """
         future = self._set_read_callback(callback)
         self._read_delimiter = delimiter
@@ -408,12 +420,23 @@ def read_bytes(self, num_bytes, callback=None, streaming_callback=None,
         .. versionchanged:: 4.0
             Added the ``partial`` argument.  The callback argument is now
             optional and a `.Future` will be returned if it is omitted.
+
+        .. deprecated:: 5.1
+
+           The ``callback`` and ``streaming_callback`` arguments are
+           deprecated and will be removed in Tornado 6.0. Use the
+           returned `.Future` (and ``partial=True`` for
+           ``streaming_callback``) instead.
+
         """
         future = self._set_read_callback(callback)
         assert isinstance(num_bytes, numbers.Integral)
         self._read_bytes = num_bytes
         self._read_partial = partial
-        self._streaming_callback = stack_context.wrap(streaming_callback)
+        if streaming_callback is not None:
+            warnings.warn("streaming_callback is deprecated, use partial instead",
+                          DeprecationWarning)
+            self._streaming_callback = stack_context.wrap(streaming_callback)
         try:
             self._try_inline_read()
         except:
@@ -434,6 +457,12 @@ def read_into(self, buf, callback=None, partial=False):
         entirely filled with read data.
 
         .. versionadded:: 5.0
+
+        .. deprecated:: 5.1
+
+           The ``callback`` argument is deprecated and will be removed
+           in Tornado 6.0. Use the returned `.Future` instead.
+
         """
         future = self._set_read_callback(callback)
 
@@ -485,9 +514,19 @@ def read_until_close(self, callback=None, streaming_callback=None):
             The callback argument is now optional and a `.Future` will
             be returned if it is omitted.
 
+        .. deprecated:: 5.1
+
+           The ``callback`` and ``streaming_callback`` arguments are
+           deprecated and will be removed in Tornado 6.0. Use the
+           returned `.Future` (and `read_bytes` with ``partial=True``
+           for ``streaming_callback``) instead.
+
         """
         future = self._set_read_callback(callback)
-        self._streaming_callback = stack_context.wrap(streaming_callback)
+        if streaming_callback is not None:
+            warnings.warn("streaming_callback is deprecated, use read_bytes(partial=True) instead",
+                          DeprecationWarning)
+            self._streaming_callback = stack_context.wrap(streaming_callback)
         if self.closed():
             if self._streaming_callback is not None:
                 self._run_read_callback(self._read_buffer_size, True)
@@ -521,6 +560,12 @@ def write(self, data, callback=None):
 
         .. versionchanged:: 4.5
             Added support for `memoryview` arguments.
+
+        .. deprecated:: 5.1
+
+           The ``callback`` argument is deprecated and will be removed
+           in Tornado 6.0. Use the returned `.Future` instead.
+
         """
         self._check_closed()
         if data:
@@ -530,6 +575,8 @@ def write(self, data, callback=None):
             self._write_buffer.append(data)
             self._total_write_index += len(data)
         if callback is not None:
+            warnings.warn("callback argument is deprecated, use returned Future instead",
+                          DeprecationWarning)
             self._write_callback = stack_context.wrap(callback)
             future = None
         else:
@@ -546,9 +593,14 @@ def write(self, data, callback=None):
     def set_close_callback(self, callback):
         """Call the given callback when the stream is closed.
 
-        This is not necessary for applications that use the `.Future`
-        interface; all outstanding ``Futures`` will resolve with a
-        `StreamClosedError` when the stream is closed.
+        This mostly is not necessary for applications that use the
+        `.Future` interface; all outstanding ``Futures`` will resolve
+        with a `StreamClosedError` when the stream is closed. However,
+        it is still useful as a way to signal that the stream has been
+        closed while no other read or write is in progress.
+
+        Unlike other callback-based interfaces, ``set_close_callback``
+        will not be removed in Tornado 6.0.
         """
         self._close_callback = stack_context.wrap(callback)
         self._maybe_add_error_listener()
@@ -807,6 +859,8 @@ def _set_read_callback(self, callback):
         assert self._read_callback is None, "Already reading"
         assert self._read_future is None, "Already reading"
         if callback is not None:
+            warnings.warn("callbacks are deprecated, use returned Future instead",
+                          DeprecationWarning)
             self._read_callback = stack_context.wrap(callback)
         else:
             self._read_future = Future()
@@ -1237,9 +1291,17 @@ class is recommended instead of calling this method directly.
            ``ssl_options=dict(cert_reqs=ssl.CERT_NONE)`` or a
            suitably-configured `ssl.SSLContext` to the
            `SSLIOStream` constructor to disable.
+
+        .. deprecated:: 5.1
+
+           The ``callback`` argument is deprecated and will be removed
+           in Tornado 6.0. Use the returned `.Future` instead.
+
         """
         self._connecting = True
         if callback is not None:
+            warnings.warn("callback argument is deprecated, use returned Future instead",
+                          DeprecationWarning)
             self._connect_callback = stack_context.wrap(callback)
             future = None
         else:
@@ -1523,9 +1585,13 @@ def _handle_write(self):
 
     def connect(self, address, callback=None, server_hostname=None):
         self._server_hostname = server_hostname
-        # Pass a dummy callback to super.connect(), which is slightly
-        # more efficient than letting it return a Future we ignore.
-        super(SSLIOStream, self).connect(address, callback=lambda: None)
+        # Ignore the result of connect(). If it fails,
+        # wait_for_handshake will raise an error too. This is
+        # necessary for the old semantics of the connect callback
+        # (which takes no arguments). In 6.0 this can be refactored to
+        # be a regular coroutine.
+        fut = super(SSLIOStream, self).connect(address)
+        fut.add_done_callback(lambda f: f.exception())
         return self.wait_for_handshake(callback)
 
     def _handle_connect(self):
@@ -1569,11 +1635,19 @@ def wait_for_handshake(self, callback=None):
         handshake to complete). It may only be called once per stream.
 
         .. versionadded:: 4.2
+
+        .. deprecated:: 5.1
+
+           The ``callback`` argument is deprecated and will be removed
+           in Tornado 6.0. Use the returned `.Future` instead.
+
         """
         if (self._ssl_connect_callback is not None or
                 self._ssl_connect_future is not None):
             raise RuntimeError("Already waiting")
         if callback is not None:
+            warnings.warn("callback argument is deprecated, use returned Future instead",
+                          DeprecationWarning)
             self._ssl_connect_callback = stack_context.wrap(callback)
             future = None
         else:

tornado/test/concurrent_test.py

@@ -20,6 +20,7 @@
 import socket
 import sys
 import traceback
+import warnings
 
 from tornado.concurrent import (Future, return_future, ReturnValueIgnoredError,
                                 run_on_executor, future_set_result_unless_cancelled)
@@ -249,20 +250,16 @@ def f():
 
 
 class CapServer(TCPServer):
+    @gen.coroutine
     def handle_stream(self, stream, address):
-        logging.debug("handle_stream")
-        self.stream = stream
-        self.stream.read_until(b"\n", self.handle_read)
-
-    def handle_read(self, data):
-        logging.debug("handle_read")
+        data = yield stream.read_until(b"\n")
         data = to_unicode(data)
         if data == data.upper():
-            self.stream.write(b"error\talready capitalized\n")
+            stream.write(b"error\talready capitalized\n")
         else:
             # data already has \n
-            self.stream.write(utf8("ok\t%s" % data.upper()))
-        self.stream.close()
+            stream.write(utf8("ok\t%s" % data.upper()))
+        stream.close()
 
 
 class CapError(Exception):
@@ -397,10 +394,30 @@ def f():
 class ManualClientTest(ClientTestMixin, AsyncTestCase):
     client_class = ManualCapClient
 
+    def setUp(self):
+        self.warning_catcher = warnings.catch_warnings()
+        self.warning_catcher.__enter__()
+        warnings.simplefilter('ignore', DeprecationWarning)
+        super(ManualClientTest, self).setUp()
+
+    def tearDown(self):
+        super(ManualClientTest, self).tearDown()
+        self.warning_catcher.__exit__(None, None, None)
+
 
 class DecoratorClientTest(ClientTestMixin, AsyncTestCase):
     client_class = DecoratorCapClient
 
+    def setUp(self):
+        self.warning_catcher = warnings.catch_warnings()
+        self.warning_catcher.__enter__()
+        warnings.simplefilter('ignore', DeprecationWarning)
+        super(DecoratorClientTest, self).setUp()
+
+    def tearDown(self):
+        super(DecoratorClientTest, self).tearDown()
+        self.warning_catcher.__exit__(None, None, None)
+
 
 class GeneratorClientTest(ClientTestMixin, AsyncTestCase):
     client_class = GeneratorCapClient

tornado/test/httpclient_test.py

@@ -4,7 +4,6 @@
 import binascii
 from contextlib import closing
 import copy
-import functools
 import sys
 import threading
 import datetime
@@ -190,10 +189,15 @@ def test_chunked_close(self):
         # over several ioloop iterations, but the connection is already closed.
         sock, port = bind_unused_port()
         with closing(sock):
-            def write_response(stream, request_data):
+            @gen.coroutine
+            def accept_callback(conn, address):
+                # fake an HTTP server using chunked encoding where the final chunks
+                # and connection close all happen at once
+                stream = IOStream(conn)
+                request_data = yield stream.read_until(b"\r\n\r\n")
                 if b"HTTP/1." not in request_data:
                     self.skipTest("requires HTTP/1.x")
-                stream.write(b"""\
+                yield stream.write(b"""\
 HTTP/1.1 200 OK
 Transfer-Encoding: chunked
 
@@ -203,14 +207,8 @@ def write_response(stream, request_data):
 2
 0
 
-""".replace(b"\n", b"\r\n"), callback=stream.close)
-
-            def accept_callback(conn, address):
-                # fake an HTTP server using chunked encoding where the final chunks
-                # and connection close all happen at once
-                stream = IOStream(conn)
-                stream.read_until(b"\r\n\r\n",
-                                  functools.partial(write_response, stream))
+""".replace(b"\n", b"\r\n"))
+                stream.close()
             netutil.add_accept_handler(sock, accept_callback)
             resp = self.fetch("http://127.0.0.1:%d/" % port)
             resp.rethrow()
@@ -383,20 +381,20 @@ def test_multi_line_headers(self):
         # http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2
         sock, port = bind_unused_port()
         with closing(sock):
-            def write_response(stream, request_data):
+            @gen.coroutine
+            def accept_callback(conn, address):
+                stream = IOStream(conn)
+                request_data = yield stream.read_until(b"\r\n\r\n")
                 if b"HTTP/1." not in request_data:
                     self.skipTest("requires HTTP/1.x")
-                stream.write(b"""\
+                yield stream.write(b"""\
 HTTP/1.1 200 OK
 X-XSS-Protection: 1;
 \tmode=block
 
-""".replace(b"\n", b"\r\n"), callback=stream.close)
+""".replace(b"\n", b"\r\n"))
+                stream.close()
 
-            def accept_callback(conn, address):
-                stream = IOStream(conn)
-                stream.read_until(b"\r\n\r\n",
-                                  functools.partial(write_response, stream))
             netutil.add_accept_handler(sock, accept_callback)
             resp = self.fetch("http://127.0.0.1:%d/" % port)
             resp.rethrow()