Move JSON-RPC server into an isolated plugin #1212

cburgdorf · 2018-08-23T12:48:06Z

What was wrong?

Currently the JSON-RPC server runs in the networking process together the syncing and other messaging. Now that isolated plugins are a thing, it's a good idea to move the JSON-RPC server because:

a separate process provides better isolation
- DDOS attacks
- crashes in the JSON-RPC can't bring the syncing down
better code organization

How was it fixed?

provide a LightPeerChainBridge plugin that brings an EventBusLightPeerChain to be consumed from other processes
enforce all RPC modules to be async
turn existing JSON-RPC server into an isolated plugin

In addition to the current functionality of the JSON-RPC server, the new plugin-based version can be disabled by passing --disable-rpc at startup

Cute Animal Picture

veox · 2018-08-27T16:12:59Z

trinity/extensibility/plugin.py

+    OWN = auto()
+
+
+class PluginContext:


I vaguely remember at least PluginContext(object) being recommended somewhere...

I believe that since python3 the two are equivalent.

pipermerriam

THis looks the same as something else I just reviewed....

cburgdorf · 2018-08-27T18:56:05Z

THis looks the same as something else I just reviewed....

Yep, Will probably have something more interesting here tomorrow ;)

cburgdorf · 2018-08-29T09:34:55Z

I moved this to the Ada Lovelace milestone because I think we shouldn't rush it. There's a lot of code to review (not only in this PR but also in the new lahja event bus library) and async problems can be very subtle.

cburgdorf · 2018-08-29T14:32:01Z

Ok, I think I need a break and I'm reaching out for help. I've been trying whole day to work out the remaining issues but there are two things that stand out that I think are related.

Trinity hangs on shutdown. This is also reproducible with a minimal test case that basically just runs event_bus.start(). This will prevent a graceful shutdown.
The tests fail pretty much for the same reason. The event bus is preventing the tests from closing, so while they succeed in principle, the test runner hangs and is shut down after 10 minutes of inactivity.

Both issues indicate that there's a problem with the event bus in general. Even the simple test in the lahja repo shows that behaviour. It goes green but never closes.

I guess it has to do with the Queue but I tried pretty much every thing I could think of (and what I found in the docs and StackOverflow). Really happy for any help on this.

UPDATE: I received an answer on StackOverflow that should help with this.

https://stackoverflow.com/questions/52081033/how-to-shutdown-process-with-event-loop-and-executor#comment91115577_52081665

cburgdorf · 2018-09-11T21:01:08Z

@pipermerriam @carver This is pretty much ready but tomorrow I'll write an alternative "less magical" version of the LightPeerChainBridgePlugin to have us look at the trade offs.

cburgdorf · 2018-09-12T08:20:23Z

Ok I think to make a fair alternative that evaluates @pipermerriam point of letting the type system help us more (in contrast to metaprogramming magic) we first need to fix the 3rd party library typing issue. Because the thing is, the revealed type of foobar will always be Any for the time being which prevents mypy from being an actual support here.

        foobar = await self.event_bus.request(event)
        reveal_type(foobar)

I will dig into that now.

cburgdorf · 2018-09-12T12:19:23Z

Alright, made the changes to Lahja to ensure the needed type safety guarantees. Will give it a shot now.

cburgdorf · 2018-09-13T13:46:14Z

@pipermerriam I think the main point left open here is the proper handling of cancellations (also, I moved it to a BaseService and use run_daemon_task already). I feel that can be addressed as part of #1284 that I would soon start looking into.

How do you feel about taking it as is?

cburgdorf · 2018-09-13T21:29:10Z

@pipermerriam this looks weird. I just rebased this and now the integration test caught this:

"p2p.exceptions.DecryptionError: Invalid frame mac: expected b'\\xde\\xcag\\x0e\\x94\\xdbkC\\xa8\\xc4\\xb8\\x10\\xdb@\\xf8o', got b'\\xdd\\x16\\xcea]B\\xbd1\\x8a\\x0f\\xa7\\xe8B\\x87\\xf9\\xce'\n"

Looks totally random to me. https://circleci.com/gh/ethereum/py-evm/77685?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

I'm out of town tomorrow until evening. Guess I'll investigate that when I come home.

cburgdorf · 2018-09-13T21:53:02Z

Ok, turns out that DecryptionError was indeed random. After all, the integration test boots the node in an environment with internet access which means these things can happen. Forgot that for a moment.

pipermerriam · 2018-09-13T23:32:35Z

two things, I never finished reviewing this (will try to do it tonight so you're clear to move forward tomorrow)

second, make an issue for that decryption error as thoseshouldn't escape the trinity process.

pipermerriam · 2018-09-13T18:51:13Z

p2p/peer.py

@@ -794,6 +804,16 @@ def __init__(self,
        self.max_peers = max_peers
        self.connected_nodes: Dict[Node, BasePeer] = {}
        self._subscribers: List[PeerSubscriber] = []
+        self.event_bus = event_bus
+        self.run_task(self.handle_peer_count_requests())


I think this should be using self.run_daemon_task

pipermerriam · 2018-09-13T18:56:00Z

trinity/chains/light.py

        header = self._headerdb.get_block_header_by_hash(block_hash)
-        return self.get_block_by_header(header)
+        return await self.get_block_by_header(header)


What am missing? The very next function definition is for get_block_by_header and it is both not a coroutine and raises a NotImplementedError.

yep, that was meant to go to coro_get_block_by_header

pipermerriam · 2018-09-14T02:27:58Z

trinity/rpc/format.py

+            return async_formatted_func
+        else:
+            @functools.wraps(func)
+            def formatted_func(self: Any, *args: Any) -> Callable[..., Any]:


Is this still necessary, quick glance suggests that all of the wrapped methods are now coroutines.

yep, will change that.

cburgdorf force-pushed the christoph/feat/plugins-own-process branch 3 times, most recently from 7adbad7 to cc59287 Compare August 27, 2018 12:04

veox reviewed Aug 27, 2018

View reviewed changes

pipermerriam reviewed Aug 27, 2018

View reviewed changes

cburgdorf force-pushed the christoph/feat/plugins-own-process branch 2 times, most recently from 2840d43 to c5ea88d Compare August 28, 2018 13:38

cburgdorf changed the title ~~Allow plugins to run in a separate process~~ Move JSON-RPC server into an isolated plugin Aug 28, 2018

cburgdorf force-pushed the christoph/feat/plugins-own-process branch 2 times, most recently from 3d598fe to baf1e40 Compare August 28, 2018 14:07

cburgdorf mentioned this pull request Aug 28, 2018

PoC: Communication between PeerPool and other process via event bus #1211

Closed

cburgdorf force-pushed the christoph/feat/plugins-own-process branch 2 times, most recently from 76551d4 to a7cbf0f Compare August 28, 2018 21:16

cburgdorf modified the milestones: Ada Lovelace (Trinity Release), Dorothy Vaughan (Trinity Release) Aug 29, 2018

cburgdorf force-pushed the christoph/feat/plugins-own-process branch 3 times, most recently from 714080a to 272e12e Compare August 29, 2018 13:03

cburgdorf closed this Aug 29, 2018

cburgdorf deleted the christoph/feat/plugins-own-process branch August 29, 2018 13:07

cburgdorf reopened this Aug 29, 2018

cburgdorf force-pushed the christoph/feat/plugins-own-process branch 2 times, most recently from 714080a to b86f3e3 Compare August 29, 2018 13:39

veox mentioned this pull request Aug 29, 2018

Test never exits cburgdorf/lahja#1

Closed

cburgdorf mentioned this pull request Sep 12, 2018

Implement auto-discovery of plugins #1230

Closed

cburgdorf mentioned this pull request Sep 12, 2018

Christoph/feat/plugins own process alternative #1280

Closed

cburgdorf force-pushed the christoph/feat/plugins-own-process branch from bbc1ed6 to 5924042 Compare September 13, 2018 13:23

cburgdorf requested a review from pipermerriam September 13, 2018 13:46

cburgdorf mentioned this pull request Sep 13, 2018

RFC: Run the Node service in trinity's main process #1292

Closed

cburgdorf force-pushed the christoph/feat/plugins-own-process branch 4 times, most recently from 33cc77e to 5c5dcc4 Compare September 13, 2018 21:19

cburgdorf force-pushed the christoph/feat/plugins-own-process branch from 5c5dcc4 to 00c0465 Compare September 13, 2018 21:40

pipermerriam approved these changes Sep 14, 2018

View reviewed changes

cburgdorf added 4 commits September 15, 2018 21:51

Ensure plugins don't hang the process on exit

aea9977

Enforce RPC methods to be async

10aa729

Add LightPeerChainBridgePlugin

04c7196

Reduce risk of CI failures

005752f

cburgdorf force-pushed the christoph/feat/plugins-own-process branch 3 times, most recently from df83846 to 6ee8b1d Compare September 15, 2018 20:32

Make JSON-RPC Server an isolated plugin

59dc7c1

cburgdorf force-pushed the christoph/feat/plugins-own-process branch from 6ee8b1d to 59dc7c1 Compare September 15, 2018 20:47

cburgdorf merged commit 1006923 into ethereum:master Sep 15, 2018

cburgdorf mentioned this pull request Sep 17, 2018

Move json-rpc server to its own process #1062

Closed

carver mentioned this pull request May 16, 2019

tests: update to match py-evm: don't skip Constantinople, add Petersburg, use --fork; break out long-running categories ethereum/trinity#608

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move JSON-RPC server into an isolated plugin #1212

Move JSON-RPC server into an isolated plugin #1212

cburgdorf commented Aug 23, 2018 •

edited

Loading

veox Aug 27, 2018

pipermerriam Aug 27, 2018

pipermerriam left a comment

cburgdorf commented Aug 27, 2018

cburgdorf commented Aug 29, 2018 •

edited

Loading

cburgdorf commented Aug 29, 2018 •

edited

Loading

cburgdorf commented Sep 11, 2018

cburgdorf commented Sep 12, 2018

cburgdorf commented Sep 12, 2018

cburgdorf commented Sep 13, 2018

cburgdorf commented Sep 13, 2018

cburgdorf commented Sep 13, 2018

pipermerriam commented Sep 13, 2018

pipermerriam Sep 13, 2018

pipermerriam Sep 13, 2018

cburgdorf Sep 15, 2018

pipermerriam Sep 14, 2018

cburgdorf Sep 15, 2018

Move JSON-RPC server into an isolated plugin #1212

Move JSON-RPC server into an isolated plugin #1212

Conversation

cburgdorf commented Aug 23, 2018 • edited Loading

What was wrong?

How was it fixed?

Cute Animal Picture

veox Aug 27, 2018

Choose a reason for hiding this comment

pipermerriam Aug 27, 2018

Choose a reason for hiding this comment

pipermerriam left a comment

Choose a reason for hiding this comment

cburgdorf commented Aug 27, 2018

cburgdorf commented Aug 29, 2018 • edited Loading

cburgdorf commented Aug 29, 2018 • edited Loading

cburgdorf commented Sep 11, 2018

cburgdorf commented Sep 12, 2018

cburgdorf commented Sep 12, 2018

cburgdorf commented Sep 13, 2018

cburgdorf commented Sep 13, 2018

cburgdorf commented Sep 13, 2018

pipermerriam commented Sep 13, 2018

pipermerriam Sep 13, 2018

Choose a reason for hiding this comment

pipermerriam Sep 13, 2018

Choose a reason for hiding this comment

cburgdorf Sep 15, 2018

Choose a reason for hiding this comment

pipermerriam Sep 14, 2018

Choose a reason for hiding this comment

cburgdorf Sep 15, 2018

Choose a reason for hiding this comment

cburgdorf commented Aug 23, 2018 •

edited

Loading

cburgdorf commented Aug 29, 2018 •

edited

Loading

cburgdorf commented Aug 29, 2018 •

edited

Loading