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

[Discussion] Plugin Remaster #4

Open
Foxtrek64 opened this issue Aug 8, 2022 · 10 comments
Open

[Discussion] Plugin Remaster #4

Foxtrek64 opened this issue Aug 8, 2022 · 10 comments

Comments

@Foxtrek64
Copy link
Contributor

Foxtrek64 commented Aug 8, 2022

After some discussions, we have outlined the following priorities for a plugins rewrite:

  • Plugin Constructed using DI.
  • IPluginDescriptor will provide Start() and Stop() async methods, which accept a cancellation token.
    • Start() will return a Task<Result>. A successful result indicates that tree initialization can continue. A failed result will abort initializing any child plugins which have a Required relationship.
    • Stop() will return void. A plugin must always be prepared to stop at any time.
  • Each plugin will get its own ScopedServiceProvider.
  • Plugins should be able to be reloaded (which stops and disposes all plugins and plugin scopes, then reloads all plugins).
  • Plugins should be able to be registered at runtime though a reload of the full plugin tree.
public interface IPluginDescriptor
{
    /// <summary>
    /// Gets the name of the plugin. This name should be unique.
    /// </summary>
    string Name { get; }

    /// <summary>
    /// Gets the description of the plugin.
    /// </summary>
    string Description { get; }

    /// <summary>
    /// Gets the version of the plugin.
    /// </summary>
    Version Version { get; }

    /// <summary>
    /// Called when the application host is ready to start the plugin.
    /// </summary>
    Task<Result> StartAsync(CancellationToken ct = default);

    /// <summary>
    /// Called when the application host is shutting down the plugin.
    /// </summary>
    /// <remarks>
    /// A plugin must be prepared to stop at any time.
    /// </remarks>
    Task StopAsync();

A plugin should also implement IDisposable to help clean up dependencies. Plugin developers may also implement IAsyncDisposable if applicable to them.

Plugin lifetime:

  • Load plugins (handles referenced assemblies being missing)
  • Service registrations - No ordering required
  • StartAsync() - Might require some ordering
  • Runtime (handling notifications/requests/direct service calls). Main application's runtime takes over.
  • StopAsync()
  • Dispose()/DisposeAsync()

Please offer thoughts and ideas. This will be shaped into a proper proposal over time.

@AraHaan
Copy link
Contributor

AraHaan commented Aug 8, 2022

I would say have them implement both IDisposable and Async Disposable.

Also I would like it to where when each plugin gets loaded they are each loaded into their own AssemblyLoadContext which Remora.Plugins could track internally (and could be collectible so then Unload() can be called).

@AraHaan
Copy link
Contributor

AraHaan commented Aug 8, 2022

Also this could remove code like this in my bot:

image

And possibly simplify this:

image

to just:

  • throw if disposed.
  • add serilog.
  • add plugin service to DI container (which will also create internal service collections for each plugin in the plugintree which then can be initialized through DI).
  • Have PluginTree be a scoped service on the application's original service provider (where PluginTree does the whole keeping track of ALC's and internal service providers for each plugin)?

@AraHaan
Copy link
Contributor

AraHaan commented Aug 9, 2022

hmm after thinking about this further I think the best option would be for an fake "service collection" that could be used for plugins to where their services can be added in a scoped way but still being able to access things from the original service provider (with the fake "service collection" able to build a fake "service provider" that will be able to obtain services from both the fake and the real one by first looking if the service is in the fake, and if it's not there it checks the real one.

@Foxtrek64
Copy link
Contributor Author

I wonder if we could implement this via our own concrete service provider, one that has the concept of "owned services". When registering a service, the consumer would do it exactly the same as the normally would, but those would effectively go into a scoped service provider specifically for that plugin. When requesting items from the service provider, all sources are searched so it appears to only be a single collection from the consumer.

This allows us to unload a plugin's services by simply disposing the scoped provider of the service we're taking down (as well as all dependent plugins, optional or not).

If we're doing that, I think it may also be good to implement named services, perhaps in the format of PluginId:ServiceName. This allows for multiple plugins to register things like a memory cache without worrying about collisions (though if we make a separate service provider, even faked, for each plugin, that could be solved simply by getting the plugin's own instance first and searching other stores separately).

There may be some consideration into private/public types too for each service registration. A plugin dealing with sensitive information, like an in-game currency, shouldn't be susceptible to tampering just because the name of the service that manages that currency is well known. This could possibly be accomplished via a separate provider for private or public registrations or a property on the registration itself, depending on how we want to scope things.

I'll work on the provider service in a separate library since I think it's a bit out of scope for this plugins library, and it's generic enough where others may find it useful.

@Foxtrek64
Copy link
Contributor Author

@AraHaan
Copy link
Contributor

AraHaan commented Aug 28, 2022

I have implemented a working service provider but it means registering a custom PluginServiceProviderFactory which then return an static instance that then lists the real service providers internally into a dictionary separated by plugin file name (without extension).

Edit: It works, but does not resolve anything properly that are not a part first service provider added inside of the PluginServiceProvider.

@AraHaan
Copy link
Contributor

AraHaan commented Aug 29, 2022

Turns out there is only a single place we could change to make an mutable service provider (I think) and we could technically copy the code from dotnet/runtime for it.

@AraHaan
Copy link
Contributor

AraHaan commented Aug 29, 2022

Also @Foxtrek64 on realoadable plugins that contain singleton services, should those singleton services be removed from it's cache inside of the custom ServiceProvider and then when when plugins are reloaded it is readded and a new instance of it be created?

Or should singleton service instances be avoided by plugins entirely?

@AraHaan
Copy link
Contributor

AraHaan commented Aug 29, 2022

Service Provider idea here: https://github.com/LuzFaltex/LuzFaltex.Extensions.DependencyInjection

And implemented:
image

@AraHaan
Copy link
Contributor

AraHaan commented Sep 2, 2022

It fully works, the only thing to wait for now is 2022.48 on Remora.Discord and it should then work for Discord Bots.

Foxtrek64#1

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

No branches or pull requests

2 participants