http: Make WS broadcast more robust against disconnect race

Adds some WebSocketHandler tests that actually connect using a WS client and
plugs a potential race condition.

Any call to write_message could fail, either due to WebSocketClosedError like
in the log below, or simply due to socket errors. To play it safe we catch all
errors and debug log that a broadcast failed.

2015-02-26 21:24:02,266 ERROR    [HttpServer] /home/adamcik/dev/mopidy/mopidy/http/handlers.py:116
  mopidy.http.handlers WebSocket request error: deque index out of range
2015-02-26 21:24:10,098 ERROR    [HttpFrontend-11] build/bdist.linux-x86_64/egg/pykka/actor.py:268
  pykka Unhandled exception in HttpFrontend (urn:uuid:e376bd95-c32e-4e17-ad20-7d0b3c0cf2b2):
Traceback (most recent call last):
  File "build/bdist.linux-x86_64/egg/pykka/actor.py", line 200, in _actor_loop
    response = self._handle_receive(message)
  File "build/bdist.linux-x86_64/egg/pykka/actor.py", line 294, in _handle_receive
    return callee(*message['args'], **message['kwargs'])
  File ".../dev/mopidy/mopidy/http/actor.py", line 77, in on_event
    on_event(name, **data)
  File ".../dev/mopidy/mopidy/http/actor.py", line 84, in on_event
    handlers.WebSocketHandler.broadcast(message)
  File ".../dev/mopidy/mopidy/http/handlers.py", line 78, in broadcast
    client.write_message(msg)
  File ".../dev/mopidy-virtualenv/local/lib/python2.7/site-packages/tornado/websocket.py", line 183, in write_message
    raise WebSocketClosedError()
WebSocketClosedError

mopidy/http/handlers.py

@@ -75,7 +75,15 @@ class WebSocketHandler(tornado.websocket.WebSocketHandler):
     @classmethod
     def broadcast(cls, msg):
         for client in cls.clients:
+            # 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:
+                logger.debug('Broadcast of WebSocket message to %s failed: %s',
+                             client.request.remote_ip, e)
+                # TODO: should this do the same cleanup as the on_message code?
 
     def initialize(self, core):
         self.jsonrpc = make_jsonrpc_wrapper(core)

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,47 @@ class StaticFileHandlerTest(tornado.testing.AsyncHTTPTestCase):
             response.headers['X-Mopidy-Version'], mopidy.__version__)
         self.assertEqual(
             response.headers['Cache-Control'], 'no-cache')
+
+
+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.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')