search

mopidy / mopidy-local-sqlite

DEPRECATED (Mopidy SQLite local library extension)
https://mopidy.com
Apache License 2.0
30 stars 10 forks source link

crash when mopidy local is disabled #30

Closed woutervanwijk closed 10 years ago

woutervanwijk commented 10 years ago

When mopidy-local is not enabled, but mopidy-local-sqlite is installed, it will crash the webserver. Seems like it wants a configuration key media_dir that's not available. Error:

File "/usr/lib/python2.7/threading.py", line 808, in bootstrap_inner self.run() File "/usr/local/lib/python2.7/dist-packages/mopidy/http/actor.py", line 103, in run self.app = tornado.web.Application(self._get_request_handlers()) File "/usr/local/lib/python2.7/dist-packages/mopidy/http/actor.py", line 118, in _get_request_handlers request_handlers.extend(self._get_app_request_handlers()) File "/usr/local/lib/python2.7/dist-packages/mopidy/http/actor.py", line 136, in _get_app_request_handlers request_handlers = app['factory'](self.config, self.core) File "/usr/local/lib/python2.7/dist-packages/mopidy_local_sqlite/http.py", line 26, in factory images = ImageDirectory(config) File "/usr/local/lib/python2.7/dist-packages/mopidy_local_sqlite/images.py", line 26, in __init self._media_dir = config['local']['media_dir'] File "/usr/local/lib/python2.7/dist-packages/mopidy/config/init.py", line 248, in getitem item = self._data.getitem(key) KeyError: u'media_dir'

jodal commented 10 years ago

Mopidy-Local-SQLite is just a library extension to Mopidy-Local itself, thus Mopidy-Local must also be activated for Mopidy-Local-SQLite to work.

jodal commented 10 years ago

That said, Mopidy-Local-SQLite could try to handle this nicer, e.g. by checking in Extension.validate_environment() that the Mopidy-Local config is present. If the Mopidy-Local extension is disabled, the matching config won't be available, IIRC.

woutervanwijk commented 10 years ago

I know, but if I disable one extension, another one that is installed shouldn't crash mopidy, imho. It should just do nothing. Or mopidy should check if an extension depends on another and disable the child-extension if the parent is disabled?

jodal commented 10 years ago

Agree. Mopidy doesn't know anything about the extension relations, so it must be the child extension that checks in Extensions.validate_environment() if the parent extension is available, and if not, raise the proper exception so it is disabled too.

tkem commented 10 years ago

Agreed. Maybe that's something that should also go into the Mopidy docs, in the "Extension Development" section?

jodal commented 10 years ago

Hum, you don't have access to the config at the time of Extension.validate_environment(), so one needs to fail the extension at a later point. For a regular frontend, I'd simply raise FrontendError from the frontend's __init__(). For a local library, it should be possible to do something similar.

tkem commented 10 years ago

AFIACS, what this whole issue really boils down is that an exception in a Web application's factory method will go unhandled, and will prevent Mopidy's Web server from being started. IMHO, exceptions from the factory methods registered via http:app should be caught in HttpServer._get_app_request_handlers(), and the corresponding Web app should be disabled without affecting other Web applications. However, I'll provide a workaround for this special case in Mopidy-Local-SQLite, ASAP.