Libplugin refactoring #3443
Merged
Libplugin refactoring #3443
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cdecker
requested review from
rustyrussell and
cdecker
and removed request for
rustyrussell
darosior
force-pushed
the
libplugin_io_plan
branch
4 times, most recently
from
7c475b4
to
d56c7f8
Compare
darosior
force-pushed
the
libplugin_io_plan
branch
from
d56c7f8
to
cf6a1de
Compare
|
Rebased (and added a1857df for errcodes). |
darosior
force-pushed
the
libplugin_io_plan
branch
from
cf6a1de
to
aa0ff6b
Compare
|
Re-rebased |
rustyrussell
approved these changes
Feb 3, 2020
| static struct timers timers; | ||
| static size_t in_timer; | ||
|
|
||
| bool deprecated_apis; |
There was a problem hiding this comment.
deprecated_apis is kinda special, I'd be tempted to leave it as a global...
There was a problem hiding this comment.
But the plugin is accessible everywhere ?
There was a problem hiding this comment.
- The core code also uses a global, so codesharing is easier this way.
- Without this, the definition of struct plugin doesn't need to be exposed.
There was a problem hiding this comment.
Ok, will do tomorrow
|
I did the change about |
darosior
force-pushed
the
libplugin_io_plan
branch
from
aa0ff6b
to
1a7d3c7
Compare
It's not lightningd-specific and we are going to need it for libplugin. The only drawback is the log_io removal in json_stream_output_write()..
Now that we have json_stream in common/, we can move all the related helpers from lightningd/json to common/json. This way everyone can benefit of them (including libplugin, the plugins themselves, potentially lightning-cli), not lightningd alone! Note that the Makefile of the common/test/ had to be modified, because the new helpers make use of common/wireaddr... Which turns out to \#include <lightingd/lightningd.h> ! So we couldnt just include the .c and add mocks if we redefined some structs (hello run-param).
Having a global object is very handy to pass to `io_plan`s, which would have otherwise required a whole bunch of new globals.
Now we have streams and a global object, we can use them for io_plans. This makes the logic closer from lightningd/plugin (it has been mostly stolen from here), and simplifies the main. This also allows plugins to use io_plans (thanks to the io_loop) for asynchronous connections. This commit only handle incoming commands.
This pass to json_stream helpers for commands outputs, but keeps compatibility with existing plugins which use jout as of now, by not starting/closing the "result"/"error" objects.
And rename the struct name, as it's now only used for RPC.
But not the outreqs helpers, which will be moved when passing send_outreq_ to using ccan/io.
autoclean needs to send outreqs from a timer cb, hence with cmd == NULL.
|
Trivial rebase on master for test autogen conflicts... |
rustyrussell
force-pushed
the
libplugin_io_plan
branch
from
1a7d3c7
to
4a296b3
Compare
|
Thanks ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In order to correctly (i.e. not loose asynchronicity) implement #3354 I needed libplugin to use
ccan/io. This escalated quickly to a complete refactoring oflibplugin.This PR:
json_stream.hand its corresponding helpers tocommon/, which was required to properly implementio_plans, and which seems generally good as it now allows plugins to use higher-level JSON helpers as inlightningd/*.stdin/stdoutplugin connections use exclusivelyio_plans which simplifies theplugin_mainand keeps the same logic as the other side:lightningd/plugin.send_outreq) useio_plans, which I believe could help with Concurrency in libplugin #3401 though I haven't tested it yet. However I'm happy to revert this patch if that clashes with @ZmnSCPxj work with sparks.plugin.Left for another PR:
Move existing plugins to useDone in libplugin: use json stream helpers and add sanity tests #3480json_streamhelpers instead of "raw"joutsrpc_delveand co.) ?Changelog-None