From f9755b562cbedb0d66ca1ea536fda21ec9daeff2 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Wed, 16 Jul 2014 22:21:05 +0200 Subject: [PATCH 1/5] http: Raise FrontendError if socket creation fails This removes the stacktraces when two Mopidy instances are started with the same hostname/port configuration. --- mopidy/http/actor.py | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/mopidy/http/actor.py b/mopidy/http/actor.py index 96c71c8c..c9272bb6 100644 --- a/mopidy/http/actor.py +++ b/mopidy/http/actor.py @@ -11,10 +11,10 @@ import tornado.ioloop import tornado.web import tornado.websocket -from mopidy import models, zeroconf +from mopidy import exceptions, models, zeroconf from mopidy.core import CoreListener from mopidy.http import handlers -from mopidy.utils import formatting +from mopidy.utils import encoding, formatting logger = logging.getLogger(__name__) @@ -33,7 +33,16 @@ class HttpFrontend(pykka.ThreadingActor, CoreListener): self.port = config['http']['port'] self.zeroconf_name = config['http']['zeroconf'] self.zeroconf_service = None - self.app = None + + try: + logger.debug('Starting HTTP server') + self.app = tornado.web.Application(self._get_request_handlers()) + self.app.listen( + self.port, self.hostname if self.hostname != '::' else None) + except IOError as error: + raise exceptions.FrontendError( + 'HTTP server startup failed: %s' % + encoding.locale_decode(error)) def on_start(self): threading.Thread(target=self._startup).start() @@ -44,10 +53,6 @@ class HttpFrontend(pykka.ThreadingActor, CoreListener): tornado.ioloop.IOLoop.instance().add_callback(self._shutdown) def _startup(self): - logger.debug('Starting HTTP server') - self.app = tornado.web.Application(self._get_request_handlers()) - self.app.listen(self.port, - self.hostname if self.hostname != '::' else None) logger.info( 'HTTP server running at http://%s:%s', self.hostname, self.port) tornado.ioloop.IOLoop.instance().start() From e9243357ae4321968c6a3eb0331c60ac3f2789f5 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Wed, 16 Jul 2014 22:22:07 +0200 Subject: [PATCH 2/5] http: Format "server running" log msg like MPD By removing the http://... URL in the log output, we hopefully remove the dubious connection between hostname/port configuration and the URL you'll actually use to browse the Mopidy web server. Example log output with this change: ... INFO Starting Mopidy frontends: HttpFrontend, MpdFrontend INFO HTTP server running at [::ffff:127.0.0.1]:6680 INFO MPD server running at [::ffff:127.0.0.1]:6600 --- mopidy/http/actor.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/mopidy/http/actor.py b/mopidy/http/actor.py index c9272bb6..9fd4ffca 100644 --- a/mopidy/http/actor.py +++ b/mopidy/http/actor.py @@ -14,7 +14,7 @@ import tornado.websocket from mopidy import exceptions, models, zeroconf from mopidy.core import CoreListener from mopidy.http import handlers -from mopidy.utils import encoding, formatting +from mopidy.utils import encoding, formatting, network logger = logging.getLogger(__name__) @@ -54,7 +54,8 @@ class HttpFrontend(pykka.ThreadingActor, CoreListener): def _startup(self): logger.info( - 'HTTP server running at http://%s:%s', self.hostname, self.port) + 'HTTP server running at [%s]:%s', + network.format_hostname(self.hostname), self.port) tornado.ioloop.IOLoop.instance().start() def _shutdown(self): From 215dd777f6a2366bb90d2d1953e9e011e059f710 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 17 Jul 2014 00:58:10 +0200 Subject: [PATCH 3/5] http: Move Mopidy request handlers to helper method --- mopidy/http/actor.py | 43 ++++++++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/mopidy/http/actor.py b/mopidy/http/actor.py index 3324a277..f705c707 100644 --- a/mopidy/http/actor.py +++ b/mopidy/http/actor.py @@ -86,32 +86,15 @@ class HttpFrontend(pykka.ThreadingActor, CoreListener): def _get_request_handlers(self): request_handlers = [] - request_handlers.extend(self._get_app_request_handlers()) request_handlers.extend(self._get_static_request_handlers()) - - # Either default Mopidy or user defined path to files - static_dir = self.config['http']['static_dir'] - if static_dir and not os.path.exists(static_dir): - logger.warning( - 'Configured http/static_dir %s does not exist. ' - 'Falling back to default HTTP handler.', static_dir) - static_dir = None - if static_dir: - request_handlers.append((r'/(.*)', handlers.StaticFileHandler, { - 'path': self.config['http']['static_dir'], - 'default_filename': 'index.html', - })) - else: - request_handlers.append((r'/', tornado.web.RedirectHandler, { - 'url': '/mopidy/', - 'permanent': False, - })) + request_handlers.extend(self._get_mopidy_request_handlers()) logger.debug( 'HTTP routes from extensions: %s', formatting.indent('\n'.join( '%r: %r' % (r[0], r[1]) for r in request_handlers))) + return request_handlers def _get_app_request_handlers(self): @@ -146,3 +129,25 @@ class HttpFrontend(pykka.ThreadingActor, CoreListener): )) logger.debug('Loaded static HTTP extension: %s', static['name']) return result + + def _get_mopidy_request_handlers(self): + # Either default Mopidy or user defined path to files + + static_dir = self.config['http']['static_dir'] + + if static_dir and not os.path.exists(static_dir): + logger.warning( + 'Configured http/static_dir %s does not exist. ' + 'Falling back to default HTTP handler.', static_dir) + static_dir = None + + if static_dir: + return [(r'/(.*)', handlers.StaticFileHandler, { + 'path': self.config['http']['static_dir'], + 'default_filename': 'index.html', + })] + else: + return [(r'/', tornado.web.RedirectHandler, { + 'url': '/mopidy/', + 'permanent': False, + })] From 3fac0cb8de2aa089f7ee858eef12d85d0feb0154 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 17 Jul 2014 00:59:09 +0200 Subject: [PATCH 4/5] http: Split socket and server creation --- mopidy/http/actor.py | 71 ++++++++++++++++++++++++++------------- tests/http/test_server.py | 33 ++++++++++-------- 2 files changed, 67 insertions(+), 37 deletions(-) diff --git a/mopidy/http/actor.py b/mopidy/http/actor.py index f705c707..dc154538 100644 --- a/mopidy/http/actor.py +++ b/mopidy/http/actor.py @@ -7,7 +7,9 @@ import threading import pykka +import tornado.httpserver import tornado.ioloop +import tornado.netutil import tornado.web import tornado.websocket @@ -26,28 +28,32 @@ class HttpFrontend(pykka.ThreadingActor, CoreListener): def __init__(self, config, core): super(HttpFrontend, self).__init__() - self.config = config - self.core = core - self.hostname = config['http']['hostname'] + self.hostname = network.format_hostname(config['http']['hostname']) self.port = config['http']['port'] - - self.zeroconf_name = config['http']['zeroconf'] - self.zeroconf_http = None - self.zeroconf_mopidy_http = None + tornado_hostname = config['http']['hostname'] + if tornado_hostname == '::': + tornado_hostname = None try: logger.debug('Starting HTTP server') - self.app = tornado.web.Application(self._get_request_handlers()) - self.app.listen( - self.port, self.hostname if self.hostname != '::' else None) + sockets = tornado.netutil.bind_sockets(self.port, tornado_hostname) + self.server = HttpServer( + config=config, core=core, sockets=sockets, + apps=self.apps, statics=self.statics) except IOError as error: raise exceptions.FrontendError( 'HTTP server startup failed: %s' % encoding.locale_decode(error)) + self.zeroconf_name = config['http']['zeroconf'] + self.zeroconf_http = None + self.zeroconf_mopidy_http = None + def on_start(self): - threading.Thread(target=self._startup).start() + logger.info( + 'HTTP server running at [%s]:%s', self.hostname, self.port) + self.server.start() if self.zeroconf_name: self.zeroconf_http = zeroconf.Zeroconf( @@ -65,18 +71,7 @@ class HttpFrontend(pykka.ThreadingActor, CoreListener): if self.zeroconf_mopidy_http: self.zeroconf_mopidy_http.unpublish() - tornado.ioloop.IOLoop.instance().add_callback(self._shutdown) - - def _startup(self): - logger.info( - 'HTTP server running at [%s]:%s', - network.format_hostname(self.hostname), self.port) - tornado.ioloop.IOLoop.instance().start() - - def _shutdown(self): - logger.debug('Stopping HTTP server') - tornado.ioloop.IOLoop.instance().stop() - logger.debug('Stopped HTTP server') + self.server.stop() def on_event(self, name, **data): event = data @@ -84,6 +79,36 @@ class HttpFrontend(pykka.ThreadingActor, CoreListener): message = json.dumps(event, cls=models.ModelJSONEncoder) handlers.WebSocketHandler.broadcast(message) + +class HttpServer(threading.Thread): + name = 'HttpServer' + + def __init__(self, config, core, sockets, apps, statics): + super(HttpServer, self).__init__() + + self.config = config + self.core = core + self.sockets = sockets + self.apps = apps + self.statics = statics + + self.app = None + self.server = None + + def run(self): + self.app = tornado.web.Application(self._get_request_handlers()) + self.server = tornado.httpserver.HTTPServer(self.app) + self.server.add_sockets(self.sockets) + + tornado.ioloop.IOLoop.instance().start() + + logger.debug('Stopped HTTP server') + + def stop(self): + logger.debug('Stopping HTTP server') + tornado.ioloop.IOLoop.instance().add_callback( + tornado.ioloop.IOLoop.instance().stop) + def _get_request_handlers(self): request_handlers = [] request_handlers.extend(self._get_app_request_handlers()) diff --git a/tests/http/test_server.py b/tests/http/test_server.py index cc20efe0..b3cfa92c 100644 --- a/tests/http/test_server.py +++ b/tests/http/test_server.py @@ -27,16 +27,19 @@ class HttpServerTest(tornado.testing.AsyncHTTPTestCase): core.get_version = mock.MagicMock(name='get_version') core.get_version.return_value = mopidy.__version__ - apps = [dict(name='testapp')] - statics = [dict(name='teststatic')] + testapps = [dict(name='testapp')] + teststatics = [dict(name='teststatic')] - http_frontend = actor.HttpFrontend(config=self.get_config(), core=core) - http_frontend.apps = [{ + apps = [{ 'name': 'mopidy', - 'factory': handlers.make_mopidy_app_factory(apps, statics), + 'factory': handlers.make_mopidy_app_factory(testapps, teststatics), }] - return tornado.web.Application(http_frontend._get_request_handlers()) + http_server = actor.HttpServer( + config=self.get_config(), core=core, sockets=[], + apps=apps, statics=[]) + + return tornado.web.Application(http_server._get_request_handlers()) class RootRedirectTest(HttpServerTest): @@ -172,12 +175,12 @@ class HttpServerWithStaticFilesTest(tornado.testing.AsyncHTTPTestCase): } core = mock.Mock() - http_frontend = actor.HttpFrontend(config=config, core=core) - http_frontend.statics = [ - dict(name='static', path=os.path.dirname(__file__)), - ] + statics = [dict(name='static', path=os.path.dirname(__file__))] - return tornado.web.Application(http_frontend._get_request_handlers()) + http_server = actor.HttpServer( + config=config, core=core, sockets=[], apps=[], statics=statics) + + return tornado.web.Application(http_server._get_request_handlers()) def test_without_slash_should_redirect(self): response = self.fetch('/static', method='GET', follow_redirects=False) @@ -222,13 +225,15 @@ class HttpServerWithWsgiAppTest(tornado.testing.AsyncHTTPTestCase): } core = mock.Mock() - http_frontend = actor.HttpFrontend(config=config, core=core) - http_frontend.apps = [{ + apps = [{ 'name': 'wsgi', 'factory': wsgi_app_factory, }] - return tornado.web.Application(http_frontend._get_request_handlers()) + http_server = actor.HttpServer( + config=config, core=core, sockets=[], apps=apps, statics=[]) + + return tornado.web.Application(http_server._get_request_handlers()) def test_without_slash_should_redirect(self): response = self.fetch('/wsgi', method='GET', follow_redirects=False) From 1ebe1151fc0a13a954e220f69ea71a4354e0d6bc Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 17 Jul 2014 01:14:06 +0200 Subject: [PATCH 5/5] http: Make event emitting testable --- mopidy/http/actor.py | 12 ++++++++---- tests/http/test_events.py | 18 ++++++------------ 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/mopidy/http/actor.py b/mopidy/http/actor.py index dc154538..a477d939 100644 --- a/mopidy/http/actor.py +++ b/mopidy/http/actor.py @@ -74,10 +74,14 @@ class HttpFrontend(pykka.ThreadingActor, CoreListener): self.server.stop() def on_event(self, name, **data): - event = data - event['event'] = name - message = json.dumps(event, cls=models.ModelJSONEncoder) - handlers.WebSocketHandler.broadcast(message) + on_event(name, **data) + + +def on_event(name, **data): + event = data + event['event'] = name + message = json.dumps(event, cls=models.ModelJSONEncoder) + handlers.WebSocketHandler.broadcast(message) class HttpServer(threading.Thread): diff --git a/tests/http/test_events.py b/tests/http/test_events.py index e86d2b2e..d03778a6 100644 --- a/tests/http/test_events.py +++ b/tests/http/test_events.py @@ -10,20 +10,12 @@ from mopidy.http import actor @mock.patch('mopidy.http.handlers.WebSocketHandler.broadcast') class HttpEventsTest(unittest.TestCase): - def setUp(self): - config = { - 'http': { - 'hostname': '127.0.0.1', - 'port': 6680, - 'static_dir': None, - 'zeroconf': '', - } - } - self.http = actor.HttpFrontend(config=config, core=mock.Mock()) def test_track_playback_paused_is_broadcasted(self, broadcast): broadcast.reset_mock() - self.http.on_event('track_playback_paused', foo='bar') + + actor.on_event('track_playback_paused', foo='bar') + self.assertDictEqual( json.loads(str(broadcast.call_args[0][0])), { 'event': 'track_playback_paused', @@ -32,7 +24,9 @@ class HttpEventsTest(unittest.TestCase): def test_track_playback_resumed_is_broadcasted(self, broadcast): broadcast.reset_mock() - self.http.on_event('track_playback_resumed', foo='bar') + + actor.on_event('track_playback_resumed', foo='bar') + self.assertDictEqual( json.loads(str(broadcast.call_args[0][0])), { 'event': 'track_playback_resumed',