Repository: tornadoweb/tornado

Issue:
Unhandled exception in open coroutine of websockethandler does not close connection
I tested the following app with version 5.1.1 of tornado:

```
import tornado
import tornado.websocket
import tornado.ioloop
from tornado import gen


class MySocket(tornado.websocket.WebSocketHandler):
    def open(self):
        print 'socket opened'
        raise Exception('bad')

    @gen.coroutine
    def on_message(self, message):
        print message

    def on_close(self):
        print 'closed'


def create_application():
    return tornado.web.Application([
        (r"^/?$", MySocket),
    ])


application = create_application()
application.listen(8000)
tornado.ioloop.IOLoop.current().start()
```

If I connect with a client, I get the following (correct) output on the server side:

```
socket opened
ERROR:tornado.application:Uncaught exception GET / (127.0.0.1)
HTTPServerRequest(protocol='http', host='localhost:8000', method='GET', uri='/', version='HTTP/1.1', remote_ip='127.0.0.1')
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/site-packages/tornado/websocket.py", line 546, in _run_callback
    result = callback(*args, **kwargs)
  File "./test.py", line 10, in open
    raise Exception('bad')
Exception: bad
closed
```

However, if I add the gen.coroutine annotation to ``open`` and run the same test, I get this output instead:

```
socket opened
ERROR:tornado.application:Exception in callback <functools.partial object at 0x7fea30c31c00>
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/site-packages/tornado/ioloop.py", line 758, in _run_callback
    ret = callback()
  File "/usr/local/lib/python2.7/site-packages/tornado/stack_context.py", line 300, in null_wrapper
    return fn(*args, **kwargs)
  File "/usr/local/lib/python2.7/site-packages/tornado/websocket.py", line 553, in <lambda>
    self.stream.io_loop.add_future(result, lambda f: f.result())
  File "/usr/local/lib/python2.7/site-packages/tornado/concurrent.py", line 261, in result
    raise_exc_info(self._exc_info)
  File "/usr/local/lib/python2.7/site-packages/tornado/gen.py", line 307, in wrapper
    result = func(*args, **kwargs)
  File "./test.py", line 11, in open
    raise Exception('bad')
Exception: bad
ERROR:tornado.application:Future exception was never retrieved: Traceback (most recent call last):
  File "/usr/local/lib/python2.7/site-packages/tornado/gen.py", line 1141, in run
    yielded = self.gen.throw(*exc_info)
  File "/usr/local/lib/python2.7/site-packages/tornado/websocket.py", line 757, in _accept_connection
    yield open_result
  File "/usr/local/lib/python2.7/site-packages/tornado/gen.py", line 1133, in run
    value = future.result()
  File "/usr/local/lib/python2.7/site-packages/tornado/concurrent.py", line 261, in result
    raise_exc_info(self._exc_info)
  File "/usr/local/lib/python2.7/site-packages/tornado/gen.py", line 307, in wrapper
    result = func(*args, **kwargs)
  File "./test.py", line 11, in open
    raise Exception('bad')
Exception: bad
```

Note that there is no "closed" message. On the client side, the client still thinks the connection is still open and I can send messages without error. Without the coroutine, the client gets a broken pipe error the next time it tries to send a message.

Discussion:
ploxiln: So, uh, this didn't work out as well as I expected - in branch5.1 websocket_connect() raises HTTPStreamClosedError for these cases, even without the backported fix to websocket.py, so "fixing" the tests for branch5.1 makes them not really distinguish before the fix from after it.

(and the last travis failure was a bunch of flake8 errors which I think are just because a new stable version of flake8 was just released)
bdarnell: > So, uh, this didn't work out as well as I expected - in branch5.1 websocket_connect() raises HTTPStreamClosedError for these cases, even without the backported fix to websocket.py, so "fixing" the tests for branch5.1 makes them not really distinguish before the fix from after it.

