search

niftynei / glightning

A Golang driver for c-lightning. Includes both an RPC client and plugin framework
https://github.com/ElementsProject/lightning
Other
42 stars 16 forks source link

Can not unload plugin added with the `plugin` cli/rpc method #16

Closed rsbondi closed 5 years ago

rsbondi commented 5 years ago

Steps:

lightningd # start without plugin
lightning-cli plugin start path/to/myplugin

The plugin starts fine but calling lightning-cli plugin stop path/to/myplugin

gives error path/to/myplugin plugin cannot be managed when lightningd is up

I notice the dynamic flag has been added in the getmanifest call in the plugin docs of clightning. It should be added here, as currently implementation in clightning.

But I question the implementation in clightning. If a plugin is loaded via the plugin command, would it not make sense to set the dynamic flag at that time? Or if dynamic is not set, then it should not be allowed to be loaded via the plugin command? Maybe I should open an issue there?

cc @darosior

darosior commented 5 years ago

But I question the implementation in clightning. If a plugin is loaded via the plugin command, would it not make sense to set the dynamic flag at that time? Or if dynamic is not set, then it should not be allowed to be loaded via the plugin command? Maybe I should open an issue there?

Yes it would. However the manifest is generated plugin-side, I don't know if it's a good design to modify it lightningd-side. Since this issue is on the glightning repo I suggest you just amend the manifest generation here as you did in #13 :-).

You can anyway open an issue on the C-lightning repo, I'd be happy to quickly implement it (shouldn't be more than a couple of line to add) and discuss it with other contributors on a PR.

EDIT: I reviewed the code and found the rationale for this decision (but you at least made me found a 'bug' :-)). Actually the startup check is done only for the stop subcommand. A dynamically-started plugin might not want to be stopped and lightningd should not force the dynamic boolean to be set.

rsbondi commented 5 years ago

Well this is unreleased, so I think I will take the discussion there.

darosior commented 5 years ago

I edited my above comment. ping @rsbondi

rsbondi commented 5 years ago

Ok, that makes sense, but not sure if I would call it a bug, maybe it is ok to start any plugin dynamically, but not allow to stop, anyway, reopening

darosior commented 5 years ago

but not sure if I would call it a bug

What I called a 'bug' (between quotes) was that I noticed unused code while reading it, not related to this issue but it made me find it.

darosior commented 5 years ago

Anyway to address your concern you should add the field at manifest creation in glightning. It seems to be located here : https://github.com/niftynei/glightning/blob/7dc7d2d4d5b7aabd49d7ebfdcc7cb7b6e886aed6/glightning/plugin.go#L443-L445

niftynei commented 5 years ago

ACK. Correct, you can't unload a plugin that hasn't marked itself as dynamic. As of right now, glightning currently does not support marking a plugin as dynamic.

If not already, it's probably worthwhile to make the relationship between dynamic and unloading more clear in the PLUGINS doc on c-lightning.

rsbondi commented 5 years ago

I was actually thinking lightning-plugin needs more clarification, I think PLUGINS is fine. I ran into this from more of a user's perspective, not clear on what was supposed to happen or why.