Repository: tornadoweb/tornado

Issue:
RedirectHandler re substitution doesn't work
### Code

``` python
import tornado.web
from tornado.web import url
import tornado.ioloop

app = tornado.web.Application([
    url(r"/pictures/(.*)", tornado.web.RedirectHandler,
        dict(url=r"/photos/\1")),
])

app.listen(8765)
tornado.ioloop.IOLoop.current().start()
```
### visit `localhost:8765/picture/1` will raise TypeError: get() takes 1 positional argument but 2 were given

```
ERROR:tornado.application:Uncaught exception GET /pictures/1 (::1)
HTTPServerRequest(protocol='http', host='localhost:8765', method='GET', uri='/pictures/1', version='HTTP/1.1', remote_ip='::1', headers={'Host': 'localhost:8765', 'Accept': 'text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8', 'Dnt': '1', 'Upgrade-Insecure-Requests': '1', 'Connection': 'keep-alive', 'User-Agent': 'Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.59 Safari/537.36', 'Cookie': 'Pycharm-73b7632f=f7202b97-d898-4046-8aae-ae6536d1d931; _xsrf=2|ca7ba18c|041b228c03a99e0221d548c50c478a96|1476528491', 'Accept-Language': 'zh-CN,zh;q=0.8', 'Accept-Encoding': 'gzip, deflate, sdch, br'})
Traceback (most recent call last):
  File "C:\Users\Jeff\AppData\Local\Programs\Python\Python35-32\lib\site-packages\tornado\web.py", line 1467, in _execute
    result = method(*self.path_args, **self.path_kwargs)
TypeError: get() takes 1 positional argument but 2 were given
ERROR:tornado.access:500 GET /pictures/1 (::1) 1532.03ms
```


Discussion:
ajdavis: Ben, do you suppose RedirectHandler should also convert named capture groups to keyword args like RequestHandler does?

Patch:
diff --git a/docs/guide/structure.rst b/docs/guide/structure.rst
index f0829df0..715e080f 100644
--- a/docs/guide/structure.rst
+++ b/docs/guide/structure.rst
@@ -278,7 +278,7 @@ to the prefix ``/photos/`` instead::
     app = tornado.web.Application([
         url(r"/photos/(.*)", MyPhotoHandler),
         url(r"/pictures/(.*)", tornado.web.RedirectHandler,
-            dict(url=r"/photos/\1")),
+            dict(url=r"/photos/{0}")),
         ])
 
 Unlike `.RequestHandler.redirect`, `.RedirectHandler` uses permanent
