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

Restore multiplexed JSONRPC command support #2092

Merged
merged 9 commits into from Nov 20, 2018

Conversation

Projects
None yet
3 participants
@rustyrussell
Copy link
Contributor

rustyrussell commented Nov 19, 2018

(On top of #2081)

This also opens the door to notifications, AFAICT.

@rustyrussell rustyrussell requested review from cdecker and ZmnSCPxj Nov 19, 2018

# They will return in the same order, since they start immediately
# (delaying completion should mean we don't see the other commands intermingled).
for i in commands:
obj, buff = l1.rpc._readobj(sock, buff)

This comment has been minimized.

@cdecker

cdecker Nov 19, 2018

Member

Probably should also assert that buff is empty here.

b'{"id":4,"jsonrpc":"2.0","method":"dev-slowcmd","params":[500]}',
b'{"id":4,"jsonrpc":"2.0","method":"dev-slowcmd","params":[500]}'
]
# no ID

This comment has been minimized.

@cdecker

cdecker Nov 19, 2018

Member

The comments seem to be misaligned with the actual tests.

This comment has been minimized.

@cdecker

cdecker Nov 19, 2018

Member

This looks like a good usecase for table driven testing: create a dict with the inputs as keys and the expected outcome as the values and then iterate through them.

@cdecker
Copy link
Member

cdecker left a comment

Nice PR, that should make it easier to build alternative uses for the JSON parser and writer.

I'm still a bit unclear about the relationship between struct command, struct json_connection and struct json_stream. In particular whi the json_stream needs to have a pointer to the command.

Also could you ELI5 how the concurrent reader and writer thing works for json_stream? It seems like we can concurrently write to the buffer while we're already flushing it to the io_conn is that correct? It seems however that unless we hand over control to the io_loop it still gets buffered in its entirety and we just io_wake the io_conn a number of times without effect (hence your comment about optimizing it), is that correct?

This would presumably be the case in which we loop over some other daemon's results incrementally reading it and writing it at the same time. Yielding control to io_loop to read the next chunk also gives the outgoing io_conn a chance to clear some of the buffer. Is that correct?

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Nov 19, 2018

Added 3 tiny fixups to see if I can make Travis happy :-)

@cdecker cdecker force-pushed the rustyrussell:guilt/json-multicmd branch 2 times, most recently from 3c4a79a to fadbb08 Nov 19, 2018

@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/json-multicmd branch from fadbb08 to a5642e4 Nov 20, 2018

@ZmnSCPxj
Copy link
Collaborator

ZmnSCPxj left a comment

Broadly approve, but my grasp of the code has eroded.

@@ -8,6 +8,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

### Added

- JSON API: use `\n\n` to terminate responses, for simplified parsing.

This comment has been minimized.

@ZmnSCPxj

ZmnSCPxj Nov 20, 2018

Collaborator

Maybe mention that the lightning RPC python now depends on this?

This comment has been minimized.

@rustyrussell

rustyrussell Nov 20, 2018

Contributor

Yep, will do.

@@ -0,0 +1,289 @@
#include <ccan/io/io.h>
/* To reach into io_plan: not a public header! */
#include <ccan/io/backend.h>

This comment has been minimized.

@ZmnSCPxj

ZmnSCPxj Nov 20, 2018

Collaborator

Ouch.

In any case, is the similar line in jsonrpc.c still necessary? Or should it have similar comment?

This comment has been minimized.

@rustyrussell

rustyrussell Nov 20, 2018

Contributor

Yes, it's ugly. Removing jsonrpc.c one now...


/* If we have a jcon, it will free result for us. */
if (cmd->jcon)
tal_steal(cmd->jcon, result);

This comment has been minimized.

@ZmnSCPxj

ZmnSCPxj Nov 20, 2018

Collaborator

If we do not have a jcon, does result get taken by this function?

This comment has been minimized.

@rustyrussell

rustyrussell Nov 20, 2018

Contributor

This function frees cmd a few lines below, so result goes with it...

rustyrussell added some commits Nov 20, 2018

CHANGELOG: note that pylightning uses double-newline.
Suggested-by: @ZmnSCPxj
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
json.c and jsonrpc.c: move functions between them.
json_stream_success / json_stream_fail belong in jsonrpc.c, and the
json_tok helpers for special types belong in json.x

json_add_object() isn't used, remove it rather than moving it.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
json_stream: disentangle JSON handling from command.
We promote 'struct json_stream' to contain the membuf; we only attach
the json_stream to the command when we actually call
json_stream_success / json_stream_fail.

This means we are closer to 'struct json_stream' being an independent
layer; the tests are already modified to use it directly to create
JSON.

This is also the first step toward re-enabling non-serial command
execution.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
jsonrpc: allow multiple commands at once.
We now keep multiple commands for a json_connection, and an array of
json_streams.

When a command wants to write something, we allocate a new json_stream
at the end of the array.

We always output from the first available json_stream; once that
command has finished, we free that and move to the next.  Once all are
done, we wake the reader.

This means we won't read a new command if output is still pending, but
as most commands don't start writing until they're ready to write
everything, we still get command parallelism.

In particular, you can now 'waitinvoice' and 'delinvoice' and it will
work even though the 'waitinvoice' blocks.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
jsonrpc: make struct json_connection definition private.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
jsonrpc: dev_slowcmd, a command which starts output then delays.
This lets us explicitly test that our JSON outputs don't intermingle.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
pytest: add test for multiplexed RPC output.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/json-multicmd branch from a5642e4 to 7b2727d Nov 20, 2018

@rustyrussell

This comment has been minimized.

Copy link
Contributor

rustyrussell commented Nov 20, 2018

OK, rebased on master, folded @cdecker fixes. New commit at the head (CHANGELOG tweak) and tail (more documentation).

rustyrussell added some commits Nov 20, 2018

pytest: test using malformed JSONRPC commands.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
jsonrpc: provide overview of how this all connects together.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/json-multicmd branch from 7b2727d to 93f989f Nov 20, 2018

@rustyrussell rustyrussell changed the title WIP: restored multiplexed JSONRPC command support Restore multiplexed JSONRPC command support Nov 20, 2018

@ZmnSCPxj

This comment has been minimized.

Copy link
Collaborator

ZmnSCPxj commented Nov 20, 2018

ACK 93f989f

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Nov 20, 2018

ACK 93f989f

@cdecker cdecker merged commit 8a246e2 into ElementsProject:master Nov 20, 2018

2 checks passed

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