Skip to content

Commit

Permalink
Improve error handling of plugin loading/unloading
Browse files Browse the repository at this point in the history
  • Loading branch information
@johnmaguire
johnmaguire committed Dec 30, 2015
1 parent bddb12e commit 3e7c13b
Show file tree
Hide file tree
Showing 2 changed files with 113 additions and 71 deletions.
157 changes: 97 additions & 60 deletions cardinal/plugins.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,10 @@ def _close_plugin_instance(self, plugin):
Raises:
PluginError -- When a plugin's close function has more than one
argument.
Returns:
Deferred -- A deferred returned by the plugin, or a generated
one.
"""

instance = self.plugins[plugin]['instance']
Expand All @@ -216,11 +220,16 @@ def _close_plugin_instance(self, plugin):
)

if len(argspec.args) == 1:
return instance.close()
# Returns a Deferred regardless of whether instance.close
# returns one. It will be a defer.succeed(return) or
# defer.fail(exception)
return defer.maybeDeferred(instance.close)
elif len(argspec.args) == 2:
return instance.close(self.cardinal)
return defer.maybeDeferred(instance.close, self.cardinal)
else:
raise PluginError("Unknown arguments for close function")
return defer.fail(
PluginError("Unknown arguments for close function")
)

def _load_plugin_config(self, plugin):
"""Loads a JSON or YAML config for a given plugin
Expand Down Expand Up @@ -342,6 +351,23 @@ def itercommands(self, channel=None):
for command in plugin['commands']:
yield command

def log_failure(self, message):
def errback(failure):
try:
failure.raiseException()
except Exception:
self.logger.exception(
message
)

return failure

return errback

@staticmethod
def ignore_failure(failure):
return None

def load(self, plugins):
"""Takes either a plugin name or a list of plugins and loads them.
Expand All @@ -353,7 +379,8 @@ def load(self, plugins):
plugins -- This can be either a single or list of plugin names.
Returns:
list -- A list of failed plugins, or an empty list.
defer.DeferredList -- A DeferredList representing each of the plugins
we are loading.
Raises:
TypeError -- When the `plugins` argument is not a string or list.
Expand All @@ -372,41 +399,45 @@ def load(self, plugins):
deferreds = []

for plugin in plugins:
# Reload flag so we can update the reload counter if necessary
self.logger.info("Attempting to load plugin: %s" % plugin)

# Toggle this to True if we reload
reload_flag = False

self.logger.info("Attempting to load plugin: %s" % plugin)
# Create a new Deferred for loading our plugin
d = defer.succeed(plugin)

# Import each plugin's module with our own hacky function to reload
# modules that have already been imported previously
try:
d = None
if plugin in self.plugins.keys():
self.logger.info("Already loaded, reloading: %s" % plugin)
reload_flag = True

d = defer.maybeDeferred(self._close_plugin_instance, plugin)
module_to_import = self.plugins[plugin]['module']

# Make sure we attempt to reload even when unloading
# didn't go smoothly
def succeed(v): return True
d.addErrback(succeed)
else:
d = defer.succeed(plugin)
module_to_import = plugin

module = self._import_module(module_to_import)
except Exception:
# Probably a syntax error in the plugin, log the exception
self.logger.exception(
"Could not load plugin module: %s" % plugin
)
d = defer.fail(PluginError(plugin))
# If the plugin is loaded, close it before loading it again
if plugin in self.plugins.keys():
reload_flag = True

continue
self.logger.info("Already loaded, reloading: %s" % plugin)

# Attempt to close the plugin instance first
d.addCallback(self._close_plugin_instance)

def load_plugin(_):
# Log and ignore failures closing plugin
d.addErrback(self.log_failure(
"Didn't close plugin cleanly: %s" % plugin
))
d.addErrback(self.ignore_failure)

# And use the existing module object for our _import_module()
# call below.
def return_plugin_module(_):
return self.plugins[plugin]['module']
d.addCallback(return_plugin_module)

# Now really import/reload the module
d.addCallback(self._import_module)
d.addErrback(self.log_failure(
"Could not load plugin module: %s" % plugin
))

# We'll run this as long as the module imports successfully. It
# returns the plugin name so that when looping over the list of
# Deferreds you can tell which plugins succeeded/failed to load.
def load_plugin(module):
# Attempt to load the config file for the given plugin.
config = None
try:
Expand All @@ -428,7 +459,6 @@ def load_plugin(_):
self.logger.exception(
"Could not instantiate plugin: %s" % plugin
)

raise

commands = self._get_plugin_commands(instance)
Expand All @@ -446,9 +476,18 @@ def load_plugin(_):
self.cardinal.reloads += 1

self.logger.info("Plugin %s successfully loaded" % plugin)
return plugin

# Returns success state and plugin name
return (True, plugin)

# Convert any uncaught Failures at this point into a value that can
# be parsed to show failure loading
def return_load_error(_):
return (False, plugin)

d.addCallback(load_plugin)
d.addErrback(return_load_error)

deferreds.append(d)

return defer.DeferredList(deferreds)
Expand All @@ -464,7 +503,8 @@ def unload(self, plugins, keep_entry=False):
keep_entry -- Do not remove the plugin from self.plugins
Returns:
list -- A list of failed plugins, or an empty list.
defer.DeferredList -- A DeferredList representing each of the plugins
we are unloading.
Raises:
TypeError -- When the `plugins` argument is not a string or list.
Expand All @@ -482,36 +522,33 @@ def unload(self, plugins, keep_entry=False):

for plugin in plugins:
self.logger.info("Attempting to unload plugin: %s" % plugin)

if plugin not in self.plugins:
self.logger.warning("Plugin was never loaded: %s" % plugin)
deferreds.append(defer.fail(PluginError(plugin)))

# Don't errback, but return error state and plugin name
deferreds.append(defer.succeed((False, plugin)))
continue

try:
# Plugin may need to close asynchronously
d = defer.maybeDeferred(self._close_plugin_instance, plugin)
except Exception:
# Log the exception that came from trying to unload the
# plugin, but don't skip over the plugin. We'll still
# unload it.
self.logger.exception(
"Didn't close plugin cleanly: %s" % plugin
)
# Plugin may need to close asynchronously
d = defer.maybeDeferred(self._close_plugin_instance, plugin)

# Log and ignore failures closing plugin
d.addErrback(self.log_failure(
"Didn't close plugin cleanly: %s" % plugin
))
d.addErrback(self.ignore_failure)

# Spawn a failed deferred instead of using the maybeDeferred
# above in case the Exception prevents the plugin from ever
# calling the deferred (or makes it succeed on accident)
d = defer.fail(PluginError(plugin))
finally:
deferreds.append(d)
# Once all references of the plugin have been removed, Python will
# eventually do garbage collection. We only saved it in one
# location, so we'll get rid of that now.
del self.plugins[plugin]

# Once all references of the plugin have been removed, Python will
# eventually do garbage collection. We only opened it in one
# location, so we'll get rid of that now.
del self.plugins[plugin]
def return_unload_success(_):
return (True, plugin)
d.addCallback(return_unload_success)

deferreds.append(d)

# Return a DeferredList so all of the deferreds can be waited on together
return defer.DeferredList(deferreds)

def unload_all(self):
Expand Down
27 changes: 16 additions & 11 deletions plugins/admin/plugin.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,18 +77,21 @@ def load_plugins(self, cardinal, user, channel, msg):
plugins.append(plugin['name'])

deferred = cardinal.plugin_manager.load(plugins)

def handle_results(plugins):
states = {True: [], False: []}
for success, plugin in plugins:
for _, (success, plugin) in plugins:
states[success].append(plugin)

This comment has been minimized.

Sign in to view

Copy link
@s-ol

s-ol Dec 30, 2015

Contributor

Instead of building a second success state, I would rather have the callback be a failure in both cases (plugin failed to load, plugin not known before) with a different exception, and then sort them by that exception type to print them.

This comment has been minimized.

Sign in to view

Copy link
@johnmaguire

johnmaguire Dec 30, 2015

Author Owner

I was running into issues getting it to work for a couple reasons.

Twisted continually threw Unhandled errors in my face if the errbacks weren't switched back into callbacks, which meant the client code (the admin plugin in this case, and also in __init__) was going to have to be responsible for handling the errbacks, which I thought was kind of annoying, and I wanted to remove. I considered it a "formatting" step. I wasn't entirely happy with it. I'll revisit it tomorrow and see if I can make it work.

This comment has been minimized.

Sign in to view

Copy link
@s-ol

s-ol Dec 30, 2015

Contributor

I just tried on my own, the problem you had was that the DeferredLists weren't created with consumeErrors=True which makes them catch Failures instead of re-raising them in the callback. I'll push to my branch (the other PR) once I tested it completely.
if len(states[True]) > 0:
cardinal.sendMsg(channel, "Plugins loaded succesfully: %s." %
', '.join(sorted(states[True])))
cardinal.sendMsg(channel,
"Plugins loaded succesfully: %s." %
', '.join(sorted(states[True])))

if len(states[False]) > 0:
cardinal.sendMsg(channel, "Plugins failed to load: %s." %
', '.join(sorted(states[False])))
cardinal.sendMsg(channel,
"Plugins failed to load: %s." %
', '.join(sorted(states[False])))

deferred.addCallback(handle_results)

Expand Down Expand Up @@ -117,16 +120,18 @@ def unload_plugins(self, cardinal, user, channel, msg):
deferred = cardinal.plugin_manager.unload(plugins)
def handle_results(plugins):
states = {True: [], False: []}
for success, plugin in plugins:
for _, (success, plugin) in plugins:
states[success].append(plugin)

if len(states[True]) > 0:
cardinal.sendMsg(channel, "Plugins unloaded succesfully: %s." %
', '.join(sorted(states[True])))
cardinal.sendMsg(channel,
"Plugins unloaded succesfully: %s." %
', '.join(sorted(states[True])))

if len(states[False]) > 0:
cardinal.sendMsg(channel, "Unknown plugins: %s." %
', '.join(sorted(states[False])))
cardinal.sendMsg(channel,
"Unknown plugins: %s." %
', '.join(sorted(states[False])))

deferred.addCallback(handle_results)

Expand Down Expand Up @@ -185,7 +190,7 @@ def enable_plugins(self, cardinal, user, channel, msg):
cardinal.sendMsg("Plugin %s does not exist" % plugin)

successful = [
channel for channel in channels if channel not in not_blacklisted
_channel for _channel in channels if _channel not in not_blacklisted
]

if len(successful) > 0:
Expand Down

0 comments on commit 3e7c13b

Please sign in to comment.