Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor dynamic plugins #3062

Merged
merged 14 commits into from Sep 30, 2019

Conversation

@darosior
Copy link
Collaborator

commented Sep 15, 2019

#2996, the discussion in #3013, and the lightningd crash due to dynamic plugin errors as mentioned by @jb55 on IRC (,and some plugin developers not that fine with the current implementation,) made me think that I was wrong in how I implemented dynamic plugins. Since we had to fix the dynamic plugins crashes anyway, I rewrited it all to separate runtime paths of dynamic plugins from startup (and potentially critic) plugins at the expense of some code duplication.

This PR :

  • removes dynamic-plugins-specific logic from lightningd/plugin.
  • removes the middle state were plugins could end up on error : at startup they are either started or killed, via RPC they are either started or killed.
  • removes the possibility to start non-dynamic plugins.
  • makes the RPC commands wait for the startup to end : if you run plugin start X you don't have to poll plugin list to know if your plugin has been started or ... (??).
@darosior darosior requested a review from cdecker as a code owner Sep 15, 2019
@darosior darosior force-pushed the darosior:refactor_dynamic_plugins branch 2 times, most recently from b71fdea to 9827432 Sep 15, 2019
@jb55

This comment has been minimized.

Copy link
Collaborator

commented Sep 15, 2019

Concept ACK!

 _____________________ 
< ACK commit 092b2384 >
 --------------------- 
        \   ^__^
         \  (oo)\_______
            (__)\       )\/\
                ||----w |
                ||     ||
@jb55

This comment has been minimized.

Copy link
Collaborator

commented Sep 15, 2019

removes the possibility to start non-dynamic plugins

what does this mean

@darosior

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 16, 2019

removes the possibility to start non-dynamic plugins

what does this mean

Before this the lock was only on plugin stop, which means you could dynamically start a plugin not signaling dynamism, but you could not stop it.

@darosior darosior force-pushed the darosior:refactor_dynamic_plugins branch 7 times, most recently from e645b70 to b5ea0ea Sep 16, 2019
@darosior

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 17, 2019

Rebased and increased starting timeout in test so that the Travis Valgrind test pass.

@darosior darosior force-pushed the darosior:refactor_dynamic_plugins branch from b5ea0ea to 3cc4154 Sep 17, 2019
@cdecker

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

Seems test_plugin_slowinit isn't happy:

lightningd: FATAL SIGNAL 6 (version 67622f2)
0x451616 send_backtrace
	common/daemon.c:40
0x451bd2 crashdump
	common/daemon.c:53
0x7f1fb2423f1f ???
	???:0
0x7f1fb2423e97 ???
	???:0
0x7f1fb2425800 ???
	???:0
0x4b61f4 call_error
	ccan/ccan/tal/tal.c:93
0x4b6c7b check_bounds
	ccan/ccan/tal/tal.c:165
0x4b5115 to_tal_hdr
	ccan/ccan/tal/tal.c:174
0x4b4c42 to_tal_hdr_or_null
	ccan/ccan/tal/tal.c:186
0x4b5d34 tal_bytelen
	ccan/ccan/tal/tal.c:632
0x458377 memleak_remove_intmap_
	common/memleak.c:145
0x442000 memleak_help_pending_requests
	lightningd/plugin.c:28
0x4589b8 call_memleak_helpers
	common/memleak.c:233
0x458a5d call_memleak_helpers
	common/memleak.c:242
0x458a5d call_memleak_helpers
	common/memleak.c:242
0x458713 memleak_enter_allocations
	common/memleak.c:258
0x42595a scan_mem
	lightningd/memdump.c:157
0x4258fe report_leak_info2
	lightningd/memdump.c:204
0x45e872 timer_expired
	common/timeout.c:39
0x41ae0d io_loop_with_timers
	lightningd/io_loop_with_timers.c:32
0x420f32 main
	lightningd/lightningd.c:835
