SpongeAPI icon indicating copy to clipboard operation
SpongeAPI copied to clipboard

Dependency Injection Enhancements

Open Xfel opened this issue 6 years ago • 3 comments

This is meant as a followup to #1156, taking a more concrete approach.

Currently, Guice is used to perform dependency injection on the plugin instance. The di environment contains a lot of useful injectable objects like config directories and task executors, but using these in other classes is not straightforward.

Thanks to guice's implicit bindings, any class that is referenced by an @Inject is automatically added to the injector. There are XX problems with this approach:

  • In order to be picked up as implicit binding, the class must be referenced, even if the main plugin class doesn't need it. Since many methods require the main plugin class to be passed as parameter, this will almost always create a reference cycle. However, this can be resolved by using PluginContainer.
  • Using @Inject in the plugin main class means that any injected object must be instantiated during the Construction lifecycle event. If we want to select an implementation dynamically, ie a data store based on a flatfile or a database, we need to read the corresponding configuration file in that phase too.

Additionally, while the plugin main class is registered with the event bus automatically, the other injected classes are not. Forge has the @Mod.EventBusSubscriber annotation to mark classes that should be registered automatically, but sponge has nothing similar.

As an improvement on these issues, I suggest the following steps:

  1. Add a class annotation @Component. Any annotated class will be bound into the plugin injector. Additionally, one should be able to annotate guice Module subclasses, which would also be added to the plugin injector. On the implementation side, forge already provides an index of all annotated classes in ASMDataTable. For SpongeVanilla, we would need to adapt the annotation scanner; the current version only searches for @Plugin. Open questions:
    • Should Modules also use @Component or a separate annotation?
    • Which scope should be used for components? Guice defaults to a prototype approach where every request gets a new instance, but for plugins, defaulting to Singleton would probably be better. Alternatively, we could take the CDI approach and pick up any Class annotated with a scope annotation.
  2. Component classes with @Listener methods should be able to have them registered automatically. In order to deal with the early instance creation, the actual listener object should only be created once the first event is fired. That does require some changes to the event listener implementation, which would take a Provider instead of the listener object instance. Querying the provider every time would also induce a small runtime overhead, but we could optimize this for singletons by only querying the provider once and storing the result in the event listener. Open questions:
    • Should the automatic registration be opt-in or opt-out?

Xfel avatar Dec 08 '18 10:12 Xfel

In regards to scope, I've been wondering if we could make a 'game' scope or how that would be managed, e.g. single player starts, plays, stops a server, picks a different world, starts, plays, stops.

However I've seen numerous bugs in the Minecraft Vanilla client, that show on rare occasion, it's possible to either connect to 2 servers at once, e.g. get the chat from one, but see the lobby of another.

Or (particularly when hardcore worlds are 'deleted') play 2 different worlds and see the world time in the client, flicker between the 2 correct values.

These are extreme edge cases, and not entirely well understood, and I also don't understand the threading model of what's happening when this occurs.

I only bring it up, because I've previously investigated trying to get 'per world' or 'per universe' injection scope working before. But ran into difficulties when I found most examples expect it to either be on a thread local basis, or have the scopes not 'interleave' when they start and end.

This probably all sounds batshit crazy, but

TLDR(too long didn't read);

I would be interested in seeing per world, per universe, per game, per user? scopes, if such a thing is even possible and I'm not just totally misunderstanding DI scoping.

ryantheleach avatar Dec 08 '18 16:12 ryantheleach

Per world scopes would be useful on Lantern implementation, where each world is processed by its own thread. Othewise i dont really see a reason to introduce per player/per world scope. While it would be useful in some cases its now worth due to increased overhead.

Some nice-to-have features:

  • All sponge Services (objects which are managed by ServiceManager) are in fact singletons, therefore we should be able to inject them

  • If a plugin X has dependency on plugin Y, and plugin Y registeres a service S sponge should be able to inject serivce S within plugin X scope.

  • if the dependency is optional sponge should be able to handle for example this:

    @Inject
    private Optional<S> thirdPartyService
    

Injecting empty optional if the theres no service S cached in ServiceManager

Annotation @Service

  • Similarily to @Component as suggested above + its instance is cached within sponge servicemanager

Annotation @PostConstruct

  • Annotation for a method, its used for @Service initialization, once all fields within the same class are successfully injected
    @Service 
    public class S {

    @Inject
    private AnotherService as;

    @PostConstruct
    public void init() {
        as is not null

Another problem is that people are shading huge dependencies into their plugin. Any deep class-path scanning could make server startup much longer. This could be fixed by an annotation processor, (similar to one which reads plugin annotation and responsible for mcmodinfo file). The processor would find all candidate classes having @Component/@Service/@Listener/... annotation during compilation and would create a static mapping file which could sponge read instead of scan all classes within a plugin jar.

NeumimTo avatar Dec 08 '18 17:12 NeumimTo

@NeumimTo Deep classpath scanning should not be an issue; both forge and spongevanilla do that already to detect @Mod/@Plugin and similar.

On injecting services: The issue here is that services can change, and we also cannot guarantee that the required service is available at construction time, so we cannot use @Inject for them. We could however use a different annotation and implement our own injection mechanism for it.

On scoping: It's certainly possible to create scopes based on players/worlds/etc, since those are stored in the cause management. This would not work in async task threads, but in that it is similar to the request/session scope in spring/cdi.

Xfel avatar Dec 08 '18 18:12 Xfel