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 <[email protected]>

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
 {

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

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

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));

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;

lightningd/plugin_hook.c

@@ -1,3 +1,4 @@
+#include <ccan/asort/asort.h>
 #include <ccan/io/io.h>
 #include <ccan/list/list.h>
 #include <common/configdir.h>
@@ -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. */

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.