diff --git a/tornado/httpserver.py b/tornado/httpserver.py
index c7b9c2f8..ff235fe4 100644
--- a/tornado/httpserver.py
+++ b/tornado/httpserver.py
@@ -179,45 +179,12 @@ class HTTPServer(TCPServer, Configurable,
         conn.start_serving(self)
 
     def start_request(self, server_conn, request_conn):
-        if isinstance(self.request_callback, httputil.HTTPServerConnectionDelegate):
-            delegate = self.request_callback.start_request(server_conn, request_conn)
-        else:
-            delegate = _CallableAdapter(self.request_callback, request_conn)
-
-        if self.xheaders:
-            delegate = _ProxyAdapter(delegate, request_conn)
-
-        return delegate
+        return _ServerRequestAdapter(self, server_conn, request_conn)
 
     def on_close(self, server_conn):
         self._connections.remove(server_conn)
 
 
-class _CallableAdapter(httputil.HTTPMessageDelegate):
-    def __init__(self, request_callback, request_conn):
-        self.connection = request_conn
-        self.request_callback = request_callback
-        self.request = None
-        self.delegate = None
-        self._chunks = []
-
-    def headers_received(self, start_line, headers):
-        self.request = httputil.HTTPServerRequest(
-            connection=self.connection, start_line=start_line,
-            headers=headers)
-
-    def data_received(self, chunk):
-        self._chunks.append(chunk)
-
-    def finish(self):
-        self.request.body = b''.join(self._chunks)
-        self.request._parse_body()
-        self.request_callback(self.request)
-
-    def on_connection_close(self):
-        self._chunks = None
-
-
 class _HTTPRequestContext(object):
     def __init__(self, stream, address, protocol):
         self.address = address
@@ -280,27 +247,58 @@ class _HTTPRequestContext(object):
         self.protocol = self._orig_protocol
 
 
-class _ProxyAdapter(httputil.HTTPMessageDelegate):
-    def __init__(self, delegate, request_conn):
+class _ServerRequestAdapter(httputil.HTTPMessageDelegate):
+    """Adapts the `HTTPMessageDelegate` interface to the interface expected
+    by our clients.
+    """
+    def __init__(self, server, server_conn, request_conn):
+        self.server = server
         self.connection = request_conn
-        self.delegate = delegate
+        self.request = None
+        if isinstance(server.request_callback,
+                      httputil.HTTPServerConnectionDelegate):
+            self.delegate = server.request_callback.start_request(
+                server_conn, request_conn)
+            self._chunks = None
+        else:
+            self.delegate = None
+            self._chunks = []
 
     def headers_received(self, start_line, headers):
-        self.connection.context._apply_xheaders(headers)
-        return self.delegate.headers_received(start_line, headers)
+        if self.server.xheaders:
+            self.connection.context._apply_xheaders(headers)
+        if self.delegate is None:
+            self.request = httputil.HTTPServerRequest(
+                connection=self.connection, start_line=start_line,
+                headers=headers)
+        else:
+            return self.delegate.headers_received(start_line, headers)
 
     def data_received(self, chunk):
-        return self.delegate.data_received(chunk)
+        if self.delegate is None:
+            self._chunks.append(chunk)
+        else:
+            return self.delegate.data_received(chunk)
 
     def finish(self):
-        self.delegate.finish()
+        if self.delegate is None:
+            self.request.body = b''.join(self._chunks)
+            self.request._parse_body()
+            self.server.request_callback(self.request)
+        else:
+            self.delegate.finish()
         self._cleanup()
 
     def on_connection_close(self):
-        self.delegate.on_connection_close()
+        if self.delegate is None:
+            self._chunks = None
+        else:
+            self.delegate.on_connection_close()
         self._cleanup()
 
     def _cleanup(self):
-        self.connection.context._unapply_xheaders()
+        if self.server.xheaders:
+            self.connection.context._unapply_xheaders()
+
 
 HTTPRequest = httputil.HTTPServerRequest
diff --git a/tornado/ioloop.py b/tornado/ioloop.py
index 1b1a07cd..d6183176 100644
--- a/tornado/ioloop.py
+++ b/tornado/ioloop.py
@@ -28,7 +28,6 @@ In addition to I/O events, the `IOLoop` can also schedule time-based events.
 
 from __future__ import absolute_import, division, print_function, with_statement
 
-import collections
 import datetime
 import errno
 import functools
@@ -694,7 +693,8 @@ class PollIOLoop(IOLoop):
         self.time_func = time_func or time.time
         self._handlers = {}
         self._events = {}
-        self._callbacks = collections.deque()
+        self._callbacks = []
+        self._callback_lock = threading.Lock()
         self._timeouts = []
         self._cancellations = 0
         self._running = False
@@ -712,7 +712,8 @@ class PollIOLoop(IOLoop):
                          self.READ)
 
     def close(self, all_fds=False):
-        self._closing = True
+        with self._callback_lock:
+            self._closing = True
         self.remove_handler(self._waker.fileno())
         if all_fds:
             for fd, handler in self._handlers.values():
@@ -799,7 +800,9 @@ class PollIOLoop(IOLoop):
             while True:
                 # Prevent IO event starvation by delaying new callbacks
                 # to the next iteration of the event loop.
