openwallet-foundation / acapy

ACA-Py is a foundation for building decentralized identity applications and services running in non-mobile environments.
https://aca-py.org
Apache License 2.0
421 stars 515 forks source link

Plugin Loading and Injection #2172

Open usingtechnology opened 1 year ago

usingtechnology commented 1 year ago

Version: aries-cloudagent-acapy-1.0.0-rc1

Not sure if there is anything actionable here, mostly for reference purposes now... may be useful if anyone else runs into a similar issue.

Loading a series of plugins, some with injection registration in on_startup handlers (I need the root profile). I noticed that if they are loaded in a certain order that the registration gets "lost".

There are 3 plugins that need the root profile and each plugin registers an on_startup handler, inside of which it binds an instance into the injector.

Multitenant Manager Provider Plugin

async def setup(context: InjectionContext):
    bus = context.inject(EventBus)
    bus.subscribe(STARTUP_EVENT_PATTERN, on_startup)

async def on_startup(profile: Profile, event: Event):
    _config = get_config(profile.settings)
    profile.context.injector.bind_instance(MultitenantProviderConfig, _config)
    profile.context.injector.bind_provider(
        BaseMultitenantManager, CustomMultitenantManagerProvider(profile)
    )
    # the AdminServer was created with the old one injected
    # replace it...
    srv = profile.context.inject(BaseAdminServer)
    srv.multitenant_manager = profile.context.inject(BaseMultitenantManager)

Innkeeper Plugin

async def setup(context: InjectionContext):
    bus = context.inject(EventBus)
    bus.subscribe(STARTUP_EVENT_PATTERN, on_startup)

async def on_startup(profile: Profile, event: Event):
    _config = get_config(profile.settings)
    mgr = TenantManager(profile, _config)
    profile.context.injector.bind_instance(TenantManager, mgr)
    await mgr.create_innkeeper()

OCA Plugin

async def setup(context: InjectionContext):
    bus = context.inject(EventBus)
    bus.subscribe(STARTUP_EVENT_PATTERN, on_startup)

async def on_startup(profile: Profile, event: Event):
    svc = OcaService(profile)
    profile.context.injector.bind_instance(OcaService, svc)

IMPORTANT the multitenant manger provider plugin is loaded explicitly in the startup (--plugin) while innkeeper and oca are loaded in series through it's parent plugin. So multitenant manger provider and the parent are loaded however acapy does it (parallel? series?) but the innkeeper and oca are 100% done in series.

Parent Plugin

async def setup(context: InjectionContext):
     ...
        for mod in MODULES:
            # call the setup explicitly...
            await mod.setup(context)
    ...

The multitenant manager provider plugin is somewhat unique (and gnarly) in that it binds an instance and a provider and then replaces a previously injected object.

The innkeeper plugin not only binds an instance, it makes use of that instance in the startup.

If I load innkeeper -> oca => multitenant manager provider, everything works as expected. The instances and providers can be used elsewhere (routes etc) and returns the instances required.

If I load multitenant -> oca -> innkeeper, it also works.

If I load multitenant -> innkeeper -> oca, it does not work. The OcaService that is bound is "lost", so error is thrown when getting the instance. aries_cloudagent.config.base.InjectionError: No instance provided for class: OcaService

I did logging of the profile.context.injector._providers and OcaService is definitely in the list after the instance is bound in it's on_startup. However, when I go to retrieve the instance from the injector (and log the _providers) it is NOT in the list... (TenantManager, MultitenantProviderConfig and BaseMultitenantManager are in the list).

    context: AdminRequestContext = request["context"]
    mgr = context.inject(TenantManager)
    service = context.inject(OcaService)

I logged the injection_context and injector classes to identify when the OcaService is dropped, and it appears that because the innkeeper makes use of it's TenantManager within it's on_startup, that "seeds" the endless copying of injection_context and injector... when oca plugin binds its instance, it is registered, but all other injection sessions are being copied from the one that the innkeeper started (which doesn't contain OcaService). What is really weird is that the innkeeper/oca order does not matter if the multitenant manager plugin is called last.

Anyway, I can obviously control my ordering, so no fix needed for me, but if plugins are encouraged going forward, this type of thing will be an issue.

Also, the way the injection_context and injector works seems to be super heavy? it/they call copy thousands of times (i guess for every call to context.inject(Class)?) My log file was mostly just start_scope and copy.

swcurran commented 1 year ago

@ianco — fodder for your plugins design?

ianco commented 1 year ago

@ianco — fodder for your plugins design?

Yes all very good info

ianco commented 1 year ago

@usingtechnology it may be a nit, but you could try moving your use of your TenantManager outside of your on_startup() method. I would presume that the on_startup() was for configuration only, and any side-effects should happen outside of the on_startup()

This would also loosely couple the innkeeper creation and the startup code (e.g. maybe in some implementation you don't want to create the innkeeper on startup but want to explicitly do it later. Also since it's a one-time operation, you don't necessarily want to do it on every aca-py startup.

One option is to move the code into your routes module, and setup an event listener. Revocation does this for example: https://github.com/hyperledger/aries-cloudagent-python/blob/main/aries_cloudagent/revocation/routes.py#L1283

Then in your startup code you could do:

profile.notify(...)

... and then your listener code could do the actual innkeeper creation.

Not sure this solves the problem or not, but I think we need to ensure that all the configuration takes place before aca-py starts actually "doing stuff"

usingtechnology commented 1 year ago

We could move the use of Tenant Manager outside of the initial on_setup...I'll add a ticket and refactor that code to be outside of the plugin startup.

ianco commented 1 year ago

My other thought is that we split the ON_STARTUP into:

ON_STARTUP_CONFIG

... and then ...

ON_STARTUP_INITIALIZE

... or something like that. We would call the *_CONFIG for all plugins followed by the *_ON_INIT, which makes the division of functionality a bit more explicit.

usingtechnology commented 1 year ago

yes, maybe too much to consider, but it'd be nice to have a weight or explicit init order, so once all the plugins are configured then you can set the order in which they are initialized...

but yes, i like that concept of separating duties and purpose.

ianco commented 1 year ago

I like the idea of having an explicit load order too ...

WadeBarnes commented 1 year ago

ON_CONFIG, ON_INITIALIZE, and ON_STARTUP would be good for the names of the ordered lifecycle events.

For the plugin ordering it could be as simple as an index defined by each plugin that could be used to sort the plugins to weight their execution order. We use that approach in indy-node-monitor to control the execution order of the plugins, here. Though such an approach would likely be too simple in this case and not scale well with an arbitrary number of plugins. A way to define dependencies would likely be better so each plugin could define which plugins need to be loaded before it and the order in which they need to be loaded. It would be the most flexible approach but could be complicated.