[WebUi] [Core] Fixes to plugin handling and WebUi plugin + tests

This should fix problems with errors occuring when failing to
enable plugins. Errors in plugin handling are handled better
and properly logged.

WebUI plugin in particular had issues when being enabled and disabled
multiple times because it was trying to create DelugeWeb component
each time it was enabled. If deluge-web is already listening on
the same port, enabling the WebUI plugin will fail, and the checkbox
will not be checked.

There are still some issues when enabling/disabling plugins by
clicking fast multiple times on the checkbox.

.travis.yml

@@ -30,7 +30,7 @@ env:
     - TOX_ENV=trial              APTPACKAGES="$APTPACKAGES $APTPACKAGES_GTKUI"
     - TOX_ENV=pygtkui            APTPACKAGES="$APTPACKAGES $APTPACKAGES_GTKUI"
 #    - TOX_ENV=testcoverage       APTPACKAGES="$APTPACKAGES $APTPACKAGES_GTKUI"
-#    - TOX_ENV=plugins
+    - TOX_ENV=plugins
 
 virtualenv:
   system_site_packages: true

deluge/component.py

@@ -10,8 +10,9 @@
 import logging
 from collections import defaultdict
 
+from twisted.internet import reactor
 from twisted.internet.defer import DeferredList, fail, maybeDeferred, succeed
-from twisted.internet.task import LoopingCall
+from twisted.internet.task import LoopingCall, deferLater
 
 log = logging.getLogger(__name__)
 
@@ -96,14 +97,13 @@ class Component(object):
             self._component_state = "Stopped"
             self._component_starting_deferred = None
             log.error(result)
-            return result
+            return fail(result)
 
         if self._component_state == "Stopped":
             if hasattr(self, "start"):
                 self._component_state = "Starting"
-                d = maybeDeferred(self.start)
+                d = deferLater(reactor, 1, self.start)
-                d.addCallback(on_start)
+                d.addCallbacks(on_start, on_start_fail)
-                d.addErrback(on_start_fail)
                 self._component_starting_deferred = d
             else:
                 d = maybeDeferred(on_start, None)