-                ncallbacks = len(self._callbacks)
+                with self._callback_lock:
+                    callbacks = self._callbacks
+                    self._callbacks = []
 
                 # Add any timeouts that have come due to the callback list.
                 # Do not run anything until we have determined which ones
@@ -828,14 +831,14 @@ class PollIOLoop(IOLoop):
                                           if x.callback is not None]
                         heapq.heapify(self._timeouts)
 
-                for i in range(ncallbacks):
-                    self._run_callback(self._callbacks.popleft())
+                for callback in callbacks:
+                    self._run_callback(callback)
                 for timeout in due_timeouts:
                     if timeout.callback is not None:
                         self._run_callback(timeout.callback)
                 # Closures may be holding on to a lot of memory, so allow
                 # them to be freed before we go into our poll wait.
-                due_timeouts = timeout = None
+                callbacks = callback = due_timeouts = timeout = None
 
                 if self._callbacks:
                     # If any callbacks or timeouts called add_callback,
@@ -931,20 +934,36 @@ class PollIOLoop(IOLoop):
         self._cancellations += 1
 
     def add_callback(self, callback, *args, **kwargs):
-        if self._closing:
-            return
-        # Blindly insert into self._callbacks. This is safe even
-        # from signal handlers because deque.append is atomic.
-        self._callbacks.append(functools.partial(
-            stack_context.wrap(callback), *args, **kwargs))
         if thread.get_ident() != self._thread_ident:
-            # This will write one byte but Waker.consume() reads many
-            # at once, so it's ok to write even when not strictly
-            # necessary.
-            self._waker.wake()
+            # If we're not on the IOLoop's thread, we need to synchronize
+            # with other threads, or waking logic will induce a race.
+            with self._callback_lock:
+                if self._closing:
+                    return
+                list_empty = not self._callbacks
+                self._callbacks.append(functools.partial(
+                    stack_context.wrap(callback), *args, **kwargs))
+                if list_empty:
+                    # If we're not in the IOLoop's thread, and we added the
+                    # first callback to an empty list, we may need to wake it
+                    # up (it may wake up on its own, but an occasional extra
+                    # wake is harmless).  Waking up a polling IOLoop is
+                    # relatively expensive, so we try to avoid it when we can.
+                    self._waker.wake()
         else:
-            # If we're on the IOLoop's thread, we don't need to wake anyone.
-            pass
+            if self._closing:
+                return
+            # If we're on the IOLoop's thread, we don't need the lock,
+            # since we don't need to wake anyone, just add the
+            # callback. Blindly insert into self._callbacks. This is
+            # safe even from signal handlers because the GIL makes
+            # list.append atomic. One subtlety is that if the signal
+            # is interrupting another thread holding the
+            # _callback_lock block in IOLoop.start, we may modify
+            # either the old or new version of self._callbacks, but
+            # either way will work.
+            self._callbacks.append(functools.partial(
+                stack_context.wrap(callback), *args, **kwargs))
 
     def add_callback_from_signal(self, callback, *args, **kwargs):
         with stack_context.NullContext():
diff --git a/tornado/platform/common.py b/tornado/platform/common.py
index 2d4065ca..b409a903 100644
--- a/tornado/platform/common.py
+++ b/tornado/platform/common.py
@@ -3,26 +3,10 @@ from __future__ import absolute_import, division, print_function, with_statement
 
 import errno
 import socket
-import time
 
 from tornado.platform import interface
 
 