0x7f1fb2406b96 ???
	???:0
0x403dd9 ???
	???:0
0xffffffffffffffff ???
	???:0

Might also be a latent bug that just surfaced now :-)

@darosior

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 18, 2019

It's weird.. I can't reproduce locally even by tweaking the timeout and slowinit's sleep..
EDIT: lowering the sleep duration in slowinit's init solved this.

@darosior darosior force-pushed the darosior:refactor_dynamic_plugins branch from 3cc4154 to 6034bfc Sep 18, 2019
@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2019

Looks like we free req somewhere without removing it from plugins->pending_requests?

Copy link
Contributor

left a comment

Love the cleanup, but I think it's got some races which need more surgery.

lightningd/plugin_control.c Outdated Show resolved Hide resolved
lightningd/plugin_control.c Show resolved Hide resolved
/* Delay to avoid a race condition with command_still_pending */
new_reltimer(cmd->ld->timers, cmd,
time_from_sec(1),
plugin_dynamic_rescan_plugins, cmd);

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Sep 19, 2019

Contributor

This is a strong sign of a bug. I suspect related to your lack of reference counting.

This comment has been minimized.

Copy link
@darosior

darosior Sep 19, 2019

Author Collaborator

Why ? I thought that was because command_still_pending was executed after the command_success.

This comment has been minimized.

Copy link
@ZmnSCPxj

ZmnSCPxj Sep 19, 2019

Collaborator

I think @darosior is correct. I had to do this a number of times long ago in my payalgo.c predecessor to the pay plugin. Caller of some utility function (which expects a callback rather than returns directly itself) calls command_still_pending, then we need to trigger command_success inside the callback, and the utility function is able to determine this condition (need to call the callback) immediately without deferring the callback. Only way to get around that was to pointlessly delay.

Though it should be fine to use time_from_sec(0) in that case instead of pointlessly delaying by 1 second.

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Sep 26, 2019

Contributor

I experienced this too, but it usually happens because you haven't plumbed through the command_result.

This seems to work:

diff --git a/lightningd/plugin_control.c b/lightningd/plugin_control.c
index 44a56782c..857e0b891 100644
--- a/lightningd/plugin_control.c
+++ b/lightningd/plugin_control.c
@@ -10,7 +10,7 @@ struct dynamic_plugin {
 /**
  * Returned by all subcommands on success.
  */
-static void plugin_dynamic_list_plugins(struct command *cmd)
+static struct command_result *plugin_dynamic_list_plugins(struct command *cmd)
 {
 	struct json_stream *response;
 	struct plugin *p;
@@ -25,22 +25,23 @@ static void plugin_dynamic_list_plugins(struct command *cmd)
 		json_object_end(response);
 	}
 	json_array_end(response);
-	was_pending(command_success(cmd, response));
+	return command_success(cmd, response);
 }
 
 /**
  * Returned by all subcommands on error.
  */
-static void plugin_dynamic_error(struct dynamic_plugin *dp, const char *error)
+static struct command_result *plugin_dynamic_error(struct dynamic_plugin *dp,
+						   const char *error)
 {
 	plugin_kill(dp->plugin, "%s: %s", dp->plugin->cmd, error);
-	was_pending(command_fail(dp->cmd, JSONRPC2_INVALID_PARAMS,
-	                         "%s: %s", dp->plugin->cmd, error));
+	return command_fail(dp->cmd, JSONRPC2_INVALID_PARAMS,
+			    "%s: %s", dp->plugin->cmd, error);
 }
 
 static void plugin_dynamic_timeout(struct dynamic_plugin *dp)
 {
-	plugin_dynamic_error(dp, "Timed out while waiting for plugin response");
+	was_pending(plugin_dynamic_error(dp, "Timed out while waiting for plugin response"));
 }
 
 static void plugin_dynamic_config_callback(const char *buffer,
@@ -61,7 +62,7 @@ static void plugin_dynamic_config_callback(const char *buffer,
 	}
 
 	/* No plugin unconfigured left, return the plugin list */
-	plugin_dynamic_list_plugins(dp->cmd);
+	was_pending(plugin_dynamic_list_plugins(dp->cmd));
 }
 
 /**
@@ -85,11 +86,15 @@ static void plugin_dynamic_manifest_callback(const char *buffer,
                                              const jsmntok_t *idtok,
                                              struct dynamic_plugin *dp)
 {
-	if (!plugin_parse_getmanifest_response(buffer, toks, idtok, dp->plugin))
-		return plugin_dynamic_error(dp, "Gave a bad response to getmanifest");
+	if (!plugin_parse_getmanifest_response(buffer, toks, idtok, dp->plugin)) {
+		was_pending(plugin_dynamic_error(dp, "Gave a bad response to getmanifest"));
+		return;
+	}
 
-	if (!dp->plugin->dynamic)
-		return plugin_dynamic_error(dp, "Not a dynamic plugin");
+	if (!dp->plugin->dynamic) {
+		was_pending(plugin_dynamic_error(dp, "Not a dynamic plugin"));
+		return;
+	}
 
 	/* We got the manifest, now send the init message */
 	plugin_dynamic_config(dp);
@@ -99,7 +104,7 @@ static void plugin_dynamic_manifest_callback(const char *buffer,
  * This starts a plugin : spawns the process, connect its stdout and stdin,
  * then sends it a getmanifest request.
  */
-static void plugin_start(struct dynamic_plugin *dp)
+static struct command_result *plugin_start(struct dynamic_plugin *dp)
 {
 	int stdin, stdout;
 	char **p_cmd;
@@ -131,32 +136,33 @@ static void plugin_start(struct dynamic_plugin *dp)
 	                            plugin_dynamic_manifest_callback, dp);
 	jsonrpc_request_end(req);
 	plugin_request_send(p, req);
+	return command_still_pending(dp->cmd);
 }
 
 /**
  * Called when trying to start a plugin through RPC, it starts the plugin and
  * will give a result 20 seconds later at the most.
  */
-static void plugin_dynamic_start(struct command *cmd, const char *plugin_path)
+static struct command_result *plugin_dynamic_start(struct command *cmd,
+						   const char *plugin_path)
 {
 	struct dynamic_plugin *dp;
 
 	dp = tal(cmd, struct dynamic_plugin);
 	dp->cmd = cmd;
 	dp->plugin = plugin_register(cmd->ld->plugins, plugin_path);
-	if (!dp->plugin) {
-		plugin_dynamic_error(dp, "Is already registered");
-		return;
-	}
+	if (!dp->plugin)
+		return plugin_dynamic_error(dp, "Is already registered");
 
-	plugin_start(dp);
+	return plugin_start(dp);
 }
 
 /**
  * Called when trying to start a plugin directory through RPC, it registers
  * all contained plugins recursively and then starts them.
  */
-static void plugin_dynamic_startdir(struct command *cmd, const char *dir_path)
+static struct command_result *plugin_dynamic_startdir(struct command *cmd,
+						      const char *dir_path)
 {
 	const char *err;
 	struct plugin *p;
@@ -165,7 +171,7 @@ static void plugin_dynamic_startdir(struct command *cmd, const char *dir_path)
 
 	err = add_plugin_dir(cmd->ld->plugins, dir_path, false);
 	if (err)
-		return was_pending(command_fail(cmd, JSONRPC2_INVALID_PARAMS, "%s", err));
+		return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "%s", err);
 
 	found = false;
 	list_for_each(&cmd->ld->plugins->plugins, p, list) {
@@ -178,7 +184,9 @@ static void plugin_dynamic_startdir(struct command *cmd, const char *dir_path)
 		}
 	}
 	if (!found)
