Skip to content
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

Convert plugin API to be purely event based #1090

Closed
pipermerriam opened this issue Jul 25, 2018 · 7 comments
Closed

Convert plugin API to be purely event based #1090

pipermerriam opened this issue Jul 25, 2018 · 7 comments

Comments

@pipermerriam
Copy link
Member

pipermerriam commented Jul 25, 2018

What things currently look like.

The current plugin API looks like this.

  • configure_parser: Hook that can be implemented by plugin to modify CLI parser configuration
  • should_start: Called by PluginManager to check whether a plugin should be started based on an event
  • start: Called by the PluginManager if should_start returns True and the plugin has not been started yet.
  • stop: Called by the PluginManager to stop plugins during shutdown.

The Event API works through the PluginManager and all events are fired by the PluginManager. The following events are currenty available.

  • TrinityStartupEvent - broadcast when the NodeClass is launched/started
  • PluginStartedEvent - broadcast when a specific plugin is started
  • ResourceAvaiableEvent - broadcast when a resource becomes available

This API has been a good start, but it has been built in direct response to the needs presented by the transaction pool and the other initial plugin experiments. Now that we are starting to see what a plugin might look like, I propose the following changes to this API.

What I think things should look like

First, I want to better define the initialization process and plugin lifecycle.

  • Plugin.bootstrap(arg_parser, sub_parser):
    • The very thing that is called on a plugin. Lets experiment with making this a classmethod since this will occur early enough in the process that I don't think we can realistically initialize plugins at this stage.
    • This is where plugins get the chance to modify the CLI
  • Plugin.__init__(args, token):
    • Plugin classes get initialized.
    • args is the CLI Argparser result.
    • token is a CancelToken that the plugin should use to gracefully handle shutdowns (not fully fleshed out).
  • Plugin.ready(chain_config, broadcast_queue, subscription_queue)
    • Concept lifted from django AppConfig.ready
    • Name subject to change
    • Concept is that this is the last point prior to things starting
    • broadcast_queue is an asyncio.Queue that the plugin can use to broadcast an event (more below)
    • subscription_queue is an asyncio.Queue that the plugin can consume events that have been broadcast by other plugins. (more below)
  • Plugin.run()
    • Starts the plugin
  • Plugin.cancel()
    • Called when the app is exiting. Last chance for a plugin to gracefully shutdown.

After ready and before shutdown is when the app is running. Within these boundaries, the Trinity application is running and plugins are active. During this period, plugins interact via events.

Here is the best description of how I think events should work.

  1. Events should probably be registered sometime during initialization. This could happen automatically by the PluginManager.
  • Plugin.get_broadcast_events(): Returns all of the events that this plugin can broadcast.
  • Plugin.get_subscription_events(registry): Returns all of the events that this plugin wants to subscribe to. The registry would be a data structure containing all of the registered events (and probably the plugin which registered them).
  1. Plugins may broadcast any event that it registered from get_broadcast_events using the broadcast_queue
  2. Plugins read events from the subscription_queue and respond.

We probably have a few core events that are provided by the plugin manager or some other built-in plugin.

  • DiscoveryStarted to give plugins access to the DiscoveryService
  • PeerPoolStarted to give plugins access to the PeerPool
@cburgdorf
Copy link
Contributor

Lets experiment with making this a classmethod since this will occur early enough in the process that I don't think we can realistically initialize plugins at this stage

I think I've found a potential problem with this. If the bootstrap method wants to take over the whole process (like the attach plugin does), it needs to assign a method to take over the process and if that method is a classmethod, it won't have access to the logger which I guess should remain instance based.

E.g. the attach plugin currently uses the logger in that method which would have to become a class method under this design.

self.logger.error(str(err))

So, if we consider that this method not only configures the parser but potentially defines a method to overtake the whole process, maybe it shouldn't become a classmethod? But that also changes the rest of the design. Need to give this more thought.

@pipermerriam
Copy link
Member Author

If the only problem is the logger then we can address that via some python wizardry using a very simple metaclass or descriptor.

@pipermerriam
Copy link
Member Author

class AutoLogger:
    def __get__(self, obj, cls):
        if self._logger is None:
            self._logger = logging.getLogger(cls.__module__ + '.' + cls.__name__)
        return self._logger

class HasLogger:
    logger = AutoLogger()

@cburgdorf
Copy link
Contributor

I'm not sure if I understand what it does since I haven't looked into metaclasses yet and I'm also not sure if HasLogger is supposed to stand for the plugin. In that case, I'd be left wondering if that wouldn't create a logger on the class level 🤔

However, on a more general note: Basically, it means that plugins that want to overtake the whole process are always restricted to assign a classmethod and don't have access to anything that is related to the instance of the plugin.

@pipermerriam
Copy link
Member Author

I'm gonna step back from this and let you figure it out. You're closer to the code.

@cburgdorf
Copy link
Contributor

I think with the event bus proving more and more as a viable solution, and if we don't hit a big performance problem, then, I think we should retire the plugin event mechanism entirely and simply let all event based communication go through the event bus.

@cburgdorf
Copy link
Contributor

We achieved this by basing our plugin architecture on the event bus.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants