lightningd: shutdown plugins in 2 steps, db_write plugins as last aft…

…er closing db

in second shutdown_plugin call, trigger EOF in plugin stdin.

This correctly handles corner cases in shutdown_plugin:
- in a non-dev build, buildin plugins don't subscribe shutdown,
  but we still need a (brief) io_loop to notify interested db_write
  plugins
- only an awaited-for plugin should call io_break
- allways send EOF to db_write plugins in the final call (subscribed or not)

This adds two helper functions:
 plugin_registered_db_write_hook
 jsonrpc_command_del

and a new plugin state: SHUTDOWN

inspired by ElementsProject#4790 and also
credit to ZmnSCPxj for mentioning the EOF idea in ElementsProject#4785

lightningd/jsonrpc.c

@@ -1227,6 +1227,18 @@ bool jsonrpc_command_add(struct jsonrpc *rpc, struct json_command *command,
 	return true;
 }
 
+bool jsonrpc_command_del(struct jsonrpc *rpc, const char *name)
+{
+	size_t count = tal_count(rpc->commands);
+	for (size_t j = 0; j < count; j++) {
+		if (streq(name, rpc->commands[j]->name)) {
+			   tal_free(rpc->commands[j]);
+			   return true;
+		}
+	}
+	return false;
+}
+
 static bool jsonrpc_command_add_perm(struct lightningd *ld,
 				     struct jsonrpc *rpc,
 				     struct json_command *command)

lightningd/jsonrpc.h

@@ -210,6 +210,9 @@ void jsonrpc_stop_all(struct lightningd *ld);
 bool jsonrpc_command_add(struct jsonrpc *rpc, struct json_command *command,
 			 const char *usage TAKES);
 
+/* Remove a command from the JSON-RPC interface */
+bool jsonrpc_command_del(struct jsonrpc *rpc, const char *name);
+
 /**
  * Begin a JSON-RPC notification with the specified topic.
  *

lightningd/lightningd.c

@@ -1244,8 +1244,8 @@ int main(int argc, char *argv[])
 	/* Get rid of per-channel subdaemons. */
 	subd_shutdown_nonglobals(ld);
 
-	/* Tell plugins we're shutting down, use force if necessary. */
-	shutdown_plugins(ld);
+	/* Tell normal plugins we're shutting down, use force if necessary. */
+	shutdown_plugins(ld, true);
 
 	/* Now kill any remaining connections */
 	jsonrpc_stop_all(ld);
@@ -1259,6 +1259,9 @@ int main(int argc, char *argv[])
 	/* Now close database */
 	ld->wallet->db = tal_free(ld->wallet->db);
 