-		plugin_dynamic_list_plugins(cmd);
+		return plugin_dynamic_list_plugins(cmd);
+	else
+		return command_still_pending(cmd);
 }
 
 static struct command_result *
@@ -211,7 +219,7 @@ plugin_dynamic_stop(struct command *cmd, const char *plugin_name)
 /**
  * Look for additions in the default plugin directory.
  */
-static void plugin_dynamic_rescan_plugins(struct command *cmd)
+static struct command_result *plugin_dynamic_rescan_plugins(struct command *cmd)
 {
 	bool found;
 	struct plugin *p;
@@ -231,7 +239,9 @@ static void plugin_dynamic_rescan_plugins(struct command *cmd)
 	}
 
 	if (!found)
-		plugin_dynamic_list_plugins(cmd);
+		return plugin_dynamic_list_plugins(cmd);
+
+	return command_still_pending(cmd);
 }
 
 /**
@@ -270,7 +280,7 @@ static struct command_result *json_plugin_control(struct command *cmd,
 			return command_param_failed();
 
 		if (access(plugin_path, X_OK) == 0)
-			plugin_dynamic_start(cmd, plugin_path);
+			return plugin_dynamic_start(cmd, plugin_path);
 		else
 			return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
 						   "%s is not executable: %s",
@@ -285,7 +295,7 @@ static struct command_result *json_plugin_control(struct command *cmd,
 			return command_param_failed();
 
 		if (access(dir_path, F_OK) == 0)
-			plugin_dynamic_startdir(cmd, dir_path);
+			return plugin_dynamic_startdir(cmd, dir_path);
 		else
 			return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
 						   "Could not open %s", dir_path);
@@ -295,23 +305,17 @@ static struct command_result *json_plugin_control(struct command *cmd,
 			   NULL))
 			return command_param_failed();
 
-		/* Delay to avoid a race condition with command_still_pending */
-		new_reltimer(cmd->ld->timers, cmd,
-		             time_from_sec(0),
-		             plugin_dynamic_rescan_plugins, cmd);
+		return plugin_dynamic_rescan_plugins(cmd);
 	} else if (streq(subcmd, "list")) {
 		if (!param(cmd, buffer, params,
 			   p_req("subcommand", param_ignore, cmd),
 			   NULL))
 			return command_param_failed();
 
-		/* Delay to avoid a race condition with command_still_pending */
-		new_reltimer(cmd->ld->timers, cmd,
-		             time_from_sec(0),
-		             plugin_dynamic_list_plugins, cmd);
+		return plugin_dynamic_rescan_plugins(cmd);
 	}
