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.