That surprises me. The original bug report (#2570) was that the connection would hang and the client would never see the error. Where is the HTTPStreamClosedError coming from if the server isn't catching the error correctly? 

Maybe that `gen.sleep(0)` is not actually forcing an extra IOLoop iteration so it's making the coroutine act like it's synchronous (seems unlikely but maybe I'm forgetting a fast path that differs with the move from `await asyncio.sleep(0)` in 6.0 to `yield gen.sleep(0)` in 5.1)
ploxiln: yeah, I'll investigate a bit and try some more stuff

Patch:
diff --git a/.flake8 b/.flake8
index 803d06d2..1c2c768d 100644
--- a/.flake8
+++ b/.flake8
@@ -10,6 +10,4 @@ ignore =
     E402,
     # E722 do not use bare except
     E722,
-    # W504 line break after binary operator
-    W504,
 doctests = true
diff --git a/tornado/util.py b/tornado/util.py
index 151b0d88..a42ebebe 100644
--- a/tornado/util.py
+++ b/tornado/util.py
@@ -59,7 +59,7 @@ else:
     import datetime  # noqa
     import types  # noqa
     from typing import Any, AnyStr, Union, Optional, Dict, Mapping  # noqa
-    from typing import List, Tuple, Match, Callable  # noqa
+    from typing import Tuple, Match, Callable  # noqa
 
     if PY3:
         _BaseString = str
@@ -252,7 +252,7 @@ _re_unescape_pattern = re.compile(r'\\(.)', re.DOTALL)
 
 def re_unescape(s):
     # type: (str) -> str
-    r"""Unescape a string escaped by `re.escape`.
+    """Unescape a string escaped by `re.escape`.
 
     May raise ``ValueError`` for regular expressions which could not
     have been produced by `re.escape` (for example, strings containing
diff --git a/tornado/web.py b/tornado/web.py
index 4340b16f..6760b0b9 100644
--- a/tornado/web.py
+++ b/tornado/web.py
@@ -1887,7 +1887,7 @@ class _ApplicationRouter(ReversibleRuleRouter):
 
 
 class Application(ReversibleRouter):
-    r"""A collection of request handlers that make up a web application.
+    """A collection of request handlers that make up a web application.
 
     Instances of this class are callable and can be passed directly to
     HTTPServer to serve the application::
diff --git a/tornado/websocket.py b/tornado/websocket.py
index 0b994fc1..7b77850a 100644
--- a/tornado/websocket.py
+++ b/tornado/websocket.py
@@ -751,10 +751,14 @@ class WebSocketProtocol13(WebSocketProtocol):
         self.stream = self.handler.stream
 
         self.start_pinging()
-        open_result = self._run_callback(self.handler.open, *self.handler.open_args,
-                                         **self.handler.open_kwargs)
-        if open_result is not None:
-            yield open_result
+        try:
+            open_result = self.handler.open(*self.handler.open_args, **self.handler.open_kwargs)
+            if open_result is not None:
+                yield open_result
+        except Exception:
+            self.handler.log_exception(*sys.exc_info())
+            self._abort()
+
         yield self._receive_frame_loop()
 
     def _parse_extensions_header(self, headers):


Test patch:
diff --git a/tornado/test/web_test.py b/tornado/test/web_test.py
index b1ae4747..16938c42 100644
--- a/tornado/test/web_test.py
+++ b/tornado/test/web_test.py
@@ -398,7 +398,7 @@ class AuthRedirectTest(WebTestCase):
                               follow_redirects=False)
         self.assertEqual(response.code, 302)
         self.assertTrue(re.match(
-            r'http://example.com/login\?next=http%3A%2F%2F127.0.0.1%3A[0-9]+%2Fabsolute',
+            'http://example.com/login\?next=http%3A%2F%2F127.0.0.1%3A[0-9]+%2Fabsolute',
             response.headers['Location']), response.headers['Location'])
 
 
diff --git a/tornado/test/websocket_test.py b/tornado/test/websocket_test.py
index ea7b1e4c..1d5bfa68 100644
--- a/tornado/test/websocket_test.py
+++ b/tornado/test/websocket_test.py
@@ -181,6 +181,18 @@ class OpenCoroutineHandler(TestWebSocketHandler):
         self.write_message('ok')
 
 
+class ErrorInOpenHandler(TestWebSocketHandler):
+    def open(self):
+        raise Exception("boom")
+
+
+class ErrorInAsyncOpenHandler(TestWebSocketHandler):
+    @gen.coroutine
+    def open(self):
+        yield gen.sleep(0.01)
+        raise Exception("boom")
+
+
 class WebSocketBaseTestCase(AsyncHTTPTestCase):
     @gen.coroutine
     def ws_connect(self, path, **kwargs):
@@ -225,6 +237,10 @@ class WebSocketTest(WebSocketBaseTestCase):
              dict(close_future=self.close_future)),
             ('/open_coroutine', OpenCoroutineHandler,
              dict(close_future=self.close_future, test=self)),
+            ("/error_in_open", ErrorInOpenHandler,
+             dict(close_future=self.close_future)),
+            ("/error_in_async_open", ErrorInAsyncOpenHandler,
+             dict(close_future=self.close_future)),
         ], template_loader=DictLoader({
             'message.html': '<b>{{ message }}</b>',
         }))
@@ -511,6 +527,20 @@ class WebSocketTest(WebSocketBaseTestCase):
         self.assertEqual(res, 'ok')
         yield self.close(ws)
 
+    @gen_test
+    def test_error_in_open(self):
+        with ExpectLog(app_log, "Uncaught exception"):
+            ws = yield self.ws_connect("/error_in_open")
+            res = yield ws.read_message()
+        self.assertIsNone(res)
+
+    @gen_test
+    def test_error_in_async_open(self):
+        with ExpectLog(app_log, "Uncaught exception"):
+            ws = yield self.ws_connect("/error_in_async_open")
+            res = yield ws.read_message()
+        self.assertIsNone(res)
+
 
 if sys.version_info >= (3, 5):
     NativeCoroutineOnMessageHandler = exec_test(globals(), locals(), """