-
-	return command_still_pending(cmd);
+	/* subcmd must be one of the above: param_subcommand checked it! */
+	abort();
 }
 
 static const struct json_command plugin_control_command = {

This comment has been minimized.

Copy link
@darosior

darosior Sep 26, 2019

Author Collaborator

Ok, thanks !

/* Delay to avoid a race condition with command_still_pending */
new_reltimer(cmd->ld->timers, cmd,
time_from_sec(1),
plugin_dynamic_list_plugins, cmd);

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Sep 19, 2019

Contributor

This too.

contrib/plugins/cowsay.sh Show resolved Hide resolved
lightningd/plugin_control.c Outdated Show resolved Hide resolved
@darosior darosior force-pushed the darosior:refactor_dynamic_plugins branch from 6034bfc to 77c3105 Sep 19, 2019
@darosior

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 19, 2019

Rebased to :

  • remove the use of pending_config and pending_manifest
  • lower the timeout from 1 to 0 in rescan and list subcommands
  • remove the configurable timeout parameter and pass the timeout to 20 seconds. I chose 20 instead of 30 as proposed by @rustyrussell because it's enough to make the tests pass under valgrind and if a plugin did not respond in the 20 seconds there high probability that it's broken and won't respond in the next 10 seconds.
darosior added 3 commits Sep 10, 2019
This reduces error details for getmanifest response,
but avoids too much code duplication in the next commit.
…lics
@darosior darosior force-pushed the darosior:refactor_dynamic_plugins branch from 77c3105 to 14799d5 Sep 19, 2019
@darosior

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 19, 2019

Corrected the doc accordingly to the removal of the configurable timeout, and rebased on top of master.

@darosior darosior force-pushed the darosior:refactor_dynamic_plugins branch from 14799d5 to 75086b7 Sep 19, 2019
Copy link
Contributor

left a comment

You also need this, AFAICT: I got a crash on memleak detection:

Traceback (most recent call last):
  File "/home/rusty/lightning-ltest/tests/plugins/broken.py", line 7, in <module>
    import an_unexistent_module_that_will_make_me_crash
ModuleNotFoundError: No module named 'an_unexistent_module_that_will_make_me_crash'
---------------------------------------- Captured stderr teardown ----------------------------------------
lightningd: FATAL SIGNAL 6 (version 75086b7-modded)
0x562244470b62 send_backtrace
	common/daemon.c:40
0x562244470c08 crashdump
	common/daemon.c:53
0x7fd0471f6f5f ???
	???:0
0x7fd0471f6ed7 ???
	???:0
0x7fd0471d8534 ???
	???:0
0x5622444ca7fa call_error
	ccan/ccan/tal/tal.c:93
0x5622444ca9c2 check_bounds
	ccan/ccan/tal/tal.c:165
0x5622444caa20 to_tal_hdr
	ccan/ccan/tal/tal.c:176
0x5622444caa82 to_tal_hdr_or_null
	ccan/ccan/tal/tal.c:186
0x5622444cb96a tal_bytelen
	ccan/ccan/tal/tal.c:632
0x5622444771e8 memleak_remove_intmap_
	common/memleak.c:145
0x56224446356c memleak_help_pending_requests
	lightningd/plugin.c:28
0x562244477559 call_memleak_helpers
	common/memleak.c:233
0x5622444775ee call_memleak_helpers
	common/memleak.c:242
0x5622444775ee call_memleak_helpers
	common/memleak.c:242
0x562244477683 memleak_enter_allocations
	common/memleak.c:258
0x56224444a2ee scan_mem
	lightningd/memdump.c:157
0x56224444a539 report_leak_info2
	lightningd/memdump.c:204
0x56224447ccbe timer_expired
	common/timeout.c:39
0x562244441267 io_loop_with_timers
	lightningd/io_loop_with_timers.c:32
0x5622444475f1 main
	lightningd/lightningd.c:835
diff --git a/lightningd/plugin.c b/lightningd/plugin.c
index 22103ae21..3ab22c82d 100644
--- a/lightningd/plugin.c
+++ b/lightningd/plugin.c
@@ -252,7 +252,6 @@ static void plugin_response_handle(struct plugin *plugin,
 	/* We expect the request->cb to copy if needed */
 	request->response_cb(plugin->buffer, toks, idtok, request->response_cb_arg);
 
-	uintmap_del(&plugin->plugins->pending_requests, id);
 	tal_free(request);
 }
 
@@ -1118,12 +1117,20 @@ void plugins_notify(struct plugins *plugins,
 		tal_free(n);
 }
 
+static void destroy_request(struct jsonrpc_request *req,
+			    struct plugin *plugin)
+{
+	uintmap_del(&plugin->plugins->pending_requests, req->id);
+}
+
 void plugin_request_send(struct plugin *plugin,
 			 struct jsonrpc_request *req TAKES)
 {
 	/* Add to map so we can find it later when routing the response */
 	tal_steal(plugin, req);
 	uintmap_add(&plugin->plugins->pending_requests, req->id, req);
+	/* Add destructor in case plugin dies. */
+	tal_add_destructor2(req, destroy_request, plugin);
 	plugin_send(plugin, req->stream);
 	/* plugin_send steals the stream, so remove the dangling
 	 * pointer here */
/* Delay to avoid a race condition with command_still_pending */
new_reltimer(cmd->ld->timers, cmd,
time_from_sec(1),
plugin_dynamic_rescan_plugins, cmd);

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Sep 26, 2019

Contributor

I experienced this too, but it usually happens because you haven't plumbed through the command_result.

This seems to work:

diff --git a/lightningd/plugin_control.c b/lightningd/plugin_control.c
index 44a56782c..857e0b891 100644
--- a/lightningd/plugin_control.c
+++ b/lightningd/plugin_control.c
@@ -10,7 +10,7 @@ struct dynamic_plugin {
 /**
  * Returned by all subcommands on success.
  */
-static void plugin_dynamic_list_plugins(struct command *cmd)
+static struct command_result *plugin_dynamic_list_plugins(struct command *cmd)
 {
 	struct json_stream *response;
 	struct plugin *p;
@@ -25,22 +25,23 @@ static void plugin_dynamic_list_plugins(struct command *cmd)
 		json_object_end(response);
 	}
 	json_array_end(response);
-	was_pending(command_success(cmd, response));
+	return command_success(cmd, response);
 }
 
 /**
  * Returned by all subcommands on error.
  */
-static void plugin_dynamic_error(struct dynamic_plugin *dp, const char *error)
+static struct command_result *plugin_dynamic_error(struct dynamic_plugin *dp,
+						   const char *error)
 {
 	plugin_kill(dp->plugin, "%s: %s", dp->plugin->cmd, error);
-	was_pending(command_fail(dp->cmd, JSONRPC2_INVALID_PARAMS,
-	                         "%s: %s", dp->plugin->cmd, error));
+	return command_fail(dp->cmd, JSONRPC2_INVALID_PARAMS,
+			    "%s: %s", dp->plugin->cmd, error);
 }
 
 static void plugin_dynamic_timeout(struct dynamic_plugin *dp)
 {
-	plugin_dynamic_error(dp, "Timed out while waiting for plugin response");
+	was_pending(plugin_dynamic_error(dp, "Timed out while waiting for plugin response"));
 }
 
 static void plugin_dynamic_config_callback(const char *buffer,
@@ -61,7 +62,7 @@ static void plugin_dynamic_config_callback(const char *buffer,
 	}
 
 	/* No plugin unconfigured left, return the plugin list */
-	plugin_dynamic_list_plugins(dp->cmd);
+	was_pending(plugin_dynamic_list_plugins(dp->cmd));
 }
 
 /**
@@ -85,11 +86,15 @@ static void plugin_dynamic_manifest_callback(const char *buffer,
                                              const jsmntok_t *idtok,
                                              struct dynamic_plugin *dp)
 {
-	if (!plugin_parse_getmanifest_response(buffer, toks, idtok, dp->plugin))
-		return plugin_dynamic_error(dp, "Gave a bad response to getmanifest");
+	if (!plugin_parse_getmanifest_response(buffer, toks, idtok, dp->plugin)) {
+		was_pending(plugin_dynamic_error(dp, "Gave a bad response to getmanifest"));
+		return;
+	}
 
-	if (!dp->plugin->dynamic)
-		return plugin_dynamic_error(dp, "Not a dynamic plugin");
+	if (!dp->plugin->dynamic) {
+		was_pending(plugin_dynamic_error(dp, "Not a dynamic plugin"));
+		return;
+	}
 
 	/* We got the manifest, now send the init message */
 	plugin_dynamic_config(dp);
@@ -99,7 +104,7 @@ static void plugin_dynamic_manifest_callback(const char *buffer,
  * This starts a plugin : spawns the process, connect its stdout and stdin,
  * then sends it a getmanifest request.
  */
-static void plugin_start(struct dynamic_plugin *dp)
+static struct command_result *plugin_start(struct dynamic_plugin *dp)
 {
 	int stdin, stdout;
 	char **p_cmd;
@@ -131,32 +136,33 @@ static void plugin_start(struct dynamic_plugin *dp)
 	                            plugin_dynamic_manifest_callback, dp);
 	jsonrpc_request_end(req);
 	plugin_request_send(p, req);
+	return command_still_pending(dp->cmd);
 }
 
 /**
  * Called when trying to start a plugin through RPC, it starts the plugin and
  * will give a result 20 seconds later at the most.
  */
-static void plugin_dynamic_start(struct command *cmd, const char *plugin_path)
+static struct command_result *plugin_dynamic_start(struct command *cmd,
+						   const char *plugin_path)
 {
 	struct dynamic_plugin *dp;
 
 	dp = tal(cmd, struct dynamic_plugin);
 	dp->cmd = cmd;
 	dp->plugin = plugin_register(cmd->ld->plugins, plugin_path);
-	if (!dp->plugin) {
-		plugin_dynamic_error(dp, "Is already registered");
-		return;
-	}
+	if (!dp->plugin)
+		return plugin_dynamic_error(dp, "Is already registered");
 
-	plugin_start(dp);
+	return plugin_start(dp);
 }
 
 /**
  * Called when trying to start a plugin directory through RPC, it registers
  * all contained plugins recursively and then starts them.
  */
-static void plugin_dynamic_startdir(struct command *cmd, const char *dir_path)
+static struct command_result *plugin_dynamic_startdir(struct command *cmd,
+						      const char *dir_path)
 {
 	const char *err;
 	struct plugin *p;
@@ -165,7 +171,7 @@ static void plugin_dynamic_startdir(struct command *cmd, const char *dir_path)
 
 	err = add_plugin_dir(cmd->ld->plugins, dir_path, false);
 	if (err)
-		return was_pending(command_fail(cmd, JSONRPC2_INVALID_PARAMS, "%s", err));
+		return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "%s", err);
 
 	found = false;
 	list_for_each(&cmd->ld->plugins->plugins, p, list) {
@@ -178,7 +184,9 @@ static void plugin_dynamic_startdir(struct command *cmd, const char *dir_path)
 		}
 	}
 	if (!found)
-		plugin_dynamic_list_plugins(cmd);
+		return plugin_dynamic_list_plugins(cmd);
+	else
+		return command_still_pending(cmd);
 }
 
 static struct command_result *