+	/* As database is closed we can safely shutdown db_write plugins */
+	shutdown_plugins(ld, false);
+
 	remove(ld->pidfile);
 
 	/* FIXME: pay can have children off tmpctx which unlink from

lightningd/plugin.c

@@ -65,6 +65,8 @@ static const char *state_desc(const struct plugin *plugin)
 		return "before replying to init";
 	case INIT_COMPLETE:
 		return "during normal operation";
+	case SHUTDOWN:
+		return "in state shutdown";
 	}
 	fatal("Invalid plugin state %i for %s",
 	      plugin->plugin_state, plugin->cmd);
@@ -216,8 +218,10 @@ static void destroy_plugin(struct plugin *p)
 
 	/* Daemon shutdown overrules plugin's importance; aborts init checks */
 	if (p->plugins->ld->state == LD_STATE_SHUTDOWN) {
-		/* But return if this was the last plugin! */
-		if (list_empty(&p->plugins->plugins)) {
+
+		/* But break io_loop if this was the last plugin we waited for */
+		if (p->plugin_state == SHUTDOWN &&
+				    !plugins_any_in_state(p->plugins, SHUTDOWN)) {
 			log_debug(p->plugins->ld->log, "io_break: %s", __func__);
 			io_break(destroy_plugin);
 		}
@@ -2140,37 +2144,88 @@ void plugins_set_builtin_plugins_dir(struct plugins *plugins,
 				NULL, NULL);
 }
 
-void shutdown_plugins(struct lightningd *ld)
+void shutdown_plugins(struct lightningd *ld, bool keep_db_write_plugins)
 {
 	struct plugin *p, *next;
+	bool need_io = false;
 
-	/* Tell them all to shutdown; if they care. */
 	list_for_each_safe(&ld->plugins->plugins, p, next, list) {
-		/* Kill immediately, deletes self from list. */
-		if (p->plugin_state != INIT_COMPLETE || !notify_plugin_shutdown(ld, p))
+
+		/* Simply kill uninitialized plugin, freeing removes it from list */
+		if (p->plugin_state < INIT_COMPLETE) {
 			tal_free(p);
+			continue;
+		}
+
+		/* First call: */
+		if (keep_db_write_plugins) {
+
+			/* Send shutdown notifications while jsonrpc still exists */
+			assert(ld->jsonrpc);
+			bool notify = notify_plugin_shutdown(ld, p);
+			need_io = need_io || notify;
+
+			if (plugin_registered_db_write_hook(p))
+				continue;
+
+			/* Mark the plugins we shall wait for, others are killed */
+			if (notify)
+				p->plugin_state = SHUTDOWN;
+			else
+				tal_free(p);
+
+		/* Second call: */
+		} else {
+			assert(!ld->wallet->db);
+
+			/* io_close sends EOF to plugin's stdin, which eventually breaks its
+			 * main io-loop (safely) after subroutines have returned. */
+			io_close(p->stdin_conn);
+			need_io = true;
+
+			/* But we still wait for them to self-terminate. */
+			p->plugin_state = SHUTDOWN;
+		}
 	}
 
-	/* If anyone was interested in shutdown, give them time. */
-	if (!list_empty(&ld->plugins->plugins)) {
+	/* io_loop sends the notifications, waits 30 seconds for marked plugins
+	 * to self-terminate, see also destroy_plugin */
+	if (need_io) {
 		struct timers *timer;
 		struct timer *expired;
 
-		/* 30 seconds should do it, use a clean timers struct */
+		/* But don't wait full 30s to just notify, also use a clean timers
+		 * struct to ignore any existing timers. */
+		int t = plugins_any_in_state(ld->plugins, SHUTDOWN) ? 30000 : 1;
 		timer = tal(NULL, struct timers);
 		timers_init(timer, time_mono());
-		new_reltimer(timer, timer, time_from_sec(30), NULL, NULL);
+		new_reltimer(timer, timer, time_from_msec(t), NULL, NULL);
 
 		void *ret = io_loop(timer, &expired);
 		assert(ret == NULL || ret == destroy_plugin);
 
-		/* Report and free remaining plugins. */
-		while (!list_empty(&ld->plugins->plugins)) {
-			p = list_pop(&ld->plugins->plugins, struct plugin, list);
+		/* Report and free plugins that didn't self-terminate in time */
+		list_for_each_safe(&ld->plugins->plugins, p, next, list) {
+			if (p->plugin_state != SHUTDOWN) {
+				continue;
+			}
+
+			list_del_from(&ld->plugins->plugins, &p->list);
 			log_debug(ld->log,
 				  "%s: failed to self-terminate in time, killing.",
 				  p->shortname);
 			tal_free(p);
 		}
 	}
+
+	if (keep_db_write_plugins)
+		/* Remove JSON-RPC methods of remaining db_write plugins, so we can
+		 * free jsonrpc. */
+		list_for_each(&ld->plugins->plugins, p, list) {
+			for (size_t i=0; i<tal_count(p->methods); i++) {
+				jsonrpc_command_del(ld->jsonrpc, p->methods[i]);
+			}
+		}
+	else
+		assert(list_empty(&ld->plugins->plugins));
 }

lightningd/plugin.h

@@ -16,7 +16,9 @@ enum plugin_state {
 	/* We have to get `init` response */
 	AWAITING_INIT_RESPONSE,
 	/* We have `init` response. */
-	INIT_COMPLETE
+	INIT_COMPLETE,
+	/* Wait for it to self-terminate */
+	SHUTDOWN,
 };
 
 /**
@@ -238,9 +240,16 @@ void plugin_kill(struct plugin *plugin, enum log_level loglevel,
 		 const char *fmt, ...);
 
 /**
- * Tell all the plugins we're shutting down, and free them.
+ * Tell subscribed plugins we're shutting down, wait for them to self-terminate
+ * and free them.
+ *
+ * @keep_db_write_plugins=true: plugins that registered the db_write hook are
+ * kept alive, but still send them a "shutdown" notification when subscribed.
+ *
+ * @keep_db_write_plugins=false: shutdown remaining plugins: send EOF to their
+ * stdin, then wait for them to self-terminate.
  */
-void shutdown_plugins(struct lightningd *ld);
+void shutdown_plugins(struct lightningd *ld, bool keep_db_write_plugins);
 
 /**
  * Returns the plugin which registers the command with name {cmd_name}

lightningd/plugin_hook.c

@@ -304,6 +304,16 @@ struct db_write_hook_req {
 	size_t *num_hooks;
 };
 
+bool plugin_registered_db_write_hook(struct plugin *plugin) {
+	const struct plugin_hook *hook = &db_write_hook;
+
+	for (size_t i = 0; i < tal_count(hook->hooks); i++)
+		if (hook->hooks[i]->plugin == plugin)
+			return true;
+
+	return false;
+}
+
 static void db_hook_response(const char *buffer, const jsmntok_t *toks,
 			     const jsmntok_t *idtok,
 			     struct db_write_hook_req *dwh_req)

lightningd/plugin_hook.h

@@ -110,6 +110,9 @@ bool plugin_hook_continue(void *arg, const char *buffer, const jsmntok_t *toks);
 struct plugin_hook *plugin_hook_register(struct plugin *plugin,
 					 const char *method);
 
+/* Helper to tell if plugin registered the db_write hook */
+bool plugin_registered_db_write_hook(struct plugin *plugin);
+
 /* Special sync plugin hook for db. */
 void plugin_hook_db_sync(struct db *db);
 

lightningd/test/run-find_my_abspath.c

@@ -195,7 +195,7 @@ void setup_topology(struct chain_topology *topology UNNEEDED,
 		    u32 min_blockheight UNNEEDED, u32 max_blockheight UNNEEDED)
 { fprintf(stderr, "setup_topology called!\n"); abort(); }
 /* Generated stub for shutdown_plugins */
-void shutdown_plugins(struct lightningd *ld UNNEEDED)
+void shutdown_plugins(struct lightningd *ld UNNEEDED, bool keep_db_write_plugins UNNEEDED)
 { fprintf(stderr, "shutdown_plugins called!\n"); abort(); }
 /* Generated stub for stop_topology */
 void stop_topology(struct chain_topology *topo UNNEEDED)