Skip to content

pylightning: Add a plugin framework #2161

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

Merged
merged 6 commits into from
Dec 15, 2018
Merged

pylightning: Add a plugin framework #2161

merged 6 commits into from
Dec 15, 2018

Conversation

cdecker
Copy link
Member

@cdecker cdecker commented Dec 10, 2018

This framework is very much inspired by flask, in that I wanted to make it as trivial as possible to get started. I had to jump through some hoops but I'm rather happy with the end result. A plugin with an option and a RPC passthrough is as simple as this:

from lightning import Plugin, monkey_patch
plugin = Plugin(autopatch=True)

@plugin.method("hello")
def hello(name, plugin):
    """Say hello"""
    greeting = plugin.get_option('greeting')
    plugin.log('{} {}'.format(greeting, name))
    return s

plugin.add_option('greeting', 'Hello', 'The greeting I should use.')
plugin.run()

It has support for monkey-patching stdout and stderr by default so that if run by lightningd it'll automatically wrap anything written to those streams into log notifications that get displayed nicely.

Depends-On: #2154

try:
result = {
"jsonrpc": "2.0",
"result": self._dispatch(request),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks to me that this will result in "linear" dispatching, i.e. commands are handled one-at-a-time.

Would it be possible to have this handle dispatching in parallel, i.e. if a command stalls processing, another command can be processed?

Or is some Python limitation exists to prevent this, or it becomes complicated?

(maybe there can be a separate decoration for methods that might block)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, hadn't thought about it yet. I wanted to keep away from multi-threading as much as possible, but I'll try to see what I can come up with.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened issue #2167 to track progress on this. Would love to hear feedback for the initial idea, which is future-inspired, but since the python futures are tightly bound to multi-threading I'm thinking of implementing my own AsyncResult instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems sensible.

@@ -895,6 +895,7 @@ void plugins_init(struct plugins *plugins, const char *dev_plugin_debug)
plugins->pending_manifests = 0;
uintmap_init(&plugins->pending_requests);

setenv("LIGHTNINGD_PLUGIN", "1", 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Convenient! We could have also added a commandline option, but this is easier for plugins to grab in most languages.

and stdin filedescriptor, so if we use them in some other way
(printing, logging, ...) we're breaking our communication
channel. This function
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did the rest of this comment get redirected to JSON? :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a common problem that occurs when monkey patching system internals. The comment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xD

@@ -17,6 +17,7 @@ class Plugin(object):
def __init__(self, stdout=None, stdin=None, autopatch=True):
self.methods = {}
self.options = {}
self.option_values = {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the distinction between options and option_values?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

options are the names of options that we registered with all their metadata, whereas option_values are the ones that were set by the init call from the plugin. We can merge them if desired of course.

@cdecker
Copy link
Member Author

cdecker commented Dec 14, 2018

Merged options and option_values as suggested by @rustyrussell and fixed the comm

It might be useful to take special precautions inside a plugin when
being run as a plugin (and not as a standalone executable). This env
var is just set so plugins can differentiate correctly. I don't unset
the variable since it shouldn't have any effect on `lightningd`
itself.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
It's flask inspired with the Plugin instance and decorators to add
methods to the plugin description.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Should be a lot easier to see what happens :-)

Signed-off-by: Christian Decker <decker.christian@gmail.com>
This is just cosmetic, and makes things like tracebacks much easier to
read.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
Suggested-by: Rusty Russell <@rustyrussell>
Signed-off-by: Christian Decker <decker.christian@gmail.com>
@cdecker
Copy link
Member Author

cdecker commented Dec 15, 2018

Interpreting the accept-review as an ACK, rebased and squashed.

ACK 5dbc836

@cdecker cdecker merged commit 42d8e07 into master Dec 15, 2018
@rustyrussell rustyrussell deleted the plugin-6 branch August 15, 2022 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants