lightningd: shutdown plugins in 2 steps, db_write plugins after closi…

…ng db

inspired by ElementsProject#4790

Still not perfect as a db_write plugin doesn't know when it can safely exit.
Because it doesn't know where lightningd is.

Maybe lightningd can in the second shutdown_plugin call, write an EOF to plugin's
stdin, which the pyln.client can handle and wait for and then self-terminate.

lightningd/jsonrpc.c

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

@@ -1235,8 +1235,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);
@@ -1250,6 +1250,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);
+
 	/* 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 "in state shutdown";
 	}
 	fatal("Invalid plugin state %i for %s",
 	      plugin->plugin_state, plugin->cmd);
@@ -216,8 +218,8 @@ 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 (!plugins_any_in_state(p->plugins, SHUTDOWN)) {
 			log_debug(p->plugins->ld->log, "io_break: %s", __func__);
 			io_break(destroy_plugin);
 		}
@@ -2152,19 +2154,43 @@ void plugins_set_builtin_plugins_dir(struct plugins *plugins,
 				NULL, NULL);
 }
 
-void shutdown_plugins(struct lightningd *ld)
+void shutdown_plugins(struct lightningd *ld, bool first_call)
 {
 	struct plugin *p, *next;
+	bool notified;
 
-	/* 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))
+
+		/* Kill uninitialized plugins, freeing also removes it from list */
+		if (p->plugin_state < INIT_COMPLETE) {
 			tal_free(p);
+			continue;
+		}
+
+		/* First call: Send shutdown notification while jsonrpc still exists */
+		if (first_call) {
+			notified = notify_plugin_shutdown(ld, p);
+
+			/* Always keep db_write plugins alive in first call */
+			if (plugin_registered_db_write_hook(p))
+				continue;
+
+			/* Mark the plugins we shall wait for, others are killed */
+			if (notified)
+				p->plugin_state = SHUTDOWN;
+			else
+				tal_free(p);
+
+		/* Second call: Only wait if plugin got notified in first call */
+		} else {
+			p->plugin_state = SHUTDOWN;
+			if (!plugin_subscriptions_contains(p, "shutdown"))
+				tal_free(p);
+		}
 	}
 
-	/* If anyone was interested in shutdown, give them time. */
-	if (!list_empty(&ld->plugins->plugins)) {
+	/* Wait for the marked plugins to self-terminate, see also destroy_plugin */
+	if (plugins_any_in_state(ld->plugins, SHUTDOWN)) {
 		struct timers *timer;
 		struct timer *expired;
 
@@ -2176,13 +2202,28 @@ void shutdown_plugins(struct lightningd *ld)
 		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 (first_call)
+		/* In first call, remove JSON-RPC methods of remaining 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,12 @@ 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, and free them.
+ *
+ * @skip_db_write_plugins - keep db_write plugins alive, i.e. the ones that
+ * registered the db_write hook
  */
-void shutdown_plugins(struct lightningd *ld);
+void shutdown_plugins(struct lightningd *ld, bool first);
 
 /**
  * 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 first_call UNNEEDED)
 { fprintf(stderr, "shutdown_plugins called!\n"); abort(); }
 /* Generated stub for stop_topology */
 void stop_topology(struct chain_topology *topo UNNEEDED)