plugins: never unregister important hooks and maintain hook semantics…

… in shutdown

Here important-plugin implies `important hook`.

Before this commit, when in shutdown:
- existing in-flight hooks where abandoned, cutting the hook-chain and never call hook_final_cb
- hooks where removed when its plugin died, even important-plugin because `shutdown` overrules
- but hook events can be called while waiting for plugins to self-terminate (up to 30s) and
  subdaemons still alive and it looks as if no plugin ever registered the hook.

After this commit, when in shutdown:
- existing in-flight hook (chains) are honoured and can finalize, same semantics as LD_STATE_RUNNING
- important-plugins are kept alive until after shutdown_subdaemons, so they don't miss hooks
- JSON RPC commands are functional, but anything unimportant-plugin related cannot be relied on

TODO:
- Run tests -> hangs forever on test_closing, so skip them

- Q. Does this open a can of worms or races when (normal) plugins with hooks die randomly?
  A. Yes, for example htlc_accepted calls triggers hook invoice_payment, but plugin (fetchinvoice?) already died

**
* CONCLUSION: If you want to give more control over shutdown, I think there could be
* a plugin `shutdown_clean.py` with RPC method `shutdown_clean`. When called, that
* plugin starts additional (important) plugin(s) that register relevant hooks and, for example, hold-off
* new htcl's and wait for existing inflight htlc's to resolve ... and finally call RPC `stop`.
*
* Note: --important-plugin only seems to work at start, not via `plugin start shutdown_clean.py`
* maybe we can add? Or do something with disable?
*
* Some parts of this commit is stil good, i.e. hook semantics of important plugins should be consistent
* untill the very last potential hook call.
**

- What if important-plugin dies unexpectatly and lightningd_exit() calls io_break() is that bad?
- What are the benefits? Add example where on shutdown inflight htlc's are resolved/cleared and
  new htlc's blocked, see ElementsProject#4842

- Split commit into hook-related stuff and others, for clarity of reasoning
- Q. How does this relate (hook-wise) to db_write plugins? A. Looks like this hook is treated like
  any other hook: when plugin dies, hook is removed, so to be safe backup needs to be `important`.
  Hook documentation does not mention `important-plugin` but BACKUP.md does.
  TODO: Tested this -> `plugin stop backup.py` -> "plugin-backup.py: Killing plugin: exited during normal operation"
  In fact, running current backup.py with current master misses a couple of writes in
  shutdown (because its hook is removed, see issue ElementsProject#4785).

lightningd/plugin.c

@@ -2092,7 +2092,8 @@ void shutdown_plugins(struct lightningd *ld, bool first)
 
 	/* Mark the set of plugins we want to shutdown in this call */
 	list_for_each_safe(&ld->plugins->plugins, p, next, list) {
-		if (first && plugin_registered_db_write_hook(p))
+		/* FIXME: Make db_write plugin's important by default? */
+		if (first && (plugin_registered_db_write_hook(p) || p->important))
 			continue;
 
 		/* don't complain about important plugins vanishing and stop sending

lightningd/plugin_hook.c

@@ -54,7 +54,7 @@ static struct plugin_hook *plugin_hook_by_name(const char *name)
 	return NULL;
 }
 
-/* When we destroy a plugin, we remove its hooks */
+/* When we destroy an unimportant plugin, we remove its hooks */
 static void destroy_hook_instance(struct hook_instance *h,
 				  struct plugin_hook *hook)
 {
@@ -91,7 +91,10 @@ struct plugin_hook *plugin_hook_register(struct plugin *plugin, const char *meth
 	h->plugin = plugin;
 	h->before = tal_arr(h, const char *, 0);
 	h->after = tal_arr(h, const char *, 0);
-	tal_add_destructor2(h, destroy_hook_instance, hook);
+
+	/* Never unregister important hooks */
+	if (!plugin->important)
+		tal_add_destructor2(h, destroy_hook_instance, hook);
 
 	tal_arr_expand(&hook->hooks, h);
 	return hook;
@@ -163,12 +166,6 @@ static void plugin_hook_callback(const char *buffer, const jsmntok_t *toks,
 	tal_del_destructor(last, plugin_hook_killed);
 	tal_free(last);
 
-	if (r->ld->state == LD_STATE_SHUTDOWN) {
-		log_debug(r->ld->log,
-			  "Abandoning plugin hook call due to shutdown");
-		return;
-	}
-
 	log_debug(r->ld->log, "Plugin %s returned from %s hook call",
 		  r->plugin->shortname, r->hook->name);