@@ -211,7 +219,7 @@ plugin_dynamic_stop(struct command *cmd, const char *plugin_name)
 /**
  * Look for additions in the default plugin directory.
  */
-static void plugin_dynamic_rescan_plugins(struct command *cmd)
+static struct command_result *plugin_dynamic_rescan_plugins(struct command *cmd)
 {
 	bool found;
 	struct plugin *p;
@@ -231,7 +239,9 @@ static void plugin_dynamic_rescan_plugins(struct command *cmd)
 	}
 
 	if (!found)
-		plugin_dynamic_list_plugins(cmd);
+		return plugin_dynamic_list_plugins(cmd);
+
+	return command_still_pending(cmd);
 }
 
 /**
@@ -270,7 +280,7 @@ static struct command_result *json_plugin_control(struct command *cmd,
 			return command_param_failed();
 
 		if (access(plugin_path, X_OK) == 0)
-			plugin_dynamic_start(cmd, plugin_path);
+			return plugin_dynamic_start(cmd, plugin_path);
 		else
 			return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
 						   "%s is not executable: %s",
@@ -285,7 +295,7 @@ static struct command_result *json_plugin_control(struct command *cmd,
 			return command_param_failed();
 
 		if (access(dir_path, F_OK) == 0)
-			plugin_dynamic_startdir(cmd, dir_path);
+			return plugin_dynamic_startdir(cmd, dir_path);
 		else
 			return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
 						   "Could not open %s", dir_path);
@@ -295,23 +305,17 @@ static struct command_result *json_plugin_control(struct command *cmd,
 			   NULL))
 			return command_param_failed();
 
-		/* Delay to avoid a race condition with command_still_pending */
-		new_reltimer(cmd->ld->timers, cmd,
-		             time_from_sec(0),
-		             plugin_dynamic_rescan_plugins, cmd);
+		return plugin_dynamic_rescan_plugins(cmd);
 	} else if (streq(subcmd, "list")) {
 		if (!param(cmd, buffer, params,
 			   p_req("subcommand", param_ignore, cmd),
 			   NULL))
 			return command_param_failed();
 
-		/* Delay to avoid a race condition with command_still_pending */
-		new_reltimer(cmd->ld->timers, cmd,
-		             time_from_sec(0),
-		             plugin_dynamic_list_plugins, cmd);
+		return plugin_dynamic_rescan_plugins(cmd);
 	}
-
-	return command_still_pending(cmd);
+	/* subcmd must be one of the above: param_subcommand checked it! */
+	abort();
 }
 
 static const struct json_command plugin_control_command = {
@darosior

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 26, 2019

I didnt encounter this memleak, thank you for the 2 patches !

darosior added 2 commits Sep 11, 2019
This remove the reliance on startup plugins' function "plugin_start" in
order to use a distinct runtime path for a dynamically started plugin,
which will allow startup plugins' code to be (almost) agnostic of
dynamic plugins.

This also makes the 'start' subcommand return only if the plugin is
either started, or killed : no weird middle state where the plugin
mishbehaving could crash lightningd.
This does the same as for the 'start' subcommand for the 'startdir'
one.

Note that we could fail to start the last plugin of a directory, but
have succesfully started the precedent plugins. This will make us return
an error to the user while some of the plugins have been started, but we
still don't end up in a transient state with
half-configured-half-errored plugins.
@darosior darosior force-pushed the darosior:refactor_dynamic_plugins branch from 75086b7 to e5493c1 Sep 26, 2019
darosior added 4 commits Sep 15, 2019
To keep only subcommands logic into the main
function.
…ugin

This merges back plugins_init and plugins_start, removes conditions on
startup, and removes the CONFIGURING state.
And make a function for the 'rescan' subcommand so that we are
consistent and have only subcommand logic in the main function,
which return command_still_pending() in any case (but 'stop').

patched-by: @rustyrussell
 __________________________________
< I was not plugin specs compliant >
 ----------------------------------
        \   ^__^
         \  (oo)\_______
            (__)\       )\/\
                ||----w |
                ||     ||
