pylightning: Asynchronous plugin rpc-methods and hooks #2326

cdecker · 2019-02-07T22:11:48Z

This PR allows RPC methods and hook-callbacks to defer returning a result by adding the sync=False argument to the @plugin.hook or @plugin.method annotations. If the annotation includes the sync=False argument it'll disregard the return value of the callback function. The function can then use the set_result or set_exception methods on the request object that they get as a parameter, when it suits them.

This allows passing a pending request off to another thread, stashing them in the local state, or any other operation, and still return a result at a later point in time. set_exception and set_result are threadsafe and can be called from anywhere.

I'm still a bit unsure if sync is the best name for the argument, I wanted to use async but the linter yells at me that async will become a keyword in python 3.8, so no luck there. Maybe @renepickhardt or our namer in chief @niftynei has a good idea? 😉

Fixes #2167

conscott · 2019-02-08T09:50:40Z

tests/plugins/asynctest.py

+from lightning import Plugin
+
+plugin = Plugin()
+requests = []


you can just dynamically attach this to object instance

plugin.requests = []

Rather that using globals below.

I suspect in the future people may also just subclass Plugin and add their local state there

Right, will fixup

Decided to initialize with a @plugin.init() decorator to make sure we have the new field whenever it is used.

conscott · 2019-02-08T10:05:44Z

contrib/pylightning/lightning/plugin.py

            monkey_patch(self, stdout=True, stderr=True)

-        self.add_method("getmanifest", self._getmanifest)
+        self.add_method("getmanifest", self._getmanifest, sync=True)


I don't have an whimsical ideas, but if async is taken, how about is_async ?

Here's a few variants that I thought of that might also work: direct=False, background=True, _async=True, or future=True. (All variants are shown with the async behavior as enabled here)

I tend to like background since it sort of describes that we want the thing to continue in the background.

+1 for background. i also like wait=False

niftynei

👍

rustyrussell · 2019-02-09T02:15:26Z

It seems weird to me to use a variant of the same annotation for a completely different beast.

After all, these functions will look quite different, won't they? Maybe @method_async ?

cdecker · 2019-02-19T19:07:10Z

I went with the background proposal, since that is the best description I could come up with. It's best not to have too many decorators since the interface gets too terse imho.

conscott · 2019-02-21T13:00:50Z

Concept ACK - tested this before, but it needs rebase

As for having this is a new decorator vs decorator arg, I don't have a super strong option, but I see rusty's case. A decorator should do one predictable thing and the background significantly changes the behavior, and expected way to handle the result.

You guys have better visibility on how many foreseeable decorators may be apart of the plugin lib, so I can't make a call on that part, which may be a valid concern.

Look there, I managed to agree with you both and say nothing :)

cdecker · 2019-02-21T13:24:32Z

As for having this is a new decorator vs decorator arg, I don't have a super strong option, but I see rusty's case. A decorator should do one predictable thing and the background significantly changes the behavior, and expected way to handle the result.

Ok, I consider myself overruled, and will create a separate decorator instead. tbh I didn't have a super strong reason to go for the single decorator, but you convinced me :-)

Sending around unnamed tuples is bound to cause some issues sooner or later, so we just create a quick class that holds all the information about a plugin method. Signed-off-by: Christian Decker <[email protected]>

We well need this in the next commit to be able to return from an asynchronous call. We also guard stdout access with a reentrant lock since we are no longer guaranteed that all communication happens on the same thread. Signed-off-by: Christian Decker <[email protected]>

This caused me to backtrack quite a bit, so this should help debugging in the future. Signed-off-by: Christian Decker <[email protected]>

These are a bit special and are handled separately. Signed-off-by: Christian Decker <[email protected]>

This isn't a problem for now since we don't support multithreading, and only allow synchronous calls, but eventually this'll become important. Signed-off-by: Christian Decker <[email protected]>

This indicates that the method or hook will accepts a request parameter, and will use that to return the result or raise an exception instead of returning the return value. This allows the hook or method to stash the incomplete request or pass it around, without blocking the JSON-RPC interface. Signed-off-by: Christian Decker <[email protected]>

Signed-off-by: Christian Decker <[email protected]>

Technically this is a node, not a direct peer, so this is correct. Signed-off-by: Christian Decker <[email protected]>

@rustyrussell

Suggested-by: Rusty Russell <@rustyrussell> Suggested-by: Conor Scott <@conscott> Signed-off-by: Christian Decker <[email protected]>

These weren't checked by CI yet, and they are really short so I just added them to the check-python target. Signed-off-by: Christian Decker <[email protected]>

rustyrussell

Ack 93be4d9

Great work, as always!

cdecker added python labels Feb 7, 2019

cdecker added this to the v0.6.4 milestone Feb 7, 2019

cdecker self-assigned this Feb 7, 2019

cdecker requested review from niftynei, renepickhardt and rustyrussell February 7, 2019 22:11

cdecker mentioned this pull request Feb 7, 2019

first version of lib-autopilot.py #1888

Closed

conscott reviewed Feb 8, 2019

View reviewed changes

cdecker force-pushed the async-plugin branch from dc94804 to 297a8dc Compare February 8, 2019 12:23

cdecker added the Work in Progress label Feb 8, 2019

cdecker force-pushed the async-plugin branch from 297a8dc to 48c6ea2 Compare February 8, 2019 15:17

niftynei reviewed Feb 9, 2019

View reviewed changes

cdecker modified the milestones: v0.6.4, v0.7 Feb 11, 2019

cdecker force-pushed the async-plugin branch from 48c6ea2 to 59e67c5 Compare February 19, 2019 19:06

cdecker removed the Work in Progress label Feb 19, 2019

cdecker added 8 commits February 21, 2019 18:56

pylightning: Wrap the plugin methods in a class

9a07d07

Sending around unnamed tuples is bound to cause some issues sooner or later, so we just create a quick class that holds all the information about a plugin method. Signed-off-by: Christian Decker <[email protected]>

pylightning: Exception if we have unfulfilled positional arguments

9f3502c

This caused me to backtrack quite a bit, so this should help debugging in the future. Signed-off-by: Christian Decker <[email protected]>

pylightning: Add support for *args and **kwargs in plugin dispatch

0d2431c

These are a bit special and are handled separately. Signed-off-by: Christian Decker <[email protected]>

pylightning: Don't always use request ID 0

661cf25

This isn't a problem for now since we don't support multithreading, and only allow synchronous calls, but eventually this'll become important. Signed-off-by: Christian Decker <[email protected]>

pylightning: Add a small test for async rpcmethods

73bad4c

Signed-off-by: Christian Decker <[email protected]>

pylightning: Rename peer_id to node_id in getroute

130afb0

Technically this is a node, not a direct peer, so this is correct. Signed-off-by: Christian Decker <[email protected]>

pylightning: Split @method and @async_method decorators

832ba0a

Suggested-by: Rusty Russell <@rustyrussell> Suggested-by: Conor Scott <@conscott> Signed-off-by: Christian Decker <[email protected]>

cdecker force-pushed the async-plugin branch from 59e67c5 to 0c0eca2 Compare February 21, 2019 18:00

pylightning: Add plugin dispatch tests to check-python and fix them

93be4d9

These weren't checked by CI yet, and they are really short so I just added them to the check-python target. Signed-off-by: Christian Decker <[email protected]>

cdecker force-pushed the async-plugin branch from 2d69242 to 93be4d9 Compare February 21, 2019 20:39

rustyrussell approved these changes Feb 22, 2019

View reviewed changes

rustyrussell merged commit f622ffb into ElementsProject:master Feb 22, 2019

darosior mentioned this pull request Nov 4, 2019

async keyword for functions in the Python API Client #1701

Closed

jb55 mentioned this pull request Mar 7, 2022

commando message hook blocking? lightningd/plugins#347

Closed

pylightning: Asynchronous plugin rpc-methods and hooks #2326

pylightning: Asynchronous plugin rpc-methods and hooks #2326

Uh oh!

Conversation

cdecker commented Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

conscott Feb 8, 2019

Choose a reason for hiding this comment

Uh oh!

cdecker Feb 8, 2019

Choose a reason for hiding this comment

Uh oh!

cdecker Feb 8, 2019

Choose a reason for hiding this comment

Uh oh!

conscott Feb 8, 2019

Choose a reason for hiding this comment

Uh oh!

cdecker Feb 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niftynei Feb 9, 2019

Choose a reason for hiding this comment

Uh oh!

niftynei left a comment

Choose a reason for hiding this comment

Uh oh!

rustyrussell commented Feb 9, 2019

Uh oh!

cdecker commented Feb 19, 2019

Uh oh!

conscott commented Feb 21, 2019

Uh oh!

cdecker commented Feb 21, 2019

Uh oh!

rustyrussell left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cdecker commented Feb 7, 2019 •

edited

Loading

cdecker Feb 8, 2019 •

edited

Loading