@@ -240,13 +240,13 @@ class ComponentRegistry(object):
         :type obj: object
 
         """
-
         if obj in self.components.values():
             log.debug("Deregistering Component: %s", obj._component_name)
             d = self.stop([obj._component_name])
 
             def on_stop(result, name):
-                del self.components[name]
+                # Component may have been removed, so pop to ensure it doesn't fail
+                self.components.pop(name, None)
             return d.addCallback(on_stop, obj._component_name)
         else:
             return succeed(None)

deluge/core/core.py

@@ -579,13 +579,11 @@ class Core(component.Component):
 
     @export
     def enable_plugin(self, plugin):
-        self.pluginmanager.enable_plugin(plugin)
+        return self.pluginmanager.enable_plugin(plugin)
-        return None
 
     @export
     def disable_plugin(self, plugin):
-        self.pluginmanager.disable_plugin(plugin)
+        return self.pluginmanager.disable_plugin(plugin)
-        return None
 
     @export
     def force_recheck(self, torrent_ids):

deluge/core/pluginmanager.py

@@ -12,6 +12,8 @@
 
 import logging
 
+from twisted.internet import defer
+
 import deluge.component as component
 import deluge.pluginmanagerbase
 from deluge.event import PluginDisabledEvent, PluginEnabledEvent
@@ -52,16 +54,29 @@ class PluginManager(deluge.pluginmanagerbase.PluginManagerBase, component.Compon
                     log.exception(ex)
 
     def enable_plugin(self, name):
+        d = defer.succeed(True)
         if name not in self.plugins:
-            deluge.pluginmanagerbase.PluginManagerBase.enable_plugin(self, name)
+            d = deluge.pluginmanagerbase.PluginManagerBase.enable_plugin(self, name)
-            if name in self.plugins:
+
-                component.get("EventManager").emit(PluginEnabledEvent(name))
+            def on_enable_plugin(result):
+                if result is True and name in self.plugins:
+                    component.get("EventManager").emit(PluginEnabledEvent(name))
+                return result
+
+            d.addBoth(on_enable_plugin)
+        return d
 
     def disable_plugin(self, name):
+        d = defer.succeed(True)
         if name in self.plugins:
-            deluge.pluginmanagerbase.PluginManagerBase.disable_plugin(self, name)
+            d = deluge.pluginmanagerbase.PluginManagerBase.disable_plugin(self, name)
-            if name not in self.plugins:
+
-                component.get("EventManager").emit(PluginDisabledEvent(name))
+            def on_disable_plugin(result):
+                if name not in self.plugins:
+                    component.get("EventManager").emit(PluginDisabledEvent(name))
+                return result
+            d.addBoth(on_disable_plugin)
+        return d
 
     def get_status(self, torrent_id, fields):
         """Return the value of status fields for the selected torrent_id."""

deluge/pluginmanagerbase.py

@@ -14,6 +14,8 @@ import logging
 import os.path
 
 import pkg_resources
+from twisted.internet import defer
+from twisted.python.failure import Failure
 
 import deluge.common
 import deluge.component as component
@@ -111,28 +113,47 @@ class PluginManagerBase(object):
             self.available_plugins.append(self.pkg_env[name][0].project_name)
 
     def enable_plugin(self, plugin_name):
-        """Enables a plugin"""
+        """Enable a plugin
+
+        Args:
+            plugin_name (str): The plugin name
+
+        Returns:
+            Deferred: A deferred with callback value True or False indicating
+                      whether the plugin is enabled or not.
+        """
         if plugin_name not in self.available_plugins:
             log.warning("Cannot enable non-existant plugin %s", plugin_name)
-            return
+            return defer.succeed(False)
 
         if plugin_name in self.plugins:
             log.warning("Cannot enable already enabled plugin %s", plugin_name)
-            return
+            return defer.succeed(True)
 
         plugin_name = plugin_name.replace(" ", "-")
         egg = self.pkg_env[plugin_name][0]
         egg.activate()
+        return_d = defer.succeed(True)
+
         for name in egg.get_entry_map(self.entry_name):
             entry_point = egg.get_entry_info(self.entry_name, name)
             try:
                 cls = entry_point.load()
                 instance = cls(plugin_name.replace("-", "_"))
+            except component.ComponentAlreadyRegistered as ex:
+                log.error(ex)
+                return defer.succeed(False)
             except Exception as ex:
                 log.error("Unable to instantiate plugin %r from %r!", name, egg.location)
                 log.exception(ex)
                 continue
-            instance.enable()
+            try:
+                return_d = defer.maybeDeferred(instance.enable)
+            except Exception as ex:
+                log.error("Unable to enable plugin '%s'!", name)
+                log.exception(ex)
+                return_d = defer.fail(False)
+
             if not instance.__module__.startswith("deluge.plugins."):
                 import warnings
                 warnings.warn_explicit(
@@ -141,25 +162,71 @@ class PluginManagerBase(object):
                     instance.__module__, 0
                 )
             if self._component_state == "Started":
-                component.start([instance.plugin._component_name])
+                def on_enabled(result, instance):
-            plugin_name = plugin_name.replace("-", " ")
+                    return component.start([instance.plugin._component_name])
-            self.plugins[plugin_name] = instance
+                return_d.addCallback(on_enabled, instance)
-            if plugin_name not in self.config["enabled_plugins"]:
+
-                log.debug("Adding %s to enabled_plugins list in config", plugin_name)
+            def on_started(result, instance):
-                self.config["enabled_plugins"].append(plugin_name)
+                plugin_name_space = plugin_name.replace("-", " ")
-            log.info("Plugin %s enabled..", plugin_name)
+                self.plugins[plugin_name_space] = instance
+                if plugin_name_space not in self.config["enabled_plugins"]:
+                    log.debug("Adding %s to enabled_plugins list in config", plugin_name_space)
+                    self.config["enabled_plugins"].append(plugin_name_space)
+                log.info("Plugin %s enabled..", plugin_name_space)
+                return True
+
+            def on_started_error(result, instance):
+                log.warn("Failed to start plugin '%s': %s", plugin_name, result.getTraceback())
+                component.deregister(instance.plugin)
+                return False
+
+            return_d.addCallbacks(on_started, on_started_error, callbackArgs=[instance], errbackArgs=[instance])
+            return return_d
+
+        return defer.succeed(False)
 
     def disable_plugin(self, name):
-        """Disables a plugin"""
+        """
-        try:
+        Disable a plugin
-            self.plugins[name].disable()
-            component.deregister(self.plugins[name].plugin)
-            del self.plugins[name]
-            self.config["enabled_plugins"].remove(name)
-        except KeyError:
-            log.warning("Plugin %s is not enabled..", name)
 
-        log.info("Plugin %s disabled..", name)
+        Args:
+            plugin_name (str): The plugin name
+
+        Returns:
+            Deferred: A deferred with callback value True or False indicating
+                      whether the plugin is disabled or not.
+        """
+        if name not in self.plugins:
+            log.warning("Plugin '%s' is not enabled..", name)
+            return defer.succeed(True)
+
+        try:
+            d = defer.maybeDeferred(self.plugins[name].disable)
+        except Exception as ex:
+            log.error("Error when disabling plugin '%s'", self.plugin._component_name)
+            log.exception(ex)
+            d = defer.succeed(False)
+
+        def on_disabled(result):
+            ret = True
+            if isinstance(result, Failure):
+                log.error("Error when disabling plugin '%s'", name)
+                log.exception(result.getTraceback())
+                ret = False
+            try:
+                component.deregister(self.plugins[name].plugin)
+                del self.plugins[name]
+                self.config["enabled_plugins"].remove(name)
+            except Exception as ex:
+                log.error("Unable to disable plugin '%s'!", name)
+                log.exception(ex)
+                ret = False
+            else:
+                log.info("Plugin %s disabled..", name)
+            return ret
+
+        d.addBoth(on_disabled)
+        return d
 
     def get_plugin_info(self, name):
         """Returns a dictionary of plugin info from the metadata"""

deluge/plugins/Stats/deluge/plugins/stats/core.py

@@ -100,7 +100,7 @@ class Core(CorePluginBase):
         try:
             self.update_timer.stop()
             self.save_timer.stop()
-        except:
+        except AssertionError:
             pass
 
     def add_stats(self, *stats):

deluge/plugins/Stats/deluge/plugins/stats/tests/test_stats.py

@@ -1,10 +1,17 @@
-import pytest
+# -*- coding: utf-8 -*-
-import twisted.internet.defer as defer
+#
+# This file is part of Deluge and is licensed under GNU General Public License 3.0, or later, with
+# the additional special exception to link portions of this program with the OpenSSL library.
+# See LICENSE for more details.
+#
+
+from twisted.internet import defer
 from twisted.trial import unittest
 
 import deluge.component as component
 from deluge.common import fsize
 from deluge.tests import common as tests_common
+from deluge.tests.basetest import BaseTestCase
 from deluge.ui.client import client
 
 
@@ -17,35 +24,41 @@ def print_totals(totals):
     print("down:", fsize(totals["total_download"] - totals["total_payload_download"]))
 
 
-class StatsTestCase(unittest.TestCase):
+class StatsTestCase(BaseTestCase):
 
-    def setUp(self):  # NOQA
+    def set_up(self):
         defer.setDebugging(True)
         tests_common.set_tmp_config_dir()
         client.start_classic_mode()
         client.core.enable_plugin("Stats")
+        return component.start()
 
-    def tearDown(self):  # NOQA
+    def tear_down(self):
         client.stop_classic_mode()
+        return component.shutdown()
 
-        def on_shutdown(result):
+    @defer.inlineCallbacks
-            component._ComponentRegistry.components = {}
-        return component.shutdown().addCallback(on_shutdown)
-
-    @pytest.mark.todo
     def test_client_totals(self):
-        StatsTestCase.test_client_totals.im_func.todo = "To be fixed"
+        plugins = yield client.core.get_available_plugins()
+        if "Stats" not in plugins:
+            raise unittest.SkipTest("WebUi plugin not available for testing")
 
-        def callback(args):
+        totals = yield client.stats.get_totals()
-            print_totals(args)
+        self.assertEquals(totals['total_upload'], 0)
-        d = client.stats.get_totals()
+        self.assertEquals(totals['total_payload_upload'], 0)
-        d.addCallback(callback)
+        self.assertEquals(totals['total_payload_download'], 0)
+        self.assertEquals(totals['total_download'], 0)
+        # print_totals(totals)
 
-    @pytest.mark.todo
+    @defer.inlineCallbacks
     def test_session_totals(self):
-        StatsTestCase.test_session_totals.im_func.todo = "To be fixed"
+        plugins = yield client.core.get_available_plugins()
+        if "Stats" not in plugins:
+            raise unittest.SkipTest("WebUi plugin not available for testing")
 
-        def callback(args):
+        totals = yield client.stats.get_session_totals()
-            print_totals(args)
+        self.assertEquals(totals['total_upload'], 0)
-        d = client.stats.get_session_totals()
+        self.assertEquals(totals['total_payload_upload'], 0)
-        d.addCallback(callback)
+        self.assertEquals(totals['total_payload_download'], 0)
+        self.assertEquals(totals['total_download'], 0)
+        # print_totals(totals)

deluge/plugins/WebUi/deluge/plugins/webui/core.py

@@ -13,6 +13,10 @@
 
 import logging
 
+from twisted.internet import defer
+from twisted.internet.error import CannotListenError
+
+import deluge.component as component
 from deluge import configmanager
 from deluge.core.rpcserver import export
 from deluge.plugins.pluginbase import CorePluginBase
@@ -27,28 +31,21 @@ DEFAULT_PREFS = {
 
 
 class Core(CorePluginBase):
+    server = None
 
     def enable(self):
         self.config = configmanager.ConfigManager("web_plugin.conf", DEFAULT_PREFS)
-        self.server = None
         if self.config['enabled']:
-            self.start()
+            self.start_server()
 
     def disable(self):
-        if self.server:
+        self.stop_server()
-            self.server.stop()
 
     def update(self):
         pass
 
-    def restart(self):
+    def _on_stop(self, *args):
-        if self.server:
+        return self.start_server()
-            self.server.stop().addCallback(self.on_stop)
-        else:
-            self.start()
-
-    def on_stop(self, *args):
-        self.start()
 
     @export
     def got_deluge_web(self):
@@ -59,25 +56,34 @@ class Core(CorePluginBase):
         except ImportError:
             return False
 
-    @export
+    def start_server(self):
-    def start(self):
         if not self.server:
             try:
                 from deluge.ui.web import server
             except ImportError:
                 return False
 
-            self.server = server.DelugeWeb()
+            try:
+                self.server = component.get("DelugeWeb")
+            except KeyError:
+                self.server = server.DelugeWeb()
 
         self.server.port = self.config["port"]
         self.server.https = self.config["ssl"]
-        self.server.start(standalone=False)
+        try:
+            self.server.start(standalone=False)
+        except CannotListenError as ex:
+            log.warn("Failed to start WebUI server: %s", ex)
+            raise
         return True
 
-    @export
+    def stop_server(self):
-    def stop(self):
         if self.server:
-            self.server.stop()
+            return self.server.stop()
+        return defer.succeed(True)
+
+    def restart_server(self):
+        return self.stop_server().addCallback(self._on_stop)
 
     @export
     def set_config(self, config):
@@ -97,11 +103,11 @@ class Core(CorePluginBase):
         self.config.save()
 
         if action == 'start':
-            return self.start()
+            return self.start_server()
         elif action == 'stop':
-            return self.stop()
+            return self.stop_server()
         elif action == 'restart':
-            return self.restart()
+            return self.restart_server()
 
     @export
     def get_config(self):

deluge/plugins/WebUi/deluge/plugins/webui/tests/test_plugin_webui.py

@@ -0,0 +1,48 @@
+# -*- coding: utf-8 -*-
+#
+# Copyright (C) 2016 bendikro <bro.devel+deluge@gmail.com>
+#
+# This file is part of Deluge and is licensed under GNU General Public License 3.0, or later, with
+# the additional special exception to link portions of this program with the OpenSSL library.
+# See LICENSE for more details.
+#
+
+from twisted.trial import unittest
+
+import deluge.component as component
+from deluge.core.core import Core
+from deluge.core.rpcserver import RPCServer
+from deluge.tests import common
+from deluge.tests.basetest import BaseTestCase
+
+common.disable_new_release_check()
+
+
+class WebUIPluginTestCase(BaseTestCase):
+
+    def set_up(self):
+        common.set_tmp_config_dir()
+        self.rpcserver = RPCServer(listen=False)
+        self.core = Core()
+        return component.start()
+
+    def tear_down(self):
+
+        def on_shutdown(result):
+            del self.rpcserver
+            del self.core
+        return component.shutdown().addCallback(on_shutdown)
+
+    def test_enable_webui(self):
+        if "WebUi" not in self.core.get_available_plugins():
+            raise unittest.SkipTest("WebUi plugin not available for testing")
+
+        d = self.core.enable_plugin("WebUi")
+
+        def result_cb(result):
+            if "WebUi" not in self.core.get_enabled_plugins():
+                self.fail("Failed to enable WebUi plugin")
+            self.assertTrue(result)
+
+        d.addBoth(result_cb)
+        return d

deluge/plugins/init.py

@@ -22,15 +22,7 @@ class PluginInitBase(object):
         self.plugin = self._plugin_cls(plugin_name)
 
     def enable(self):
-        try:
+        return self.plugin.enable()
-            self.plugin.enable()
-        except Exception as ex:
-            log.error("Unable to enable plugin \"%s\"!", self.plugin._component_name)
-            log.exception(ex)
 
     def disable(self):
-        try:
+        return self.plugin.disable()
-            self.plugin.disable()
-        except Exception as ex:
-            log.error("Unable to disable plugin \"%s\"!", self.plugin._component_name)
-            log.exception(ex)

deluge/tests/test_alertmanager.py

@@ -1,30 +1,31 @@
-from twisted.trial import unittest
+# -*- coding: utf-8 -*-
+#
+# This file is part of Deluge and is licensed under GNU General Public License 3.0, or later, with
+# the additional special exception to link portions of this program with the OpenSSL library.
+# See LICENSE for more details.
+#
 
 import deluge.component as component
 from deluge.core.core import Core
 
+from .basetest import BaseTestCase
 
-class AlertManagerTestCase(unittest.TestCase):
+
-    def setUp(self):  # NOQA
+class AlertManagerTestCase(BaseTestCase):
+
+    def set_up(self):
         self.core = Core()
-
         self.am = component.get("AlertManager")
-        component.start(["AlertManager"])
+        return component.start(["AlertManager"])
 
-    def tearDown(self):  # NOQA
+    def tear_down(self):
-        def on_shutdown(result):
+        return component.shutdown()
-            component._ComponentRegistry.components = {}
-            del self.am
-            del self.core
-
-        return component.shutdown().addCallback(on_shutdown)
 
     def test_register_handler(self):
         def handler(alert):
             return
 
         self.am.register_handler("dummy_alert", handler)
-
         self.assertEquals(self.am.handlers["dummy_alert"], [handler])
 
     def test_deregister_handler(self):

deluge/ui/client.py

@@ -588,7 +588,7 @@ class Client(object):
         if self.is_classicmode():
             self._daemon_proxy.disconnect()
             self.stop_classic_mode()
-            return
+            return defer.succeed(True)
 
         if self._daemon_proxy:
             return self._daemon_proxy.disconnect()

deluge/ui/gtkui/pluginmanager.py

@@ -66,7 +66,11 @@ class PluginManager(deluge.pluginmanagerbase.PluginManagerBase, component.Compon
             self.enable_plugin(plugin)
 
     def _on_plugin_enabled_event(self, name):
-        self.enable_plugin(name)
+        try:
+            self.enable_plugin(name)
+        except Exception as ex:
+            log.warn("Failed to enable plugin '%s': ex: %s", name, ex)
+
         self.run_on_show_prefs()
 
     def _on_plugin_disabled_event(self, name):

deluge/ui/gtkui/preferences.py

@@ -929,15 +929,22 @@ class Preferences(component.Component):
         client.force_call()
 
     def on_plugin_toggled(self, renderer, path):
-        log.debug("on_plugin_toggled")
         row = self.plugin_liststore.get_iter_from_string(path)
         name = self.plugin_liststore.get_value(row, 0)
         value = self.plugin_liststore.get_value(row, 1)
+        log.debug("on_plugin_toggled - %s: %s", name, value)
         self.plugin_liststore.set_value(row, 1, not value)
         if not value:
-            client.core.enable_plugin(name)
+            d = client.core.enable_plugin(name)
         else:
-            client.core.disable_plugin(name)
+            d = client.core.disable_plugin(name)
+
+        def on_plugin_action(arg):
+            if not value and arg is False:
+                log.warn("Failed to enable plugin '%s'",  name)
+                self.plugin_liststore.set_value(row, 1, False)
+
+        d.addBoth(on_plugin_action)
 
     def on_plugin_selection_changed(self, treeselection):
         log.debug("on_plugin_selection_changed")

deluge/ui/web/json_api.py

@@ -392,13 +392,14 @@ class WebApi(JSONComponent):
             default = component.get("DelugeWeb").config["default_daemon"]
             host = component.get("Web")._get_host(default)
             if host:
-                self._connect_daemon(*host[1:])
+                return self._connect_daemon(*host[1:])
             else:
-                self._connect_daemon()
+                return self._connect_daemon()
+        return defer.succeed(True)
 
     def _on_client_disconnect(self, *args):
         component.get("Web.PluginManager").stop()
-        self.stop()
+        return self.stop()
 
     def _get_host(self, host_id):
         """