@darosior darosior force-pushed the darosior:refactor_dynamic_plugins branch from e5493c1 to bb8f41a Sep 26, 2019
@darosior

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 26, 2019

Rebased to aply @rustyrussell patches and to patch plugin_dynamic_stop to effectively free the plugin after having killed it.

darosior added 5 commits Sep 15, 2019
This adapts the test to the new 'plugin' command: no more sleeping,
since we are synchronous !

This tests the timeout by increasing the 'slowinit' plugin sleep
duration at init reception.

This adds a broken plugin to make sure we won't crash because of a
misbehaving plugin (unmet dependency is the most common case).
Authored-by: @rustyrussell
@darosior darosior force-pushed the darosior:refactor_dynamic_plugins branch from bb8f41a to 84b5b36 Sep 26, 2019
@darosior

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 26, 2019

Removed the sleep()s in the tests.

Copy link
Contributor

left a comment

Ack 84b5b36

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2019

Great work! Thanks!

@rustyrussell rustyrussell merged commit d0ccd15 into ElementsProject:master Sep 30, 2019
3 checks passed
3 checks passed
bitcoin-bot/acks Acks by rustyrussell
bitcoin-bot/fixups PR does not contain unsquashed fixups
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@darosior darosior deleted the darosior:refactor_dynamic_plugins branch Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.