lightningd: make shutdown_plugins a two-step function

so that plugins that registered db_write hook are kept alive
until after the last db transaction

individual plugins can now be in state SHUTDOWN

issue ElementsProject#4785

lightningd/lightningd.c

@@ -1133,8 +1133,8 @@ int main(int argc, char *argv[])
 	/* We're not going to collect our children. */
 	remove_sigchild_handler();
 
-	/* Tell plugins we're shutting down. */
-	shutdown_plugins(ld);
+	/* Tell plugins we're shutting down, except db_write plugins */
+	shutdown_plugins(ld, true);
 	shutdown_subdaemons(ld);
 
 	/* Clean up the JSON-RPC. This needs to happen in a DB transaction since
@@ -1144,6 +1144,10 @@ int main(int argc, char *argv[])
 	tal_free(ld->jsonrpc);
 	db_commit_transaction(ld->wallet->db);
 
+	/* Shutdown remaining plugins and close the db */
+	shutdown_plugins(ld, false);
+	tal_free(ld->wallet);
+
 	/* Clean our our HTLC maps, since they use malloc. */
 	htlc_in_map_clear(&ld->htlcs_in);
 	htlc_out_map_clear(&ld->htlcs_out);

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 "during shutdown";
 	}
 	fatal("Invalid plugin state %i for %s",
 	      plugin->plugin_state, plugin->cmd);
@@ -82,7 +84,6 @@ struct plugins *plugins_new(const tal_t *ctx, struct log_book *log_book,
 	p->startup = true;
 	p->plugin_cmds = tal_arr(p, struct plugin_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;
@@ -193,9 +194,9 @@ static void destroy_plugin(struct plugin *p)
 
 	/* If we are shutting down, do not continue to checking if
 	 * the dying plugin is important.  */
-	if (p->plugins->shutdown) {
-		/* But return if this was the last plugin! */
-		if (list_empty(&p->plugins->plugins))
+	if (p->plugin_state == SHUTDOWN) {
+		/* At shutdown we wait for plugins to self-terminate */
+		if (!plugins_any_in_state(p->plugins, SHUTDOWN))
 			io_break(p->plugins);
 		return;
 	}
@@ -722,7 +723,7 @@ static void plugin_conn_finish(struct io_conn *conn, struct plugin *plugin)
 {
 	/* This is expected at shutdown of course. */
 	plugin_kill(plugin,
-		    plugin->plugins->shutdown
+		    plugin->plugin_state == SHUTDOWN
 		    ? LOG_DBG : LOG_INFORM,
 		    "exited %s", state_desc(plugin));
 }
@@ -1974,12 +1975,10 @@ void plugins_notify(struct plugins *plugins,
 	if (taken(n))
 		tal_steal(tmpctx, n);
 
-	/* If we're shutting down, ld->plugins will be NULL */
-	if (plugins) {
-		list_for_each(&plugins->plugins, p, list) {
-			plugin_single_notify(p, n);
-		}
-	}
+    list_for_each(&plugins->plugins, p, list) {
+        if (p->plugin_state != SHUTDOWN)
+            plugin_single_notify(p, n);
+    }
 }
 
 static void destroy_request(struct jsonrpc_request *req,
@@ -2080,43 +2079,51 @@ static void plugin_shutdown_timeout(struct lightningd *ld)
 	io_break(ld->plugins);
 }
 
-void shutdown_plugins(struct lightningd *ld)
+void shutdown_plugins(struct lightningd *ld, bool keep_db_write_plugins)
 {
 	struct plugin *p, *next;
 
-	/* This makes sure we don't complain about important plugins
-	 * vanishing! */
-	ld->plugins->shutdown = true;
-
-	/* Tell them all to shutdown; if they care. */
 	list_for_each_safe(&ld->plugins->plugins, p, next, list) {
+
+		/* maybe keep some alive for db_write's */
+		if (plugin_registered_db_write_hook(p) && keep_db_write_plugins)
+			continue;
+
+		/* don't complain about important plugins vanishing and stop sending
+		 * it any (other) notifications */
+		p->plugin_state = SHUTDOWN;
+
 		/* Kill immediately, deletes self from list. */
 		if (!notify_plugin_shutdown(ld, p))
 			tal_free(p);
 	}
 
 	/* If anyone was interested in shutdown, give them time. */
-	if (!list_empty(&ld->plugins->plugins)) {
+	if (plugins_any_in_state(ld->plugins, SHUTDOWN)) {
 		struct oneshot *t;
 
-		/* 30 seconds should do it. */
+		/* 30 seconds should do it, plugin destructor can also io_break */
 		t = new_reltimer(ld->timers, ld,
 				 time_from_sec(30),
 				 plugin_shutdown_timeout, ld);
 
 		io_loop_with_timers(ld);
 		tal_free(t);
 
-		/* Report and free remaining plugins. */
-		while (!list_empty(&ld->plugins->plugins)) {
-			p = list_pop(&ld->plugins->plugins, struct plugin, list);
+		/* Report and free remaining plugins */
+		list_for_each_safe(&ld->plugins->plugins, p, next, list) {
+			/* except ones we want to keep alive */
+			if (plugin_registered_db_write_hook(p) && keep_db_write_plugins)
+				continue;
+
 			log_debug(ld->log,
 				  "%s: failed to shutdown, killing.",
 				  p->shortname);
 			tal_free(p);
 		}
 	}
 
-	/* NULL stops notifications trying to access plugins. */
-	ld->plugins = tal_free(ld->plugins);
+	if (!keep_db_write_plugins) {
+		ld->plugins = tal_free(ld->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,
+	/* We are shutting down, maybe waiting for it to self-terminate */
+	SHUTDOWN
 };
 
 /**
@@ -113,9 +115,6 @@ struct plugins {
 	/* Blacklist of plugins from --disable-plugin */
 	const char **blacklist;
 
-	/* Whether we are shutting down (`plugins_free` is called) */
-	bool shutdown;
-
 	/* Index to show what order they were added in */
 	u64 plugin_idx;
 
@@ -236,9 +235,10 @@ 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 plugins we're shutting down, wait and free them. Except
+ * plugins that registered the db_write hook when `keep_db_write_plugins`.
  */
-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

@@ -290,6 +290,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

@@ -108,6 +108,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

@@ -199,7 +199,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)