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

JSON-RPC passthrough for plugin (Endless plugin saga Part 2) #2121

Merged
merged 16 commits into from Dec 2, 2018

Conversation

Projects
None yet
4 participants
@cdecker
Copy link
Member

cdecker commented Nov 28, 2018

This is the second major plugin feature: plugins can register RPC methods with lightningd's JSON-RPC interface, and calls to those methods will be transparently routed to the plugin.

The preparatory work requires wrapping the information related to the JSON-RPC in a new struct. This also gives us a clean root object for all related objects as well, which was so far done with the listener object. Furthermore the JSON-RPC instantiation and initialization (listening) was split so we can instantiate and add new methods before starting to listen, and the example plugin now handles exceptions and array-based as well as dict-based arguments correctly (I plan on formalizing this in a utility Plugin class for python plugins once we have plugin -> lightningd notifications for logging).

@cdecker cdecker self-assigned this Nov 28, 2018

@cdecker cdecker requested a review from rustyrussell Nov 28, 2018

@niftynei
Copy link
Collaborator

niftynei left a comment

Noted a few things that we should probably look to document in the How To for plugins. Otherwise looking good.

ld->rpc_listener = io_new_listener(ld->rpc_filename, fd, incoming_jcon_connected, ld);
jsonrpc->rpc_listener = io_new_listener(
ld->rpc_filename, fd, incoming_jcon_connected, ld);
log_debug(jsonrpc->log, "Listening on '%s'", ld->rpc_filename);

This comment has been minimized.

@niftynei

niftynei Nov 28, 2018

Collaborator

nice fix.

}

