From fb295ffb51d3eca178f6d102d49f690ebaf3f7b7 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 3 Nov 2020 09:50:52 +1030 Subject: [PATCH] plugin: sort topological candidates by specified order. We previously registered hooks up in who-replies-to-getmanifest-first order, but then if any had dependencies it would scatter that order. This allows users to manually set dependencies developers have forgotten by specifying the plugins manually in their configuration or cmdline. This was an excellent consideration by @mschmook. Signed-off-by: Rusty Russell --- doc/PLUGINS.md | 14 ++++-- doc/lightningd-config.5 | 9 ++-- doc/lightningd-config.5.md | 7 ++- lightningd/plugin.c | 2 + lightningd/plugin.h | 6 +++ lightningd/plugin_hook.c | 93 ++++++++++++++++---------------------- tests/test_plugin.py | 1 - 7 files changed, 66 insertions(+), 66 deletions(-) diff --git a/doc/PLUGINS.md b/doc/PLUGINS.md index 8ea5d3288..306d71aaf 100644 --- a/doc/PLUGINS.md +++ b/doc/PLUGINS.md @@ -712,11 +712,15 @@ 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 in any order which meets their `before` and `after` requirements -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 61e0af300..61708660d 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 91aebbc97..8bd4fc591 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 1c89d141a..370ee63b0 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 */ @@ -199,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)); diff --git a/lightningd/plugin.h b/lightningd/plugin.h index 4b1a91f25..070c0184a 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 9a000e91a..321cb75fe 100644 --- a/lightningd/plugin_hook.c +++ b/lightningd/plugin_hook.c @@ -1,3 +1,4 @@ +#include #include #include #include @@ -401,25 +402,9 @@ void plugin_hook_add_deps(struct plugin_hook *hook, add_deps(&h->after, buffer, after); } -/* From https://en.wikipedia.org/wiki/Topological_sorting#Kahn's_algorithm - * (https://creativecommons.org/licenses/by-sa/3.0/): - * L ← Empty list that will contain the sorted elements - * S ← Set of all nodes with no incoming edge - * - * while S is not empty do - * remove a node n from S - * add n to L - * for each node m with an edge e from n to m do - * remove edge e from the graph - * if m has no other incoming edges then - * insert m into S - * - * if graph has edges then - * return error (graph has at least one cycle) - * else - * return L (a topologically sorted order) - */ 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; @@ -434,16 +419,33 @@ static struct hook_node *find_hook(struct hook_node *graph, const char *name) 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; - struct hook_node **l, **s; - struct plugin **ret; + 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); @@ -481,45 +483,26 @@ static struct plugin **plugin_hook_make_ordered(const tal_t *ctx, } } - /* Populate array of ready-to-go nodes. */ - s = tal_arr(graph, struct hook_node *, 0); - for (size_t i = 0; i < tal_count(graph); i++) { - if (graph[i].num_incoming == 0) - tal_arr_expand(&s, &graph[i]); + 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--; } - l = tal_arr(graph, struct hook_node *, 0); - while (tal_count(s)) { - struct hook_node *n = s[0]; - tal_arr_expand(&l, n); - tal_arr_remove(&s, 0); - - /* for each node m with an edge e from n to m do - * remove edge e from the graph - * if m has no other incoming edges then - * insert m into S - */ - for (size_t i = 0; i < tal_count(n->outgoing); i++) { - if (--n->outgoing[i]->num_incoming == 0) - tal_arr_expand(&s, n->outgoing[i]); + 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); } - } - - /* Check for any left over: these cannot be loaded. */ - ret = tal_arr(ctx, struct plugin *, 0); - for (size_t i = 0; i < tal_count(graph); i++) { - if (graph[i].num_incoming) - tal_arr_expand(&ret, graph[i].hook->plugin); - } - if (tal_count(ret) != 0) return ret; + } - /* Success! Write them back in order. */ - assert(tal_count(l) == tal_count(hook->hooks)); - for (size_t i = 0; i < tal_count(hook->hooks); i++) - hook->hooks[i] = l[i]->hook; - - return tal_free(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. */ diff --git a/tests/test_plugin.py b/tests/test_plugin.py index c96a22c74..5c41737ce 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -2118,7 +2118,6 @@ def test_hook_dep(node_factory): assert not l1.daemon.is_in_log("unknown plugin (?!dep_a.py)") -@pytest.mark.xfail(strict=True) 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.