-def try_close(f):
-    # Avoid issue #875 (race condition when using the file in another
-    # thread).
-    for i in range(10):
-        try:
-            f.close()
-        except IOError:
-            # Yield to another thread
-            time.sleep(1e-3)
-        else:
-            break
-    # Try a last time and let raise
-    f.close()
-
-
 class Waker(interface.Waker):
     """Create an OS independent asynchronous pipe.
 
@@ -91,7 +75,7 @@ class Waker(interface.Waker):
     def wake(self):
         try:
             self.writer.send(b"x")
-        except (IOError, socket.error, ValueError):
+        except (IOError, socket.error):
             pass
 
     def consume(self):
@@ -105,4 +89,4 @@ class Waker(interface.Waker):
 
     def close(self):
         self.reader.close()
-        try_close(self.writer)
+        self.writer.close()
diff --git a/tornado/platform/posix.py b/tornado/platform/posix.py
index 572c0139..41a5794c 100644
--- a/tornado/platform/posix.py
+++ b/tornado/platform/posix.py
@@ -21,7 +21,7 @@ from __future__ import absolute_import, division, print_function, with_statement
 import fcntl
 import os
 
-from tornado.platform import common, interface
+from tornado.platform import interface
 
 
 def set_close_exec(fd):
@@ -53,7 +53,7 @@ class Waker(interface.Waker):
     def wake(self):
         try:
             self.writer.write(b"x")
-        except (IOError, ValueError):
+        except IOError:
             pass
 
     def consume(self):
@@ -67,4 +67,4 @@ class Waker(interface.Waker):
 
     def close(self):
         self.reader.close()
-        common.try_close(self.writer)
+        self.writer.close()
diff --git a/tornado/web.py b/tornado/web.py
index a0cb0e8e..f4c50e3c 100644
--- a/tornado/web.py
+++ b/tornado/web.py
@@ -2191,13 +2191,29 @@ class RedirectHandler(RequestHandler):
         application = web.Application([
             (r"/oldpath", web.RedirectHandler, {"url": "/newpath"}),
         ])
+
+    `RedirectHandler` supports regular expression substitutions. E.g., to
+    swap the first and second parts of a path while preserving the remainder::
+
+        application = web.Application([
+            (r"/(.*?)/(.*?)/(.*)", web.RedirectHandler, {"url": "/{1}/{0}/{2}"}),
+        ])
+
+    The final URL is formatted with `str.format` and the substrings that match
+    the capturing groups. In the above example, a request to "/a/b/c" would be
+    formatted like::
+
+        str.format("/{1}/{0}/{2}", "a", "b", "c")  # -> "/b/a/c"
+
+    Use Python's :ref:`format string syntax <formatstrings>` to customize how
+    values are substituted.
     """
     def initialize(self, url, permanent=True):
         self._url = url
         self._permanent = permanent
 
-    def get(self):
-        self.redirect(self._url, permanent=self._permanent)
+    def get(self, *args):
+        self.redirect(self._url.format(*args), permanent=self._permanent)
 
 
 class StaticFileHandler(RequestHandler):


Test patch:
diff --git a/tornado/test/web_test.py b/tornado/test/web_test.py
index 89f34071..b09bb9ab 100644
--- a/tornado/test/web_test.py
+++ b/tornado/test/web_test.py
@@ -2866,3 +2866,20 @@ class URLSpecReverseTest(unittest.TestCase):
     def test_reverse_arguments(self):
         self.assertEqual('/api/v1/foo/bar',
                          url(r'^/api/v1/foo/(\w+)$', None).reverse('bar'))
+
+
+class RedirectHandlerTest(WebTestCase):
+    def get_handlers(self):
+        return [
+            ('/src', WebRedirectHandler, {'url': '/dst'}),
+            (r'/(.*?)/(.*?)/(.*)', WebRedirectHandler, {'url': '/{1}/{0}/{2}'})]
+
+    def test_basic_redirect(self):
+        response = self.fetch('/src', follow_redirects=False)
+        self.assertEqual(response.code, 301)
+        self.assertEqual(response.headers['Location'], '/dst')
+
+    def test_redirect_pattern(self):
+        response = self.fetch('/a/b/c', follow_redirects=False)
+        self.assertEqual(response.code, 301)
+        self.assertEqual(response.headers['Location'], '/b/a/c')