if (!desctok || desctok->type != JSMN_STRING) {
plugin_kill(plugin, tal_fmt(plugin,

This comment has been minimized.

@niftynei

niftynei Nov 28, 2018

Collaborator

meta comment that we should make sure we make clear in the docs that a description is mandatory


/* Find the plugin that registered this RPC call */
list_for_each(&plugins->plugins, plugin, list) {
for (size_t i=0; i<tal_count(plugin->methods); i++) {

This comment has been minimized.

@niftynei

niftynei Nov 28, 2018

Collaborator

If I'm reading this correctly, this means that every plugin will need to register independent method names, ie they should namespace their methods so there's no conflict with another plugin, otherwise you might call the wrong plugin! (probably the first one that registered that method name)

This comment has been minimized.

@renepickhardt

renepickhardt Nov 28, 2018

Collaborator

That is a very nice catch. However I believe forcing users of an API to do something like name spacing correctly will probably be hard to enforce.
Therefor I believe that c-lightning should handle the name spacing and not the plugin developer. In that sense within clightning there would be a namespace given by something like plugin_name::plugin->method

This comment has been minimized.

@rustyrussell

rustyrussell Nov 29, 2018

Contributor

As written, the first plugin "wins" and the others get killed for trying. That's fair (minimal) until we figure out how to handle it well.

Since I want to move "pay" to a plugin, I can't really advocate forced namespacing. I think "be careful" is OK for the moment: we might allow disambiguation like "plugin.method" later.

This comment has been minimized.

@cdecker

cdecker Nov 29, 2018

Member

I think it's fair to push the "multiplexing" of multiple plugins wanting to register the same method out to a "dispatch plugin", i.e., a plugin that knows how to route them, and not handle them internally.

@renepickhardt
Copy link
Collaborator

renepickhardt left a comment

just a general meta comment about the impact on the python RPC interface and how to make plugin functionallity available for that.

assert(len(cmd) == 1)

# Now try to call it and see what it returns:
greet = n.rpc.hello(name='Sun')

This comment has been minimized.

@renepickhardt

renepickhardt Nov 28, 2018

Collaborator

meta: how will developers that use the python RPC interface know that there is a new RPC-call available? Even if plugin developers would inharite from the LightningRPC class this would not help as this derived class is not shipped via pip3.

I believe the best way to handle this is to change the pylightning.py file in a way so that the available RPC calls provide methods dynamically (maybe via reflection?)

This comment has been minimized.

@rustyrussell

rustyrussell Nov 29, 2018

Contributor

Unknown rpc calls are handled in pylightning, but it's a bit obscure IMHO. You need to call it using a dict of options, not inline, IIRC....

This comment has been minimized.

@cdecker

cdecker Nov 29, 2018

Member

We can certainly make pylightning a bit smarter to accept either all list arguments or all keyword arguments. I'll note that for the next cleanup PR.

@rustyrussell
Copy link
Contributor

rustyrussell left a comment

I approve the general approach and idea; very nice!

A few minor cleanups along the way; probably worth a rebase to address them. Esp the one patch which already seems to contain unrelated fixups...

return false;

tal_resize(&rpc->commands, count + 1);
rpc->commands[count] = command;

This comment has been minimized.

@rustyrussell

rustyrussell Nov 29, 2018

Contributor

*tal_arr_expand(&rpc->commands) = command;?

Or maybe = tal_steal(rpc, command); depending on how it's used...

This comment has been minimized.

@cdecker

cdecker Nov 29, 2018

Member

That is one weird syntax imho 😉

Thanks for pointing that out, I was wondering how to call it. Fixup incoming.

&& json_tok_streq(buffer, tok, cmdlist[i]->name))
return cmdlist[i];
/* commands[i]->name can be NULL in test code. */
for (size_t i=0; i<tal_count(commands); i++)

This comment has been minimized.

@rustyrussell

rustyrussell Nov 29, 2018

Contributor

I wonder if this comment is still true?

This comment has been minimized.

@cdecker

cdecker Nov 29, 2018

Member

Well they can now be null because we destroyed a plugin :-) So maybe we need to amend the comment.

This comment has been minimized.

@cdecker

cdecker Nov 29, 2018

Member

The comment has been amended in a later commit to reflect this.

plugin,
tal_fmt(plugin,
"rpcmethod does not have a string \"name\": %.*s",
meth->end - meth->start, buffer + meth->start));

This comment has been minimized.

@rustyrussell

rustyrussell Nov 29, 2018

Contributor

You do this a few times; better make plugin_kill take a format string directly?

This comment has been minimized.

@cdecker

cdecker Nov 29, 2018

Member

Added a new commit to the end.

return false;
}

cmd = notleak(tal(plugin, struct json_command));

This comment has been minimized.

@rustyrussell

rustyrussell Nov 29, 2018

Contributor

notleak() should be unnecessary here if memleak walks plugin list correctly?

This comment has been minimized.

@cdecker

cdecker Nov 29, 2018

Member

The problem is that json_command is not a list-element, since it is one of those structs that we instantiate inline in code in some places. So we'd need to keep an array of pointers and then manually remove them when checking memleak memleak. If desired I can do that in the next cleanup round 😉

for (cur = methods + 1; cur->parent == methpos; cur = json_next(cur))
ok &= plugin_rpcmethod_add(req->plugin, buffer, cur);

return ok;

This comment has been minimized.

@rustyrussell

rustyrussell Nov 29, 2018

Contributor

I generally consider it a bug to return with no caller paying attention to the return value. In this case, it's probably buggy because we kill the plugin the first plugin_rpcmethod_add, so we should terminate the loop?

This comment has been minimized.

@cdecker

cdecker Nov 29, 2018

Member

Nice catch, I add a fixup that exits the loop on the first error, logs a message about the cause and kills the plugin instead of keeping it limping along.


/* Find the plugin that registered this RPC call */
list_for_each(&plugins->plugins, plugin, list) {
for (size_t i=0; i<tal_count(plugin->methods); i++) {

This comment has been minimized.

@rustyrussell

rustyrussell Nov 29, 2018

Contributor

As written, the first plugin "wins" and the others get killed for trying. That's fair (minimal) until we figure out how to handle it well.

Since I want to move "pay" to a plugin, I can't really advocate forced namespacing. I think "be careful" is OK for the moment: we might allow disambiguation like "plugin.method" later.

for (size_t i=0; i<tal_count(plugin->methods); i++) {
if (streq(request->method, plugin->methods[i])) {
request->plugin = plugin;
break;

This comment has been minimized.

@rustyrussell

rustyrussell Nov 29, 2018

Contributor

This break is a bit suboptimal, since it doesn't break out of the outer loop.

Cleanest is extract this code into 'find_plugin_for_cmd()', and have this return plugin;.

This comment has been minimized.

@cdecker

cdecker Nov 29, 2018

Member

Added fixup that uses a goto instead.

* cleanup after dying */
assert(request->plugin);

tal_steal(request->plugin, request);

This comment has been minimized.

@rustyrussell

rustyrussell Nov 29, 2018

Contributor

I wonder what happens if plugin dies while this request is still pending?

This comment has been minimized.

@cdecker

cdecker Nov 29, 2018

Member

The plugin should get freed, cleaning up the connections and all pending requests. I don't currently plan on restarting failed plugins.

req->response + res->start);
else
json_add_member(response, NULL, "%.*s", res->end - res->start,
req->response + res->start);

This comment has been minimized.

@rustyrussell

rustyrussell Nov 29, 2018

Contributor

BTW, there are json_tok_contents and json_tok_len for exactly this "strings are special" case:

     json_add_member(response, NULL, "%.*s", json_tok_len(t), json_tok_contents(t));

This comment has been minimized.

@cdecker

cdecker Nov 29, 2018

Member

Thanks, will do a pass and replace where necessary.

This comment has been minimized.

@cdecker

cdecker Nov 29, 2018

Member

Turns out this is the only case this is necessary, so I added a fixup 👍

assert(len(cmd) == 1)

# Now try to call it and see what it returns:
greet = n.rpc.hello(name='Sun')

This comment has been minimized.

@rustyrussell

rustyrussell Nov 29, 2018

Contributor

Unknown rpc calls are handled in pylightning, but it's a bit obscure IMHO. You need to call it using a dict of options, not inline, IIRC....

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Nov 29, 2018

I think I addressed all the current feedback. With the notleak fix for json_commands registered by the plugins postponed to a cleanup PR later since that'd be a new commit on top anyway.

The fixups don't apply cleanly (there are 2 that touch code that is adjacent to a later commit which git can't handle automatically), so I'm happy to rebase & squash once everybody has given the all-clear on the fixups.

as the name was not previously registered. This includes both built-in
methods, such as `help` and `getinfo`, as well as methods registered
by other plugins. If there is a conflict then `lightningd` will report
an error and exit.

This comment has been minimized.

@niftynei

niftynei Nov 29, 2018

Collaborator

👍

@cdecker cdecker force-pushed the plugin-3 branch from b0e1f6e to f5efc0d Nov 29, 2018

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Nov 29, 2018

Hunted down a stupid memory issue that valgrind was complaining about.

@cdecker cdecker force-pushed the plugin-3 branch from f5efc0d to 61fca54 Nov 29, 2018

}
}
}