@@ -415,11 +416,12 @@ class WebApi(JSONComponent):
 
     def start(self):
         self.core_config.start()
-        self.sessionproxy.start()
+        return self.sessionproxy.start()
 
     def stop(self):
         self.core_config.stop()
         self.sessionproxy.stop()
+        return defer.succeed(True)
 
     def _connect_daemon(self, host="localhost", port=58846, username="", password=""):
         """

deluge/ui/web/server.py

@@ -577,12 +577,13 @@ class DelugeWeb(component.Component):
         Args:
             standalone (bool): Whether the server runs as a standalone process
                                If standalone, start twisted reactor.
-
-        Returns:
-            Deferred
         """
-        log.info("%s %s.", _("Starting server in PID"), os.getpid())
+        if self.socket:
+            log.warn("DelugeWeb is already running and cannot be started")
+            return
+
         self.standalone = standalone
+        log.info("Starting webui server at PID %s", os.getpid())
         if self.https:
             self.start_ssl()
         else:
@@ -629,7 +630,7 @@ class DelugeWeb(component.Component):
 
     def shutdown(self, *args):
         self.stop()
-        if self.standalone:
+        if self.standalone and reactor.running:
             reactor.stop()
 
 

tox.ini

@@ -61,7 +61,9 @@ whitelist_externals = trial
 commands = trial --reporter=deluge-reporter deluge/tests
 
 [testenv:plugins]
-commands = py.test deluge/plugins
+commands =
+    python setup.py egg_info_plugins
+    py.test deluge/plugins
 
 [testenv:py26]
 basepython = python2.6