Skip to content

Commit

Permalink
Make plugin loading mechanism rely on exceptions
Browse files Browse the repository at this point in the history
  • Loading branch information
S0lll0s committed Dec 30, 2015
1 parent 2527599 commit afed1bb
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 50 deletions.
56 changes: 26 additions & 30 deletions cardinal/plugins.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -228,9 +228,7 @@ def _close_plugin_instance(self, plugin):
elif len(argspec.args) == 2:
return defer.maybeDeferred(instance.close, self.cardinal)
else:
return defer.fail(
PluginError("Unknown arguments for close function")
)
raise PluginError("Unknown arguments for close function", plugin)

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

def _log_failure(self, message):
def _log_failure(self, message, plugin):
def errback(failure):
try:
failure.raiseException()
except Exception:
self.logger.exception(
message
)
self.logger.exception(
"%s: %s" % (message, plugin)
)
self.logger.exception(failure.value)

This comment has been minimized.

Sign in to view

Copy link
@s-ol

s-ol Dec 30, 2015

Contributor

line 359 is there so plugin dev's can see the actual error in the log, but I would like to somehow present the stack trace instead (possibly only show this in a debug mode at all?)

This comment has been minimized.

Sign in to view

Copy link
@s-ol

s-ol Dec 30, 2015

Contributor

perhaps useful: http://www.bbarrows.com/blog/2012/09/24/implementing-exception-logging-in-python/

This comment has been minimized.

Sign in to view

Copy link
@johnmaguire

johnmaguire Dec 30, 2015

Owner

self.logger.exception() should pass the exception info to the logger. I'm not sure if I was getting tracebacks or just the specific line that errored, including a code snippet, but it seemed good to me. If you're not getting it anymore, it's because of the removal of the try: failure.raiseException() that I did, to ensure that info was passed.

If you mean you want to show the full extended stack trace, I don't have a good answer. Maybe I'm misunderstanding the problem.

This comment has been minimized.

Sign in to view

Copy link
@s-ol

s-ol Dec 30, 2015

Contributor

For me it only logged the error string, but not the line or stack trace like this

# make sure we always end up with a PluginError
if failure.type != PluginError:
raise PluginError(message, plugin)

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 Down Expand Up @@ -418,22 +414,21 @@ def load(self, plugins):
# Attempt to close the plugin instance first
d.addCallback(self._close_plugin_instance)

# Log and ignore failures closing plugin
# Log failures closing plugin
d.addErrback(self._log_failure(
"Didn't close plugin cleanly: %s" % plugin
"Didn't close plugin cleanly", 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)
d.addBoth(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
"Could not load plugin module", plugin
))

# We'll run this as long as the module imports successfully. It
Expand Down Expand Up @@ -481,19 +476,21 @@ def load_plugin(module):
self.logger.info("Plugin %s successfully loaded" % plugin)

# Returns success state and plugin name
return (True, plugin)
return 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)
def return_load_error(failure):
if failure.type == PluginError:
raise PluginError(failure.value.args[0], plugin)
raise PluginError(failure.value, plugin)

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

deferreds.append(d)

return defer.DeferredList(deferreds)
return defer.DeferredList(deferreds, consumeErrors=True)

This comment has been minimized.

Sign in to view

Copy link
@johnmaguire

johnmaguire Dec 30, 2015

Owner

I almost used this flag but I couldn't clearly figure out what it does -- does it simply prevent any unhandled errbacks from Deferreds in the list from generating "Unhandled Failure" messages? Does it work for Deferreds already in a failed state prior to being added to the list?

This comment has been minimized.

Sign in to view

Copy link
@s-ol

s-ol Dec 30, 2015

Contributor

it works by adding an errback to the callbacks that are added, so it doesn't matter whether the Callback has been fired yet. Basically, when you insert a deferred into a list without the option, this happens:

Callback Errback note
something raises an exception
\
save state in list save state in list added by DeferredList
UNHANDLED ERROR twisted implicit "call/errbacks"

so what you are supposed to do, to handle errors is add an errback after adding the Deferred to the list and handling the error there. If you set consumeErrors then the added errBack doesn't pass the Failure on but instead goes back to the callback chain:

Callback Errback note
something raises an exception
\
save state in list save state in list, catch failure added by DeferredList
/
UNHANDLED ERROR twisted implicit "call/errbacks"

