From b4a2ce7b858ee9e12e0304115d9f2d9d8b10a83a Mon Sep 17 00:00:00 2001 From: Simon Vrouwe Date: Tue, 27 Aug 2019 08:00:31 +0300 Subject: [PATCH 1/4] lightningd/plugin: add `early_conf` field, allowing plugins to sync init early To accomodate for plugins that want to be initialized before anything significant happens, such as a db_write in case of a backup plugin. --- lightningd/lightningd.c | 3 ++ lightningd/plugin.c | 52 +++++++++++++++++++++++++-- lightningd/plugin.h | 11 ++++++ lightningd/test/run-find_my_abspath.c | 3 ++ 4 files changed, 67 insertions(+), 2 deletions(-) diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index e66f7c8576c0..7027fa31040b 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -686,6 +686,9 @@ int main(int argc, char *argv[]) /*~ Make sure we can reach the subdaemons, and versions match. */ test_subdaemons(ld); + /* Some *restricted* plugins may init (or abort) early before db touched */ + plugins_early_config(ld->plugins); + /*~ Our "wallet" code really wraps the db, which is more than a simple * bitcoin wallet (though it's that too). It also stores channel * states, invoices, payments, blocks and bitcoin transactions. */ diff --git a/lightningd/plugin.c b/lightningd/plugin.c index 69d4e824d4a6..d909d9ead433 100644 --- a/lightningd/plugin.c +++ b/lightningd/plugin.c @@ -32,6 +32,8 @@ * willing to wait. Plugins shouldn't do any initialization in the * `getmanifest` call anyway, that's what `init `is for. */ #define PLUGIN_MANIFEST_TIMEOUT 60 +/* Timeout for the init response of early_config plugins */ +#define PLUGIN_CONFIG_TIMEOUT 10 struct plugins *plugins_new(const tal_t *ctx, struct log_book *log_book, struct lightningd *ld) @@ -796,6 +798,12 @@ static bool plugin_hooks_add(struct plugin *plugin, const char *buffer, return true; } +static void plugin_early_conf_timeout(struct plugin *plugin) +{ + log_broken(plugin->log, "The early_conf plugin failed to respond to \"init\" in time, terminating."); + fatal("Can't recover from plugin failure, terminating."); +} + static void plugin_manifest_timeout(struct plugin *plugin) { log_broken(plugin->log, "The plugin failed to respond to \"getmanifest\" in time, terminating."); @@ -810,8 +818,8 @@ static void plugin_manifest_cb(const char *buffer, const jsmntok_t *idtok, struct plugin *plugin) { - const jsmntok_t *resulttok, *dynamictok; - bool dynamic_plugin; + const jsmntok_t *resulttok, *dynamictok, *earlyconftok; + bool dynamic_plugin, earlyconf_plugin; /* Check if all plugins have replied to getmanifest, and break * if they have and this is the startup init */ @@ -832,6 +840,12 @@ static void plugin_manifest_cb(const char *buffer, if (dynamictok && json_to_bool(buffer, dynamictok, &dynamic_plugin)) plugin->dynamic = dynamic_plugin; + earlyconftok = json_get_member(buffer, resulttok, "early_conf"); + if (earlyconftok && json_to_bool(buffer, earlyconftok, &earlyconf_plugin)) + plugin->early_config = earlyconf_plugin; + else + plugin->early_config = false; + if (!plugin_opts_add(plugin, buffer, resulttok) || !plugin_rpcmethods_add(plugin, buffer, resulttok) || !plugin_subscriptions_add(plugin, buffer, resulttok) || @@ -1027,6 +1041,15 @@ static void plugin_config_cb(const char *buffer, struct plugin *plugin) { plugin->plugin_state = CONFIGURED; + + if (plugin->plugins->startup && plugin->early_config) { + plugin->plugins->pending_early_configs--; + tal_free(plugin->timeout_timer); + /* This will break the io_loop in plugins_early_config */ + if (plugin->plugins->pending_early_configs == 0) + io_break(plugin->plugins); + } + } /* FIXME(cdecker) This just builds a string for the request because @@ -1061,6 +1084,31 @@ static void plugin_config(struct plugin *plugin) jsonrpc_request_end(req); plugin_request_send(plugin, req); + + /* Wait for early_config plugin's `init` response during startup, so they + * can do their thing or abort at an early stage */ + if (plugin->plugins->startup && plugin->early_config) { + plugin->plugins->pending_early_configs++; + /* FIXME assert plugin->timeout_timer == NULL here ? */ + plugin->timeout_timer = new_reltimer(plugin->plugins->ld->timers, + plugin, time_from_sec(PLUGIN_CONFIG_TIMEOUT), + plugin_early_conf_timeout, plugin); + } +} + +void plugins_early_config(struct plugins *plugins) +{ + struct plugin *p; + list_for_each(&plugins->plugins, p, list) { + if (p->early_config && p->plugin_state == UNCONFIGURED) { + p->plugin_state = CONFIGURING; + plugin_config(p); + } + } + /* At start-up, wait for early_config plugins to initialize */ + if (plugins->pending_early_configs > 0) + io_loop_with_timers(plugins->ld); + } void plugins_config(struct plugins *plugins) diff --git a/lightningd/plugin.h b/lightningd/plugin.h index 8787b62379f5..5dff969a98e9 100644 --- a/lightningd/plugin.h +++ b/lightningd/plugin.h @@ -32,6 +32,8 @@ struct plugin { /* If this plugin can be restarted without restarting lightningd */ bool dynamic; bool signal_startup; + /* Plugin initializes early, blocking lightningd, but is very restricted */ + bool early_config; /* Stuff we read */ char *buffer; @@ -65,6 +67,7 @@ struct plugin { struct plugins { struct list_head plugins; size_t pending_manifests; + size_t pending_early_configs; bool startup; /* Currently pending requests by their request ID */ @@ -159,6 +162,14 @@ void PRINTF_FMT(2,3) plugin_kill(struct plugin *plugin, char *fmt, ...); * incoming JSON-RPC calls and messages. */ void plugins_config(struct plugins *plugins); + +/** + * Sync'ed version of above, only for some very *limited* plugins that have set + * `early_config` in their getmanifest response to indicate they want to be + * initialized early. + */ +void plugins_early_config(struct plugins *plugins); + /** * Add the plugin option and their respective options to listconfigs. * diff --git a/lightningd/test/run-find_my_abspath.c b/lightningd/test/run-find_my_abspath.c index 649b0f4a7532..2c4046550395 100644 --- a/lightningd/test/run-find_my_abspath.c +++ b/lightningd/test/run-find_my_abspath.c @@ -140,6 +140,9 @@ void per_peer_state_set_fds_arr(struct per_peer_state *pps UNNEEDED, const int * /* Generated stub for plugins_config */ void plugins_config(struct plugins *plugins UNNEEDED) { fprintf(stderr, "plugins_config called!\n"); abort(); } +/* Generated stub for plugins_early_config */ +void plugins_early_config(struct plugins *plugins UNNEEDED) +{ fprintf(stderr, "plugins_early_config called!\n"); abort(); } /* Generated stub for plugins_init */ void plugins_init(struct plugins *plugins UNNEEDED, const char *dev_plugin_debug UNNEEDED) { fprintf(stderr, "plugins_init called!\n"); abort(); } From ddd70640ad39e6c166b73ca616eb993c06f2d465 Mon Sep 17 00:00:00 2001 From: Simon Vrouwe Date: Tue, 27 Aug 2019 15:12:27 +0300 Subject: [PATCH 2/4] pylightning/plugin: add `early_conf` field and init `startup` to None --- contrib/pylightning/lightning/plugin.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/contrib/pylightning/lightning/plugin.py b/contrib/pylightning/lightning/plugin.py index 1f2ff0e12d72..400951bb2279 100644 --- a/contrib/pylightning/lightning/plugin.py +++ b/contrib/pylightning/lightning/plugin.py @@ -99,7 +99,7 @@ class Plugin(object): """ - def __init__(self, stdout=None, stdin=None, autopatch=True, dynamic=True): + def __init__(self, stdout=None, stdin=None, autopatch=True, dynamic=True, early_conf=False): self.methods = {'init': Method('init', self._init, MethodType.RPCMETHOD)} self.options = {} @@ -121,8 +121,9 @@ def __init__(self, stdout=None, stdin=None, autopatch=True, dynamic=True): self.rpc_filename = None self.lightning_dir = None self.rpc = None - self.startup = True + self.startup = None self.dynamic = dynamic + self.early_conf = early_conf self.child_init = None self.write_lock = RLock() @@ -527,7 +528,8 @@ def _getmanifest(self, **kwargs): 'rpcmethods': methods, 'subscriptions': list(self.subscriptions.keys()), 'hooks': hooks, - 'dynamic': self.dynamic + 'dynamic': self.dynamic, + 'early_conf': self.early_conf } def _init(self, options, configuration, request): From e7441ecfa42c784b325e5b4bc78e2ca19aa00e6d Mon Sep 17 00:00:00 2001 From: Simon Vrouwe Date: Tue, 27 Aug 2019 15:37:49 +0300 Subject: [PATCH 3/4] lightningd/plugin: handle plugin init errors Allow a plugin to have itself killed cleanly by lightningd when it returns an init-error. Or abort the lightningd startup in case it is an `early_conf` plugin. TODO: - check `json` id - add error code + message --- lightningd/plugin.c | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/lightningd/plugin.c b/lightningd/plugin.c index d909d9ead433..5e6a4ea2739f 100644 --- a/lightningd/plugin.c +++ b/lightningd/plugin.c @@ -1021,6 +1021,7 @@ void plugins_start(struct plugins *plugins, const char *dev_plugin_debug) void plugins_init(struct plugins *plugins, const char *dev_plugin_debug) { plugins->pending_manifests = 0; + plugins->pending_early_configs = 0; uintmap_init(&plugins->pending_requests); plugins_add_default_dir(plugins, @@ -1040,16 +1041,45 @@ static void plugin_config_cb(const char *buffer, const jsmntok_t *idtok, struct plugin *plugin) { - plugin->plugin_state = CONFIGURED; + const jsmntok_t *errortok; + /* An init-error during startup from an `early_conf` plugin will crash + * lightningd, outside startup it will kill the plugin (any type). */ + errortok = json_get_member(buffer, toks, "error"); + /* FIXME: shouldn't this be a JSMN_OBJECT ? */ + if (errortok && errortok->type != JSMN_STRING) { + /* FIXME: should `static` plugins be treated same way?*/ + if (plugin->plugins->startup && plugin->early_config) + fatal("init error at early config of plugin '%s':" + " \"error\" result is not an object: %.*s", plugin->cmd, + toks[0].end - toks[0].start, + buffer + toks[0].start); + + /* Plugins that don't like to be started at run-time, can return an + * init error ... and be killed cleanly */ + plugin_kill(plugin, + "\"error\" result is not an object: %.*s", + toks[0].end - toks[0].start, + buffer + toks[0].start); + } + /* TODO: add error code + message + * TODO: check json `id` */ if (plugin->plugins->startup && plugin->early_config) { + if (errortok) + fatal("init error at early config of plugin '%s'", plugin->cmd); plugin->plugins->pending_early_configs--; tal_free(plugin->timeout_timer); + /* This will break the io_loop in plugins_early_config */ if (plugin->plugins->pending_early_configs == 0) io_break(plugin->plugins); } + if (errortok) { + plugin_kill(plugin, "error initializing plugin"); + return; + } + plugin->plugin_state = CONFIGURED; } /* FIXME(cdecker) This just builds a string for the request because From 8c1fc0fe0246773187adfccaa18ed0dfa6771dda Mon Sep 17 00:00:00 2001 From: Simon Vrouwe Date: Sat, 31 Aug 2019 20:23:23 +0300 Subject: [PATCH 4/4] lightningd/plugin: kill static and early_conf plugins, when freshly started via RPC Increase PLUGIN_CONFIG_TIMEOUT to 60, the max time needed by `early_conf` plugin's init, for example to compare database with a backup. fixup test to pass Travis, but not complete yet --- lightningd/plugin.c | 24 ++++++++++++++++-------- tests/test_plugin.py | 10 +++++++--- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/lightningd/plugin.c b/lightningd/plugin.c index 5e6a4ea2739f..e42f4491c1e9 100644 --- a/lightningd/plugin.c +++ b/lightningd/plugin.c @@ -33,7 +33,7 @@ * `getmanifest` call anyway, that's what `init `is for. */ #define PLUGIN_MANIFEST_TIMEOUT 60 /* Timeout for the init response of early_config plugins */ -#define PLUGIN_CONFIG_TIMEOUT 10 +#define PLUGIN_CONFIG_TIMEOUT 60 struct plugins *plugins_new(const tal_t *ctx, struct log_book *log_book, struct lightningd *ld) @@ -839,6 +839,8 @@ static void plugin_manifest_cb(const char *buffer, dynamictok = json_get_member(buffer, resulttok, "dynamic"); if (dynamictok && json_to_bool(buffer, dynamictok, &dynamic_plugin)) plugin->dynamic = dynamic_plugin; + else + plugin->dynamic = true; earlyconftok = json_get_member(buffer, resulttok, "early_conf"); if (earlyconftok && json_to_bool(buffer, earlyconftok, &earlyconf_plugin)) @@ -846,13 +848,19 @@ static void plugin_manifest_cb(const char *buffer, else plugin->early_config = false; - if (!plugin_opts_add(plugin, buffer, resulttok) || - !plugin_rpcmethods_add(plugin, buffer, resulttok) || - !plugin_subscriptions_add(plugin, buffer, resulttok) || - !plugin_hooks_add(plugin, buffer, resulttok)) - plugin_kill( - plugin, - "Failed to register options, methods, hooks, or subscriptions."); + /* A previously unloaded plugin can indicate us it doesn't want to be + * started-via-RPC, then kill it and don't registers hooks etc.*/ + if (!plugin->plugins->startup + && (!plugin->dynamic || plugin->early_config)) { + plugin_kill(plugin, "because plugin config%s%s", + !plugin->dynamic ? " dynamic=True" : "", + plugin->early_config ? " early_conf=True" : ""); + } else if (!plugin_opts_add(plugin, buffer, resulttok) || + !plugin_rpcmethods_add(plugin, buffer, resulttok) || + !plugin_subscriptions_add(plugin, buffer, resulttok) || + !plugin_hooks_add(plugin, buffer, resulttok)) + plugin_kill( plugin, + "Failed to register options, methods, hooks, or subscriptions."); /* If all plugins have replied to getmanifest and this is not * the startup init, configure them */ diff --git a/tests/test_plugin.py b/tests/test_plugin.py index 4ab2fceb6cb2..01a5bbabb36f 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -146,12 +146,16 @@ def test_plugin_command(node_factory): cmd = [hlp for hlp in n.rpc.help()["help"] if "hello" in hlp["command"]] assert(len(cmd) == 0) + # Test that a static plugin is killed when started via RPC + n.rpc.plugin_start(plugin=os.path.join(os.getcwd(), "tests/plugins/static.py")) + n.daemon.wait_for_log(r"plugin-static.py Killing plugin: because plugin config dynamic=True") + assert not n.daemon.is_in_log(r"Static plugin initialized.", start=0) + # Test that we cannot stop a plugin with 'dynamic' set to False in # getmanifest - n.rpc.plugin_start(plugin=os.path.join(os.getcwd(), "tests/plugins/static.py")) - n.daemon.wait_for_log(r"Static plugin initialized.") + n2 = node_factory.get_node(options={"plugin": os.path.join(os.getcwd(), "tests/plugins/static.py")}) with pytest.raises(RpcError, match=r"plugin cannot be managed when lightningd is up"): - n.rpc.plugin_stop(plugin="static.py") + n2.rpc.plugin_stop(plugin="static.py") def test_plugin_disable(node_factory):