found:
/* This should never happen, it'd mean that a plugin didn't
* cleanup after dying */
assert(request->plugin);

This comment has been minimized.

@rustyrussell

rustyrussell Nov 29, 2018

Contributor

Hmm, prefer found: below this, and make the assert() and abort().

@rustyrussell

This comment has been minimized.

Copy link
Contributor

rustyrussell commented Nov 30, 2018

GH decided to "hide" two of my comments (maybe I commented on code which was replaced in a later patch?). Quoting here: in particular, this means there's no dangling NULLs for plugins:

Let's add some code to utils.h for this case... UNTESTED CODE FOLLOWS

#define tal_arr_remove(p, n) tal_arr_remove_((p), sizeof(**p), (n))

void tal_arr_remove_(void *p, size_t elemsize, size_t n)
{
    // p is a pointer-to-pointer for tal_resize.
    char *objp = *(char **)p;
    size_t len = tal_bytelen(objp);
    assert(len % elemsize == 0);
    assert((n + 1) * elemsize <= len);
    memmove(objp + elemsize * n, objp + elemsize * (n+1),
                      len - (elemsize * (n+1)));
    tal_resize((char **)p, len - elemsize);
}
@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Nov 30, 2018

I had seen that comment, and then forgot about it 😟

Since we now have a conflict and the series doesn't squash cleanly I'll rebase and squash, before adding new fixups on top.

cdecker added some commits Nov 26, 2018

plugin: Fix memory leak when requests are done
We weren't cleaning the requests we fulfilled, so this does that :-)
plugin: Allow both array as well as object params in example plugin
Signed-off-by: Christian Decker <decker.christian@gmail.com>
plugin: The example plugin can now return errors
Signed-off-by: Christian Decker <decker.christian@gmail.com>
jsonrpc: Make an explicit jsonrpc struct
This wraps the listener, a separate log and the registered
commands. This is mainly needed once we dynamically add
sjson_command`s to the JSON-RPC.
jsonrpc: Split the jsonrpc object creation from starting to listen
This is needed in order to be able to add methods while initializing
the plugins, but before actually moving to the config dir and starting
to listen.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
plugin: Remove added JSON-RPC methods if a plugin gets killed
Removes the method from the dispatch table, leaving a NULL entry
currently.

Signed-off-by: Christian Decker <@cdecker>
plugin: Plugins need a list of methods they registered
This will be used in the next commit to dispatch calls to the correct
plugin.

Signed-off-by: Christian Decker <@cdecker>
plugin: Make memleak happy
List element structs must have the list_node as their first element.

Signed-off-by: Christian Decker <@cdecker>
plugin: Map results back to the incoming JSON-RPC request
The final step in the JSON-RPC passthrough: map the result we got from
the plugin back to the original request we got from the client.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
pytest: Add a test for the JSON-RPC passthrough
Tests JSON-RPC registration, as well as success and failures.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
plugin: Update documentation of the rpcmethods
Suggested-by: Lisa Neigut <@niftynei>
Signed-off-by: Christian Decker <decker.christian@gmail.com>
plugin: Make plugin_kill a printf-like function
Suggested-by: Rusty Russell <@rustyrussell>
Signed-off-by: Christian Decker <@cdecker>
plugin: Iterate over the options from a plugin using the tok->size
I had this really contorted way of iterating over options that could
cause valgrind to choke. This is the much more intuitive way to
iterate.

Signed-off-by: Christian Decker <decker.christian@gmail.com>

@cdecker cdecker force-pushed the plugin-3 branch from 61fca54 to 01e555a Nov 30, 2018

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Nov 30, 2018

Ok, force pushed the rebase. The The reviews should be good up to 01e555a (see the diff is here, it contains the changes from #2118 only).

Adding a new commit for @rustyrussell proposed tal_arr_remove on top.

The range-diff comparing pre-rebase and post-rebase can be found here, it basically just rolls in the fixups.

@rustyrussell

This comment has been minimized.

Copy link
Contributor

rustyrussell commented Dec 2, 2018

Ack 01e555a

@rustyrussell rustyrussell merged commit d7e94a9 into master Dec 2, 2018

3 checks passed

ackbot PR ack'd by rustyrussell
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment