Refactor plugin support to exclusively use EventBus events #1331

cburgdorf · 2018-09-27T17:52:15Z

What was wrong?

Currently plugins have two different ways of consuming events:

Comsuming lahja events through the EventBus
Consuming plugin events through handle_event

With the upcoming lahja release, this can be unified.

How was it fixed?

Plugins consume EventBus events only
should_start is gone. The model has turned. While previously the PluginManager called _start() on the plugins, plugins do now call handle_start() themselves.
Because of the former, it is now the responsible of the plugin base classes to flip a running property on which the PluginManager figures out which plugins need to be stopped
The Node does not depend on the PluginManager any more

Known issues:

I think the start and handle_start methods could use better names. The handle_start method is what plugins need to call to start the plugin whereas the start method is what they need to implement.

Cute Animal Picture

cburgdorf · 2018-10-10T11:38:29Z

@pipermerriam this is now based on the freshly released lahja 0.9.0 release and can be given a full review. The thing that I personal dislike the most about it is the naming of start and handle_start where start is a method to be implemented by plugins to wrap the boot process of the plugin and handle_start is what plugins need to call to actually start itself when they want to.

Maybe we should rename handle_start to run and start to bootstrap?

pipermerriam · 2018-10-10T15:13:17Z

trinity/extensibility/plugin.py

        """
-        Called at startup, giving the plugin a chance to amend the Trinity CLI argument parser
+        Called after the plugin received its context and is ready to bootstrap itself.


It'd be cool to have a large docstring for this class which diagram'd the lifecycle of a plugin (and probably a page in the docs once the API stablizes.)

Yes, I plan to improve docs for plugins soon! (#1103)

pipermerriam · 2018-10-10T15:14:26Z

trinity/extensibility/plugin.py

        pass

-    def should_start(self) -> bool:
+    def handle_start(self) -> None:


I like bootstrap or boot.

pipermerriam · 2018-10-10T15:14:50Z

trinity/extensibility/plugin.py

+        self.event_bus.broadcast(
+            PluginStartedEvent(type(self))
+        )
+        self.logger.info("Plugin started: %s", self.name)

    def start(self) -> None:


I'm ok with run but I think I like start more since stop is a natural opposite.

Right, then let's leave this be called start and rename handle_start to boot

pipermerriam · 2018-10-10T15:16:29Z

trinity/extensibility/plugin_manager.py

                continue

            try:
                self._logger.info("Stopping plugin: %s", plugin.name)
                plugin.stop()
+                plugin.running = False


This seems wrong for something external to the plugin to manage its state. Shouldn't this be handled by the plugin.stop method?

Mmh...well, since stop is implemented by each individual plugin that would mean that plugin authors either need to remember to call super().stop() (bad because if they forget it, they are basically leaving the PluginManager thinking that the plugin would still be running when in reality it isn't.

Alternatively we do the same as we do with the start / boot pair which means there will be another method (shutdown ?) which sets running=False and then delegates to stop(). At least, that would be symmetric to the start / boot pair.

However setting plugin.running from the manager lets us avoid this extra indirection and I'm personally ok with having the PluginManager make the flip here. Your call.

I'm generally a fan of the model you proposed with stop/shutdown. One is internal, only accessed by the manager and handles the orchestration like setting flags, triggering asyncio.Events, the other is the hook for the user to implement the logic. We've been using this with BaseService for a while and I think it's working out well.

Alright, will refactor that prior to merging this.

Ok, I just looked at the BaseService again and now I think the naming should be:

start (public, the API to call to start the plugin)
_start (private, only to be implemented by the plugin to encapsulate the starting logic)

stop (public, the API called by the PluginManager to stop the plugin (will flip ready internally)
_stop (private, only to be implemented by the plugin to encapsulate the starting logic)

That is inline with the BaseService that uses run + _run intended to be overwritten and cleanup + _cleanup intended to be overwritten.

Do you agree with that?

@pipermerriam this 5ee9b53

cburgdorf force-pushed the christoph/feat/plugins-love-lahja branch from 1a704b0 to 712285c Compare September 28, 2018 19:35

cburgdorf force-pushed the christoph/feat/plugins-love-lahja branch from 712285c to 407e9be Compare October 8, 2018 09:44

cburgdorf changed the title ~~Refactor plugin support to rely exclusively on EventBus events~~ Refactor plugin support to exclusively on EventBus events Oct 8, 2018

cburgdorf force-pushed the christoph/feat/plugins-love-lahja branch 6 times, most recently from a30b56b to f3f8b29 Compare October 8, 2018 13:24

cburgdorf changed the title ~~Refactor plugin support to exclusively on EventBus events~~ Refactor plugin support to exclusively use EventBus events Oct 8, 2018

cburgdorf requested a review from pipermerriam October 8, 2018 20:12

cburgdorf force-pushed the christoph/feat/plugins-love-lahja branch from f3f8b29 to e879f92 Compare October 9, 2018 10:24

cburgdorf mentioned this pull request Oct 9, 2018

Ensure internal events work with non-pickable events ethereum/lahja#18

Merged

cburgdorf added this to the Annie Easley (Trinity release) milestone Oct 10, 2018

Get rid of TrinityStartupEvent

6f239c5

cburgdorf force-pushed the christoph/feat/plugins-love-lahja branch 3 times, most recently from 28dfc4d to 4d268a7 Compare October 10, 2018 11:33

cburgdorf mentioned this pull request Oct 10, 2018

Remove obsolete hack #1382

Merged

pipermerriam approved these changes Oct 10, 2018

View reviewed changes

cburgdorf added 2 commits October 10, 2018 20:12

Refactor plugin architecture

7766403

Better encapsulation + API name changes

5ee9b53

cburgdorf force-pushed the christoph/feat/plugins-love-lahja branch from 4d268a7 to 5ee9b53 Compare October 10, 2018 18:52

cburgdorf merged commit 9eabc8d into ethereum:master Oct 10, 2018

cburgdorf mentioned this pull request Oct 26, 2018

Create guide on how to create plugins #1429

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor plugin support to exclusively use EventBus events #1331

Refactor plugin support to exclusively use EventBus events #1331

cburgdorf commented Sep 27, 2018 •

edited

Loading

cburgdorf commented Oct 10, 2018

pipermerriam Oct 10, 2018

cburgdorf Oct 10, 2018

pipermerriam Oct 10, 2018

pipermerriam Oct 10, 2018

cburgdorf Oct 10, 2018

pipermerriam Oct 10, 2018

cburgdorf Oct 10, 2018

pipermerriam Oct 10, 2018

cburgdorf Oct 10, 2018 •

edited

Loading

cburgdorf Oct 10, 2018

cburgdorf Oct 10, 2018

Refactor plugin support to exclusively use EventBus events #1331

Refactor plugin support to exclusively use EventBus events #1331

Conversation

cburgdorf commented Sep 27, 2018 • edited Loading

What was wrong?

How was it fixed?

Known issues:

Cute Animal Picture

cburgdorf commented Oct 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cburgdorf Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cburgdorf commented Sep 27, 2018 •

edited

Loading

cburgdorf Oct 10, 2018 •

edited

Loading