(that's my understanding anyways)

This comment has been minimized.

Sign in to view

Copy link
@johnmaguire

johnmaguire Dec 30, 2015

Owner

But if it's putting it back into the callback chain instead of the errback chain, I'm confused as to how DeferredList reports its status as failed. :S

As long as it works though...

This comment has been minimized.

Sign in to view

Copy link
@s-ol

s-ol Dec 30, 2015

Contributor

It saves the status of the deferred as the callback flow passes through the DeferredList, thats what the callback/errbacks it adds are there for normally. What happens afterwards it doesn't care about. Take a look at my diagrams again, the /s etc are supposed to be an example chain flow.

This comment has been minimized.

Sign in to view

Copy link
@johnmaguire

johnmaguire Dec 30, 2015

Owner

I understand that, I guess I'm more confused about whether state can ever change again. The flow diagram looks like after saving the state, and catching failure, it switches back to the callback chain. Does that mean if another callback occurs, it'll be put back to the successful state within the DeferredList?

i.e.

d1 = defer.fail(PluginError("Some error", plugin))
d2 = defer.succeed(plugin)
dl = defer.DeferredList([d1, d2], consume_errors=True)
dl.addCallback(some_callback)

If I'm understanding correctly, consume_errors=True here is putting both d1 and d2 back into the callback chain (so d2 is no longer processing errbacks).

  1. If .addCallback() were applied to d2, would DeferredList notice, and change the state for d2 back to success?
  2. If .addCallback() were applied to dl (the list), would the state be changed? Or does that callback for the DeferredList not change the state of any child Deferreds?

This comment has been minimized.

Sign in to view

Copy link
@s-ol

s-ol Dec 30, 2015

Contributor

no, the callback chains just go on without notifying the DeferredList as far as I know.

This comment has been minimized.

Sign in to view

Copy link
@johnmaguire

johnmaguire Dec 30, 2015

Owner

So the consume_errors=True flag only matters at the time that the DeferredList object is created? And the states only represent the state as of the time that the DeferredList was created?

This comment has been minimized.

Sign in to view

Copy link
@s-ol

s-ol Dec 31, 2015

Contributor

almost, the states represent the callback chain each deferred was in at the time of being added to the list.

def unload(self, plugins, keep_entry=False):
"""Takes either a plugin name or a list of plugins and unloads them.
Expand Down Expand Up @@ -529,30 +526,29 @@ def unload(self, plugins, keep_entry=False):
self.logger.warning("Plugin was never loaded: %s" % plugin)

# Don't errback, but return error state and plugin name
deferreds.append(defer.succeed((False, plugin)))
deferreds.append(defer.fail(PluginError("Plugin was never loaded", plugin)))

This comment has been minimized.

Sign in to view

Copy link
@johnmaguire

johnmaguire Dec 30, 2015

Owner

I definitely like this better. I really didn't like converting all the errors into success-state Deferreds. The comment here needs an update at least.

This comment has been minimized.

Sign in to view

Copy link
@s-ol

s-ol Dec 30, 2015

Contributor

Oh yeah, I kind of suck at documenting code (or keeping it documented).

This comment has been minimized.

Sign in to view

Copy link
@johnmaguire

johnmaguire Dec 30, 2015

Owner

And I overdocument, no worries ;)
continue

# 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
"Didn't close plugin cleanly", plugin
))
d.addErrback(self._ignore_failure)

# 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]

def return_unload_success(_):
return (True, plugin)
d.addCallback(return_unload_success)
def return_plugin(_):
return plugin
d.addCallback(return_plugin)

deferreds.append(d)

return defer.DeferredList(deferreds)
return defer.DeferredList(deferreds, consumeErrors=True)

def unload_all(self):
"""Unloads all loaded plugins.
Expand Down
52 changes: 32 additions & 20 deletions plugins/admin/plugin.py
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from collections import defaultdict

class AdminPlugin(object):
# A dictionary which will contain the owner nicks and vhosts
owners = None
Expand Down Expand Up @@ -79,19 +81,24 @@ def load_plugins(self, cardinal, user, channel, msg):
deferred = cardinal.plugin_manager.load(plugins)

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

if len(states[True]) > 0:
successes = []
failures = defaultdict(list)
for success, plugin in plugins:
if success:
successes.append(plugin)
else:
failures[plugin.value.args[0]].append(plugin.value.args[1])

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

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

This comment has been minimized.

Sign in to view

Copy link
@johnmaguire

johnmaguire Dec 30, 2015

Owner

This is kinda neat, but I wanna think about it. I don't want Cardinal to do too much line-spam, and there's already three lines printed for a reload that had both successful unsuccessful loads. This could cause it to increase, and since the information provided by doing this can be found in the log output anyway, I'm not sure there's a good reason to expose it to the IRC buffer.

This comment has been minimized.

Sign in to view

Copy link
@s-ol

s-ol Dec 30, 2015

Contributor

yeah, this was more about showing off that that info is now available but it doesn't make much sense to the users in most cases I guess. Although these functions are only going to be used by admins (and rarely) anyway.

This comment has been minimized.

Sign in to view

Copy link
@johnmaguire

johnmaguire Dec 30, 2015

Owner

They should only be used by admins, true, but they're also usable in a channel. Restricting to PM is an option too, but I sorta like the flexibility (and personally usually call .reload in a channel, usually as a semi-signal that I've fixed something). But like you said, since they're only going to be used by admins, I sort of assumed they'd have access to the logs. And if they don't, they also don't have access to fix the problem.
cardinal.sendMsg(channel, " %s (%s)" %
(', '.join(sorted(failures[reason])), reason ))

deferred.addCallback(handle_results)

Expand Down Expand Up @@ -119,19 +126,24 @@ def unload_plugins(self, cardinal, user, channel, msg):
# Returns a list of plugins that weren't loaded to begin with
deferred = cardinal.plugin_manager.unload(plugins)
def handle_results(plugins):
states = {True: [], False: []}
for _, (success, plugin) in plugins:
states[success].append(plugin)

if len(states[True]) > 0:
successes = []
failures = defaultdict(list)
for success, plugin in plugins:
if success:
successes.append(plugin)
else:
failures[plugin.value.args[0]].append(plugin.value.args[1])

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

if len(states[False]) > 0:
cardinal.sendMsg(channel,
"Unknown plugins: %s." %
', '.join(sorted(states[False])))
if len(failures.keys()) > 0:
cardinal.sendMsg(channel, "Plugins failed to unload:")
for reason in failures.keys():
cardinal.sendMsg(channel, " %s (%s)" %
(', '.join(sorted(failures[reason])), reason ))

deferred.addCallback(handle_results)

Expand Down

0 comments on commit afed1bb

Please sign in to comment.