diff --git a/common/utils.h b/common/utils.h index e1471c34d066..fa63c6c881a6 100644 --- a/common/utils.h +++ b/common/utils.h @@ -47,9 +47,9 @@ void clear_softref_(const tal_t *outer, size_t outersize, void **ptr); /* Note: p is never a complex expression, otherwise this multi-evaluates! */ #define tal_arr_expand(p, s) \ do { \ - size_t n = tal_count(*(p)); \ - tal_resize((p), n+1); \ - (*(p))[n] = (s); \ + size_t n_ = tal_count(*(p)); \ + tal_resize((p), n_+1); \ + (*(p))[n_] = (s); \ } while(0) /** diff --git a/contrib/pyln-client/pyln/client/plugin.py b/contrib/pyln-client/pyln/client/plugin.py index 4be219f4ea90..3678bb90b4c4 100644 --- a/contrib/pyln-client/pyln/client/plugin.py +++ b/contrib/pyln-client/pyln/client/plugin.py @@ -57,6 +57,8 @@ def __init__(self, name: str, func: Callable[..., JSONType], self.desc = desc self.long_desc = long_desc self.deprecated = deprecated + self.before: List[str] = [] + self.after: List[str] = [] class Request(dict): @@ -405,7 +407,9 @@ def decorator(f: Callable[..., JSONType]) -> Callable[..., JSONType]: return decorator def add_hook(self, name: str, func: Callable[..., JSONType], - background: bool = False) -> None: + background: bool = False, + before: Optional[List[str]] = None, + after: Optional[List[str]] = None) -> None: """Register a hook that is called synchronously by lightningd on events """ if name in self.methods: @@ -427,15 +431,23 @@ def add_hook(self, name: str, func: Callable[..., JSONType], method = Method(name, func, MethodType.HOOK) method.background = background + method.before = [] + if before: + method.before = before + method.after = [] + if after: + method.after = after self.methods[name] = method - def hook(self, method_name: str) -> JsonDecoratorType: + def hook(self, method_name: str, + before: List[str] = None, + after: List[str] = None) -> JsonDecoratorType: """Decorator to add a plugin hook to the dispatch table. Internally uses add_hook. """ def decorator(f: Callable[..., JSONType]) -> Callable[..., JSONType]: - self.add_hook(method_name, f, background=False) + self.add_hook(method_name, f, background=False, before=before, after=after) return f return decorator @@ -689,7 +701,9 @@ def _getmanifest(self, **kwargs) -> JSONType: continue if method.mtype == MethodType.HOOK: - hooks.append(method.name) + hooks.append({'name': method.name, + 'before': method.before, + 'after': method.after}) continue doc = inspect.getdoc(method.func) diff --git a/doc/PLUGINS.md b/doc/PLUGINS.md index c40e9d6a7bcc..33792db94e3c 100644 --- a/doc/PLUGINS.md +++ b/doc/PLUGINS.md @@ -93,8 +93,8 @@ example: "disconnect" ], "hooks": [ - "openchannel", - "htlc_accepted" + { "name": "openchannel", "before": ["another_plugin"] }, + { "name": "htlc_accepted" } ], "features": { "node": "D0000000", @@ -678,14 +678,25 @@ declares that it'd like to be consulted on what to do next for certain events in the daemon. A hook can then decide how `lightningd` should react to the given event. +When hooks are registered, they can optionally specify "before" and +"after" arrays of plugin names, which control what order they will be +called in. If a plugin name is unknown, it is ignored, otherwise if the +hook calls cannot be ordered to satisfy the specifications of all +plugin hooks, the plugin registration will fail. + The call semantics of the hooks, i.e., when and how hooks are called, depend on the hook type. Most hooks are currently set to `single`-mode. In this mode only a single plugin can register the hook, and that plugin will get called for each event of that type. If a second plugin attempts to register the hook -it gets killed and a corresponding log entry will be added to the logs. In -`chain`-mode multiple plugins can register for the hook type and they are -called sequentially if a matching event is triggered. Each plugin can then -handle the event or defer by returning a `continue` result like the following: +it gets killed and a corresponding log entry will be added to the logs. + +In `chain`-mode multiple plugins can register for the hook type and +they are called in any order they are loaded (i.e. cmdline order +first, configuration order file second: though note that the order of +plugin directories is implementation-dependent), overriden only by +`before` and `after` requirements the plugin's hook registrations specify. +Each plugin can then handle the event or defer by returning a +`continue` result like the following: ```json { diff --git a/doc/lightningd-config.5 b/doc/lightningd-config.5 index 61e0af300f9a..61708660ddc6 100644 --- a/doc/lightningd-config.5 +++ b/doc/lightningd-config.5 @@ -519,14 +519,17 @@ additional paths too: \fBplugin\fR=\fIPATH\fR Specify a plugin to run as part of c-lightning\. This can be specified -multiple times to add multiple plugins\. +multiple times to add multiple plugins\. Note that unless plugins themselves +specify ordering requirements for being called on various hooks, plugins will +be ordered by commandline, then config file\. \fBplugin-dir\fR=\fIDIRECTORY\fR Specify a directory to look for plugins; all executable files not containing punctuation (other than \fI\.\fR, \fI-\fR or \fI_) in 'DIRECTORY\fR are loaded\. \fIDIRECTORY\fR must exist; this can be specified multiple times to -add multiple directories\. +add multiple directories\. The ordering of plugins within a directory +is currently unspecified\. \fBclear-plugins\fR @@ -581,4 +584,4 @@ Main web site: \fIhttps://github.com/ElementsProject/lightning\fR Note: the modules in the ccan/ directory have their own licenses, but the rest of the code is covered by the BSD-style MIT license\. -\" SHA256STAMP:6b275a3227db6565ad3c5369b95d3eefe9472864b8ed9e6c5c768981cdf23b8f +\" SHA256STAMP:c927bd3afb61288bb67d941d113cdeefe1363b0c7a28936ef30d43af3ce66098 diff --git a/doc/lightningd-config.5.md b/doc/lightningd-config.5.md index 91aebbc9706a..8bd4fc591c8d 100644 --- a/doc/lightningd-config.5.md +++ b/doc/lightningd-config.5.md @@ -427,13 +427,16 @@ additional paths too: **plugin**=*PATH* Specify a plugin to run as part of c-lightning. This can be specified -multiple times to add multiple plugins. +multiple times to add multiple plugins. Note that unless plugins themselves +specify ordering requirements for being called on various hooks, plugins will +be ordered by commandline, then config file. **plugin-dir**=*DIRECTORY* Specify a directory to look for plugins; all executable files not containing punctuation (other than *.*, *-* or *\_) in 'DIRECTORY* are loaded. *DIRECTORY* must exist; this can be specified multiple times to -add multiple directories. +add multiple directories. The ordering of plugins within a directory +is currently unspecified. **clear-plugins** This option clears all *plugin*, *important-plugin*, and *plugin-dir* options diff --git a/lightningd/plugin.c b/lightningd/plugin.c index 679b59d562a0..370ee63b083d 100644 --- a/lightningd/plugin.c +++ b/lightningd/plugin.c @@ -58,6 +58,7 @@ struct plugins *plugins_new(const tal_t *ctx, struct log_book *log_book, p->json_cmds = tal_arr(p, struct command *, 0); p->blacklist = tal_arr(p, const char *, 0); p->shutdown = false; + p->plugin_idx = 0; #if DEVELOPER p->dev_builtin_plugins_unimportant = false; #endif /* DEVELOPER */ @@ -83,24 +84,46 @@ void plugins_free(struct plugins *plugins) tal_free(plugins); } -static void check_plugins_resolved(struct plugins *plugins) +/* Once they've all replied with their manifests, we can order them. */ +static void check_plugins_manifests(struct plugins *plugins) { - /* As startup, we break out once all getmanifest are returned */ - if (plugins->startup) { - if (!plugins_any_in_state(plugins, AWAITING_GETMANIFEST_RESPONSE)) - io_break(plugins); - /* Otherwise we wait until all finished. */ - } else if (plugins_all_in_state(plugins, INIT_COMPLETE)) { - struct command **json_cmds; - - /* Clear commands first, in case callbacks add new ones. - * Paranoia, but wouldn't that be a nasty bug to find? */ - json_cmds = plugins->json_cmds; - plugins->json_cmds = tal_arr(plugins, struct command *, 0); - for (size_t i = 0; i < tal_count(json_cmds); i++) - plugin_cmd_all_complete(plugins, json_cmds[i]); - tal_free(json_cmds); + struct plugin **depfail; + + if (plugins_any_in_state(plugins, AWAITING_GETMANIFEST_RESPONSE)) + return; + + /* Now things are settled, try to order hooks. */ + depfail = plugin_hooks_make_ordered(tmpctx); + for (size_t i = 0; i < tal_count(depfail); i++) { + /* Only complain and free plugins! */ + if (depfail[i]->plugin_state != NEEDS_INIT) + continue; + plugin_kill(depfail[i], + "Cannot meet required hook dependencies"); } + + /* As startup, we break out once all getmanifest are returned */ + if (plugins->startup) + io_break(plugins); + else + /* Otherwise we go straight into configuring them */ + plugins_config(plugins); +} + +static void check_plugins_initted(struct plugins *plugins) +{ + struct command **json_cmds; + + if (!plugins_all_in_state(plugins, INIT_COMPLETE)) + return; + + /* Clear commands first, in case callbacks add new ones. + * Paranoia, but wouldn't that be a nasty bug to find? */ + json_cmds = plugins->json_cmds; + plugins->json_cmds = tal_arr(plugins, struct command *, 0); + for (size_t i = 0; i < tal_count(json_cmds); i++) + plugin_cmd_all_complete(plugins, json_cmds[i]); + tal_free(json_cmds); } struct command_result *plugin_register_all_complete(struct lightningd *ld, @@ -117,7 +140,6 @@ static void destroy_plugin(struct plugin *p) { struct plugin_rpccall *call; - plugin_hook_unregister_all(p); list_del(&p->list); /* Terminate all pending RPC calls with an error. */ @@ -126,10 +148,16 @@ static void destroy_plugin(struct plugin *p) call->cmd, PLUGIN_TERMINATED, "Plugin terminated before replying to RPC call.")); } + /* Reset, so calls below don't try to fail it again! */ + list_head_init(&p->pending_rpccalls); - /* Don't call this if we're still parsing options! */ - if (p->plugin_state != UNCONFIGURED) - check_plugins_resolved(p->plugins); + /* If this was last one manifests were waiting for, handle deps */ + if (p->plugin_state == AWAITING_GETMANIFEST_RESPONSE) + check_plugins_manifests(p->plugins); + + /* If this was the last one init was waiting for, handle cmd replies */ + if (p->plugin_state == AWAITING_INIT_RESPONSE) + check_plugins_initted(p->plugins); /* If we are shutting down, do not continue to checking if * the dying plugin is important. */ @@ -172,6 +200,7 @@ struct plugin *plugin_register(struct plugins *plugins, const char* path TAKES, p->used = 0; p->subscriptions = NULL; p->dynamic = false; + p->index = plugins->plugin_idx++; p->log = new_log(p, plugins->log_book, NULL, "plugin-%s", path_basename(tmpctx, p->cmd)); @@ -1057,20 +1086,46 @@ static const char *plugin_subscriptions_add(struct plugin *plugin, static const char *plugin_hooks_add(struct plugin *plugin, const char *buffer, const jsmntok_t *resulttok) { - const jsmntok_t *hookstok = json_get_member(buffer, resulttok, "hooks"); + const jsmntok_t *t, *hookstok, *beforetok, *aftertok; + size_t i; + + hookstok = json_get_member(buffer, resulttok, "hooks"); if (!hookstok) return NULL; - for (int i = 0; i < hookstok->size; i++) { - char *name = json_strdup(tmpctx, plugin->buffer, - json_get_arr(hookstok, i)); - if (!plugin_hook_register(plugin, name)) { + json_for_each_arr(i, t, hookstok) { + char *name; + struct plugin_hook *hook; + + if (t->type == JSMN_OBJECT) { + const jsmntok_t *nametok; + + nametok = json_get_member(buffer, t, "name"); + if (!nametok) + return tal_fmt(plugin, "no name in hook obj %.*s", + json_tok_full_len(t), + json_tok_full(buffer, t)); + name = json_strdup(tmpctx, buffer, nametok); + beforetok = json_get_member(buffer, t, "before"); + aftertok = json_get_member(buffer, t, "after"); + } else if (deprecated_apis) { + name = json_strdup(tmpctx, plugin->buffer, t); + beforetok = aftertok = NULL; + } else { + return tal_fmt(plugin, + "hooks must be an array of objects"); + } + + hook = plugin_hook_register(plugin, name); + if (!hook) { return tal_fmt(plugin, "could not register hook '%s', either the " "name doesn't exist or another plugin " "already registered it.", name); } + + plugin_hook_add_deps(hook, plugin, buffer, beforetok, aftertok); tal_free(name); } return NULL; @@ -1192,9 +1247,6 @@ bool plugins_all_in_state(const struct plugins *plugins, enum plugin_state state return true; } -/* FIXME: Forward declaration to reduce patch noise */ -static void plugin_config(struct plugin *plugin); - /** * Callback for the plugin_manifest request. */ @@ -1211,20 +1263,13 @@ static void plugin_manifest_cb(const char *buffer, return; } - /* At startup, we want to io_break once all getmanifests are done */ - check_plugins_resolved(plugin->plugins); + /* Reset timer, it'd kill us otherwise. */ + plugin->timeout_timer = tal_free(plugin->timeout_timer); - if (plugin->plugins->startup) { - /* Reset timer, it'd kill us otherwise. */ - plugin->timeout_timer = tal_free(plugin->timeout_timer); - } else { - /* Note: here 60 second timer continues through init */ - /* After startup, automatically call init after getmanifest */ - if (!plugin->dynamic) - plugin_kill(plugin, "Not a dynamic plugin"); - else - plugin_config(plugin); - } + if (!plugin->plugins->startup && !plugin->dynamic) + plugin_kill(plugin, "Not a dynamic plugin"); + else + check_plugins_manifests(plugin->plugins); } /* If this is a valid plugin return full path name, otherwise NULL */ @@ -1329,6 +1374,27 @@ void plugins_add_default_dir(struct plugins *plugins) } } +static void plugin_set_timeout(struct plugin *p) +{ + bool debug = false; + +#if DEVELOPER + if (p->plugins->ld->dev_debug_subprocess + && strends(p->cmd, p->plugins->ld->dev_debug_subprocess)) + debug = true; +#endif + + /* Don't timeout if they're running a debugger. */ + if (debug) + p->timeout_timer = NULL; + else { + p->timeout_timer + = new_reltimer(p->plugins->ld->timers, p, + time_from_sec(PLUGIN_MANIFEST_TIMEOUT), + plugin_manifest_timeout, p); + } +} + const char *plugin_send_getmanifest(struct plugin *p) { char **cmd; @@ -1367,16 +1433,7 @@ const char *plugin_send_getmanifest(struct plugin *p) plugin_request_send(p, req); p->plugin_state = AWAITING_GETMANIFEST_RESPONSE; - /* Don't timeout if they're running a debugger. */ - if (debug) - p->timeout_timer = NULL; - else { - p->timeout_timer - = new_reltimer(p->plugins->ld->timers, p, - time_from_sec(PLUGIN_MANIFEST_TIMEOUT), - plugin_manifest_timeout, p); - } - + plugin_set_timeout(p); return NULL; } @@ -1448,7 +1505,7 @@ static void plugin_config_cb(const char *buffer, plugin_cmd_succeeded(plugin->start_cmd, plugin); plugin->start_cmd = NULL; } - check_plugins_resolved(plugin->plugins); + check_plugins_initted(plugin->plugins); } void @@ -1512,6 +1569,7 @@ plugin_config(struct plugin *plugin) { struct jsonrpc_request *req; + plugin_set_timeout(plugin); req = jsonrpc_request_start(plugin, "init", plugin->log, NULL, plugin_config_cb, plugin); plugin_populate_init_request(plugin, req); diff --git a/lightningd/plugin.h b/lightningd/plugin.h index 4b1a91f25179..070c0184ad8e 100644 --- a/lightningd/plugin.h +++ b/lightningd/plugin.h @@ -50,6 +50,9 @@ struct plugin { enum plugin_state plugin_state; + /* Our unique index, which is default hook ordering. */ + u64 index; + /* If this plugin can be restarted without restarting lightningd */ bool dynamic; @@ -113,6 +116,9 @@ struct plugins { /* Whether we are shutting down (`plugins_free` is called) */ bool shutdown; + /* Index to show what order they were added in */ + u64 plugin_idx; + #if DEVELOPER /* Whether builtin plugins should be overridden as unimportant. */ bool dev_builtin_plugins_unimportant; diff --git a/lightningd/plugin_hook.c b/lightningd/plugin_hook.c index 351d9f8870ed..321cb75fec53 100644 --- a/lightningd/plugin_hook.c +++ b/lightningd/plugin_hook.c @@ -1,3 +1,4 @@ +#include #include #include #include @@ -18,6 +19,14 @@ struct plugin_hook_request { struct lightningd *ld; }; +struct hook_instance { + /* What plugin registered */ + struct plugin *plugin; + + /* Dependencies it asked for. */ + const char **before, **after; +}; + /* A link in the plugin_hook call chain (there's a joke in there about * computer scientists and naming...). The purpose is to act both as a list * from which elements can be popped off as we progress along the chain as @@ -30,12 +39,20 @@ struct plugin_hook_call_link { struct plugin_hook_request *req; }; -static struct plugin_hook *plugin_hook_by_name(const char *name) +static struct plugin_hook **get_hooks(size_t *num) { static struct plugin_hook **hooks = NULL; static size_t num_hooks; if (!hooks) hooks = autodata_get(hooks, &num_hooks); + *num = num_hooks; + return hooks; +} + +static struct plugin_hook *plugin_hook_by_name(const char *name) +{ + size_t num_hooks; + struct plugin_hook **hooks = get_hooks(&num_hooks); for (size_t i=0; iname, name)) @@ -43,61 +60,52 @@ static struct plugin_hook *plugin_hook_by_name(const char *name) return NULL; } -bool plugin_hook_register(struct plugin *plugin, const char *method) +/* When we destroy a plugin, we remove its hooks */ +static void destroy_hook_instance(struct hook_instance *h, + struct plugin_hook *hook) +{ + for (size_t i = 0; i < tal_count(hook->hooks); i++) { + if (h == hook->hooks[i]) { + tal_arr_remove(&hook->hooks, i); + return; + } + } + abort(); +} + +struct plugin_hook *plugin_hook_register(struct plugin *plugin, const char *method) { + struct hook_instance *h; struct plugin_hook *hook = plugin_hook_by_name(method); if (!hook) { /* No such hook name registered */ - return false; + return NULL; } - /* Make sure the plugins array is initialized. */ - if (hook->plugins == NULL) - hook->plugins = notleak(tal_arr(NULL, struct plugin *, 0)); + /* Make sure the hook_elements array is initialized. */ + if (hook->hooks == NULL) + hook->hooks = notleak(tal_arr(NULL, struct hook_instance *, 0)); /* If this is a single type hook and we have a plugin registered we * must fail this attempt to add the plugin to the hook. */ - if (hook->type == PLUGIN_HOOK_SINGLE && tal_count(hook->plugins) > 0) - return false; + if (hook->type == PLUGIN_HOOK_SINGLE && tal_count(hook->hooks) > 0) + return NULL; /* Ensure we don't register the same plugin multple times. */ - for (size_t i=0; iplugins); i++) - if (hook->plugins[i] == plugin) - return true; + for (size_t i=0; ihooks); i++) + if (hook->hooks[i]->plugin == plugin) + return NULL; /* Ok, we're sure they can register and they aren't yet registered, so * register them. */ - tal_arr_expand(&hook->plugins, plugin); - return true; -} - -bool plugin_hook_unregister(struct plugin *plugin, const char *method) -{ - struct plugin_hook *hook = plugin_hook_by_name(method); - - if (!hook || !hook->plugins) { - /* No such hook name registered */ - return false; - } - - for (size_t i = 0; i < tal_count(hook->plugins); i++) { - if (hook->plugins[i] == plugin) { - tal_arr_remove(&hook->plugins, i); - return true; - } - } - return false; -} - -void plugin_hook_unregister_all(struct plugin *plugin) -{ - static struct plugin_hook **hooks = NULL; - static size_t num_hooks; - if (!hooks) - hooks = autodata_get(hooks, &num_hooks); - - for (size_t i = 0; i < num_hooks; i++) - plugin_hook_unregister(plugin, hooks[i]->name); + h = tal(plugin, struct hook_instance); + h->plugin = plugin; + h->before = tal_arr(h, const char *, 0); + h->after = tal_arr(h, const char *, 0); + tal_add_destructor2(h, destroy_hook_instance, hook); + + tal_arr_expand(&hook->hooks, h); + return hook; } /* Mutual recursion */ @@ -244,23 +252,24 @@ bool plugin_hook_call_(struct lightningd *ld, const struct plugin_hook *hook, { struct plugin_hook_request *ph_req; struct plugin_hook_call_link *link; - if (tal_count(hook->plugins)) { + if (tal_count(hook->hooks)) { /* If we have a plugin that has registered for this * hook, serialize and call it */ /* FIXME: technically this is a leak, but we don't * currently have a list to store these. We might want * to eventually to inspect in-flight requests. */ - ph_req = notleak(tal(hook->plugins, struct plugin_hook_request)); + ph_req = notleak(tal(hook->hooks, struct plugin_hook_request)); ph_req->hook = hook; ph_req->cb_arg = tal_steal(ph_req, cb_arg); ph_req->db = ld->wallet->db; ph_req->ld = ld; list_head_init(&ph_req->call_chain); - for (size_t i=0; iplugins); i++) { + for (size_t i=0; ihooks); i++) { /* We allocate this off of the plugin so we get notified if the plugin dies. */ - link = tal(hook->plugins[i], struct plugin_hook_call_link); - link->plugin = hook->plugins[i]; + link = tal(hook->hooks[i]->plugin, + struct plugin_hook_call_link); + link->plugin = hook->hooks[i]->plugin; link->req = ph_req; tal_add_destructor(link, plugin_hook_killed); list_add_tail(&ph_req->call_chain, &link->list); @@ -324,10 +333,10 @@ void plugin_hook_db_sync(struct db *db) struct plugin *plugin; const char **changes = db_changes(db); - if (tal_count(hook->plugins) == 0) + if (tal_count(hook->hooks) == 0) return; - ph_req = notleak(tal(hook->plugins, struct plugin_hook_request)); + ph_req = notleak(tal(hook->hooks, struct plugin_hook_request)); /* FIXME: do IO logging for this! */ req = jsonrpc_request_start(NULL, hook->name, NULL, NULL, db_hook_response, @@ -335,7 +344,7 @@ void plugin_hook_db_sync(struct db *db) ph_req->hook = hook; ph_req->db = db; - plugin = ph_req->plugin = hook->plugins[0]; + plugin = ph_req->plugin = hook->hooks[0]->plugin; json_add_num(req->stream, "data_version", db_data_version_get(db)); @@ -357,3 +366,166 @@ void plugin_hook_db_sync(struct db *db) io_break(ret); } } + +static void add_deps(const char ***arr, + const char *buffer, + const jsmntok_t *arrtok) +{ + const jsmntok_t *t; + size_t i; + + if (!arrtok) + return; + + json_for_each_arr(i, t, arrtok) + tal_arr_expand(arr, json_strdup(*arr, buffer, t)); +} + +void plugin_hook_add_deps(struct plugin_hook *hook, + struct plugin *plugin, + const char *buffer, + const jsmntok_t *before, + const jsmntok_t *after) +{ + struct hook_instance *h = NULL; + + /* We just added this, it must exist */ + for (size_t i = 0; i < tal_count(hook->hooks); i++) { + if (hook->hooks[i]->plugin == plugin) { + h = hook->hooks[i]; + break; + } + } + assert(h); + + add_deps(&h->before, buffer, before); + add_deps(&h->after, buffer, after); +} + +struct hook_node { + /* Is this copied into the ordered array yet? */ + bool finished; + struct hook_instance *hook; + size_t num_incoming; + struct hook_node **outgoing; +}; + +static struct hook_node *find_hook(struct hook_node *graph, const char *name) +{ + for (size_t i = 0; i < tal_count(graph); i++) { + if (plugin_paths_match(graph[i].hook->plugin->cmd, name)) + return graph + i; + } + return NULL; +} + +/* Sometimes naive is best. */ +static struct hook_node *get_best_candidate(struct hook_node *graph) +{ + struct hook_node *best = NULL; + + for (size_t i = 0; i < tal_count(graph); i++) { + if (graph[i].finished) + continue; + if (graph[i].num_incoming != 0) + continue; + if (!best + || best->hook->plugin->index > graph[i].hook->plugin->index) + best = &graph[i]; + } + return best; +} + +static struct plugin **plugin_hook_make_ordered(const tal_t *ctx, + struct plugin_hook *hook) +{ + struct hook_node *graph, *n; + struct hook_instance **done; + + /* Populate graph nodes */ + graph = tal_arr(tmpctx, struct hook_node, tal_count(hook->hooks)); + for (size_t i = 0; i < tal_count(graph); i++) { + graph[i].finished = false; + graph[i].hook = hook->hooks[i]; + graph[i].num_incoming = 0; + graph[i].outgoing = tal_arr(graph, struct hook_node *, 0); + } + + /* Add edges. */ + for (size_t i = 0; i < tal_count(graph); i++) { + for (size_t j = 0; j < tal_count(graph[i].hook->before); j++) { + struct hook_node *n = find_hook(graph, + graph[i].hook->before[j]); + if (!n) { + /* This is useful for typos! */ + log_debug(graph[i].hook->plugin->log, + "hook %s before unknown plugin %s", + hook->name, + graph[i].hook->before[j]); + continue; + } + tal_arr_expand(&graph[i].outgoing, n); + n->num_incoming++; + } + for (size_t j = 0; j < tal_count(graph[i].hook->after); j++) { + struct hook_node *n = find_hook(graph, + graph[i].hook->after[j]); + if (!n) { + /* This is useful for typos! */ + log_debug(graph[i].hook->plugin->log, + "hook %s after unknown plugin %s", + hook->name, + graph[i].hook->after[j]); + continue; + } + tal_arr_expand(&n->outgoing, &graph[i]); + graph[i].num_incoming++; + } + } + + done = tal_arr(tmpctx, struct hook_instance *, 0); + while ((n = get_best_candidate(graph)) != NULL) { + tal_arr_expand(&done, n->hook); + n->finished = true; + for (size_t i = 0; i < tal_count(n->outgoing); i++) + n->outgoing[i]->num_incoming--; + } + + if (tal_count(done) != tal_count(hook->hooks)) { + struct plugin **ret = tal_arr(ctx, struct plugin *, 0); + for (size_t i = 0; i < tal_count(graph); i++) { + if (!graph[i].finished) + tal_arr_expand(&ret, graph[i].hook->plugin); + } + return ret; + } + + /* Success! Copy ordered hooks back. */ + memcpy(hook->hooks, done, tal_bytelen(hook->hooks)); + return NULL; +} + +/* Plugins could fail due to multiple hooks, but only add once. */ +static void append_plugin_once(struct plugin ***ret, struct plugin *p) +{ + for (size_t i = 0; i < tal_count(*ret); i++) { + if ((*ret)[i] == p) + return; + } + tal_arr_expand(ret, p); +} + +struct plugin **plugin_hooks_make_ordered(const tal_t *ctx) +{ + size_t num_hooks; + struct plugin_hook **hooks = get_hooks(&num_hooks); + struct plugin **ret = tal_arr(ctx, struct plugin *, 0); + + for (size_t i=0; inum_hook_subs; i++) - json_add_string(params, NULL, p->hook_subs[i].name); + for (size_t i = 0; i < p->num_hook_subs; i++) { + json_object_start(params, NULL); + json_add_string(params, "name", p->hook_subs[i].name); + if (p->hook_subs[i].before) { + json_array_start(params, "before"); + for (size_t j = 0; p->hook_subs[i].before[j]; j++) + json_add_string(params, NULL, + p->hook_subs[i].before[j]); + json_array_end(params); + } + if (p->hook_subs[i].after) { + json_array_start(params, "after"); + for (size_t j = 0; p->hook_subs[i].after[j]; j++) + json_add_string(params, NULL, + p->hook_subs[i].after[j]); + json_array_end(params); + } + json_object_end(params); + } json_array_end(params); if (p->our_features != NULL) { diff --git a/plugins/libplugin.h b/plugins/libplugin.h index 9ecb680654ea..684c8a41af44 100644 --- a/plugins/libplugin.h +++ b/plugins/libplugin.h @@ -93,6 +93,8 @@ struct plugin_hook { struct command_result *(*handle)(struct command *cmd, const char *buf, const jsmntok_t *params); + /* If non-NULL, these are NULL-terminated arrays of deps */ + const char **before, **after; }; /* Return the feature set of the current lightning node */ diff --git a/tests/plugins/dep_a.py b/tests/plugins/dep_a.py new file mode 100755 index 000000000000..be1af29ed5b1 --- /dev/null +++ b/tests/plugins/dep_a.py @@ -0,0 +1,15 @@ +#!/usr/bin/env python3 +from pyln.client import Plugin + +"""A simple plugin that must come before dep_b. +""" +plugin = Plugin() + + +@plugin.hook('htlc_accepted', before=['dep_b.py']) +def on_htlc_accepted(htlc, plugin, **kwargs): + print("htlc_accepted called") + return {'result': 'continue'} + + +plugin.run() diff --git a/tests/plugins/dep_b.py b/tests/plugins/dep_b.py new file mode 100755 index 000000000000..d11045728419 --- /dev/null +++ b/tests/plugins/dep_b.py @@ -0,0 +1,15 @@ +#!/usr/bin/env python3 +from pyln.client import Plugin + +"""A simple plugin that must come after dep_a, before dep_c. +""" +plugin = Plugin() + + +@plugin.hook('htlc_accepted', before=['dep_c.py'], after=['dep_a.py']) +def on_htlc_accepted(htlc, plugin, **kwargs): + print("htlc_accepted called") + return {'result': 'continue'} + + +plugin.run() diff --git a/tests/plugins/dep_c.py b/tests/plugins/dep_c.py new file mode 100755 index 000000000000..6c8eb656bdb9 --- /dev/null +++ b/tests/plugins/dep_c.py @@ -0,0 +1,15 @@ +#!/usr/bin/env python3 +from pyln.client import Plugin + +"""A simple plugin that must come before dep_a. +""" +plugin = Plugin() + + +@plugin.hook('htlc_accepted', before=['dep_a.py']) +def on_htlc_accepted(htlc, plugin, **kwargs): + print("htlc_accepted called") + return {'result': 'continue'} + + +plugin.run() diff --git a/tests/plugins/dep_d.py b/tests/plugins/dep_d.py new file mode 100755 index 000000000000..2f5fb935fda3 --- /dev/null +++ b/tests/plugins/dep_d.py @@ -0,0 +1,15 @@ +#!/usr/bin/env python3 +from pyln.client import Plugin + +"""A simple plugin that must come before dep_e. +""" +plugin = Plugin() + + +@plugin.hook('htlc_accepted', before=['dep_e.py']) +def on_htlc_accepted(htlc, plugin, **kwargs): + print("htlc_accepted called") + return {'result': 'continue'} + + +plugin.run() diff --git a/tests/plugins/dep_e.py b/tests/plugins/dep_e.py new file mode 100755 index 000000000000..6d1b679df04f --- /dev/null +++ b/tests/plugins/dep_e.py @@ -0,0 +1,15 @@ +#!/usr/bin/env python3 +from pyln.client import Plugin + +"""A simple plugin that registers an htlc_accepted hook.. +""" +plugin = Plugin() + + +@plugin.hook('htlc_accepted') +def on_htlc_accepted(htlc, plugin, **kwargs): + print("htlc_accepted called") + return {'result': 'continue'} + + +plugin.run() diff --git a/tests/plugins/test_libplugin.c b/tests/plugins/test_libplugin.c index 073565696698..ac2641304369 100644 --- a/tests/plugins/test_libplugin.c +++ b/tests/plugins/test_libplugin.c @@ -118,9 +118,14 @@ static const struct plugin_command commands[] = { { } }; +static const char *before[] = { "dummy", NULL }; +static const char *after[] = { "dummy", NULL }; + static const struct plugin_hook hooks[] = { { "peer_connected", json_peer_connected, + before, + after } }; diff --git a/tests/test_plugin.py b/tests/test_plugin.py index 598833f8343c..73d031bbd776 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -612,18 +612,9 @@ def test_openchannel_hook_chaining(node_factory, bitcoind): with pytest.raises(RpcError, match=r'They sent error channel'): l1.rpc.fundchannel(l2.info['id'], 100005) - # Note: hook chain order is currently undefined, because hooks are registerd - # as a result of the getmanifest call, which may take some random time. - # We need to workaround that fact, so test can be stable - correct_order = l2.daemon.is_in_log(hook_msg + "reject for a reason") - if correct_order: - assert l2.daemon.wait_for_log(hook_msg + "reject for a reason") - # the other plugin must not be called - assert not l2.daemon.is_in_log("reject on principle") - else: - assert l2.daemon.wait_for_log(hook_msg + "reject on principle") - # the other plugin must not be called - assert not l2.daemon.is_in_log("reject for a reason") + assert l2.daemon.wait_for_log(hook_msg + "reject for a reason") + # the third plugin must now not be called anymore + assert not l2.daemon.is_in_log("reject on principle") # 100000sat is good for hook_accepter, so it should fail 'on principle' # at third hook openchannel_reject.py @@ -1990,3 +1981,72 @@ def test_htlc_accepted_hook_failcodes(node_factory): inv = l2.rpc.invoice(42, 'failcode{}'.format(failcode), '')['bolt11'] with pytest.raises(RpcError, match=r'failcodename.: .{}.'.format(expected)): l1.rpc.pay(inv) + + +def test_hook_dep(node_factory): + dep_a = os.path.join(os.path.dirname(__file__), 'plugins/dep_a.py') + dep_b = os.path.join(os.path.dirname(__file__), 'plugins/dep_b.py') + dep_c = os.path.join(os.path.dirname(__file__), 'plugins/dep_c.py') + l1, l2, l3 = node_factory.line_graph(3, opts=[{}, + {'plugin': dep_b}, + {'plugin': [dep_a, dep_b]}]) + + # l2 complains about the two unknown plugins, only. + # (Could be already past) + l2.daemon.logsearch_start = 0 + l2.daemon.wait_for_logs(["unknown plugin dep_a.py", + "unknown plugin dep_c.py"]) + assert not l2.daemon.is_in_log("unknown plugin (?!dep_a.py|dep_c.py)") + logstart = l2.daemon.logsearch_start + + # l3 complains about the dep_c, only. + assert l3.daemon.is_in_log("unknown plugin dep_c.py") + assert not l3.daemon.is_in_log("unknown plugin (?!dep_c.py)") + + # A says it has to be before B. + l2.rpc.plugin_start(plugin=dep_a) + l2.daemon.wait_for_log(r"started.*dep_a.py") + # Still doesn't know about c. + assert l2.daemon.is_in_log("unknown plugin dep_c.py", logstart) + + l1.pay(l2, 100000) + # They must be called in this order! + l2.daemon.wait_for_log(r"dep_a.py: htlc_accepted called") + l2.daemon.wait_for_log(r"dep_b.py: htlc_accepted called") + + # But depc will not load, due to cyclical dep + with pytest.raises(RpcError, match=r'Cannot meet required hook dependencies'): + l2.rpc.plugin_start(plugin=dep_c) + + l1.rpc.plugin_start(plugin=dep_c) + l1.daemon.wait_for_log(r"started.*dep_c.py") + + # Complaints about unknown plugin a, but nothing else + assert l1.daemon.is_in_log("unknown plugin dep_a.py") + assert not l1.daemon.is_in_log("unknown plugin (?!dep_a.py)") + + +def test_hook_dep_stable(node_factory): + # Load in order A, D, E, B. + # A says it has to be before B, D says it has to be before E. + # It should load in the order specified. + + dep_a = os.path.join(os.path.dirname(__file__), 'plugins/dep_a.py') + dep_b = os.path.join(os.path.dirname(__file__), 'plugins/dep_b.py') + dep_d = os.path.join(os.path.dirname(__file__), 'plugins/dep_d.py') + dep_e = os.path.join(os.path.dirname(__file__), 'plugins/dep_e.py') + l1, l2 = node_factory.line_graph(2, opts=[{}, + {'plugin': [dep_a, dep_d, dep_e, dep_b]}]) + + # dep_a mentions deb_c, but nothing else should be unknown. + # (Could be already past) + l2.daemon.logsearch_start = 0 + l2.daemon.wait_for_log("unknown plugin dep_c.py") + assert not l2.daemon.is_in_log("unknown plugin (?!|dep_c.py)") + + l1.pay(l2, 100000) + # They must be called in this order! + l2.daemon.wait_for_log(r"dep_a.py: htlc_accepted called") + l2.daemon.wait_for_log(r"dep_d.py: htlc_accepted called") + l2.daemon.wait_for_log(r"dep_e.py: htlc_accepted called") + l2.daemon.wait_for_log(r"dep_b.py: htlc_accepted called")