Merge pull request #1020 from adamcik/feature/fix-ws-disconnect-race

Fix WS disconnect race

docs/changelog.rst

@@ -98,6 +98,10 @@ v0.20.0 (UNRELEASED)
   "database". If you insist on using a client that needs these commands change
   :confval:`mpd/command_blacklist`.
 
+**HTTP frontend**
+
+- Prevent race condition in webservice broadcast from breaking the server.
+
 **Audio**
 
 - Deprecated :meth:`mopidy.audio.Audio.emit_end_of_stream`. Pass a

mopidy/http/handlers.py

@@ -10,7 +10,7 @@ import tornado.websocket
 
 import mopidy
 from mopidy import core, models
-from mopidy.utils import jsonrpc
+from mopidy.utils import encoding, jsonrpc
 
 
 logger = logging.getLogger(__name__)
@@ -75,7 +75,16 @@ class WebSocketHandler(tornado.websocket.WebSocketHandler):
     @classmethod
     def broadcast(cls, msg):
         for client in cls.clients:
-            client.write_message(msg)
+            # We could check for client.ws_connection, but we don't really
+            # care why the broadcast failed, we just want the rest of them
+            # to succeed, so catch everything.
+            try:
+                client.write_message(msg)
+            except Exception as e:
+                error_msg = encoding.locale_decode(e)
+                logger.debug('Broadcast of WebSocket message to %s failed: %s',
+                             client.request.remote_ip, error_msg)
+                # TODO: should this do the same cleanup as the on_message code?
 
     def initialize(self, core):
         self.jsonrpc = make_jsonrpc_wrapper(core)
@@ -113,7 +122,8 @@ class WebSocketHandler(tornado.websocket.WebSocketHandler):
                     'Sent WebSocket message to %s: %r',
                     self.request.remote_ip, response)
         except Exception as e:
-            logger.error('WebSocket request error: %s', e)
+            error_msg = encoding.locale_decode(e)
+            logger.error('WebSocket request error: %s', error_msg)
             if self.ws_connection:
                 # Tornado 3.2+ checks if self.ws_connection is None before
                 # using it, but not older versions.

tests/http/test_handlers.py

@@ -2,8 +2,11 @@ from __future__ import absolute_import, unicode_literals
 
 import os
 
+import mock
+
 import tornado.testing
 import tornado.web
+import tornado.websocket
 
 import mopidy
 from mopidy.http import handlers
@@ -35,3 +38,49 @@ class StaticFileHandlerTest(tornado.testing.AsyncHTTPTestCase):
             response.headers['X-Mopidy-Version'], mopidy.__version__)
         self.assertEqual(
             response.headers['Cache-Control'], 'no-cache')
+
+
+# We aren't bothering with skipIf as then we would need to "backport" gen_test
+if hasattr(tornado.websocket, 'websocket_connect'):
+    class WebSocketHandlerTest(tornado.testing.AsyncHTTPTestCase):
+        def get_app(self):
+            self.core = mock.Mock()
+            return tornado.web.Application([
+                (r'/ws/?', handlers.WebSocketHandler, {'core': self.core})
+            ])
+
+        def connection(self):
+            url = self.get_url('/ws').replace('http', 'ws')
+            return tornado.websocket.websocket_connect(url, self.io_loop)
+
+        @tornado.testing.gen_test
+        def test_invalid_json_rpc_request_doesnt_crash_handler(self):
+            # An uncaught error would result in no message, so this is just a
+            # simplistic test to verify this.
+            conn = yield self.connection()
+            conn.write_message('invalid request')
+            message = yield conn.read_message()
+            self.assertTrue(message)
+
+        @tornado.testing.gen_test
+        def test_broadcast_makes_it_to_client(self):
+            conn = yield self.connection()
+            handlers.WebSocketHandler.broadcast('message')
+            message = yield conn.read_message()
+            self.assertEqual(message, 'message')
+
+        @tornado.testing.gen_test
+        def test_broadcast_to_client_that_just_closed_connection(self):
+            conn = yield self.connection()
+            conn.stream.close()
+            handlers.WebSocketHandler.broadcast('message')
+
+        @tornado.testing.gen_test
+        def test_broadcast_to_client_without_ws_connection_present(self):
+            yield self.connection()
+            # Tornado checks for ws_connection and raises WebSocketClosedError
+            # if it is missing, this test case simulates winning a race were
+            # this has happened but we have not yet been removed from clients.
+            for client in handlers.WebSocketHandler.clients:
+                client.ws_connection = None
+            handlers.WebSocketHandler.broadcast('message')