Plugin entry points using Settings.getAsClass are broken

[rjernst]

I think the underlying issue in #12643 is how SpawnModules works. This is an entry point for various pluggable classes that goes out and tries to load plugged in modules from the classloader. However, this no longer works now that plugins get their own classloader.

I think the right thing to do is eliminate SpawnModules altogether. Any plugin entry points should:

1. Have a method on a module the plugin can register new classes with
2. Have tests that these entry points actually work with a class outside of the core classloader

I think this has to block 2.0 beta, otherwise many plugins are just broken (like all the discovery plugins right now).

[rmuir]

This code is really broken, this class needs to die right here and now, there is no other way about it.

[uboness]

I'm sorry... I don't see any problem with SpawnModules itself here. this construct is there to just promote modularity. The real problem here is that we currently allow/rely on configuring classes in settings - that's the wrong thing here... the fact that in the DiscoveryModule we do it when we spawn modules doesn't make the SpawnModules wrong (we could just as have that wrong everywhere else in the codebase)

I agree with that in general, all plugin points should be exposed as methods on the relevant modules (and the DiscoveryModule is a great example where it's done wrong).

[rmuir]

I'll bite: SpawnShittyModules is broken because it enforces no contract whatsoever, its the most worthless interface: the last fucking thing we need is 10-20 class instantiation mechanisms.

I consider this blocker issue open until this class is removed. If you like it, go get it from source history.

[uboness]

I'll bite:

sure... I'm immuned :)

SpawnShittyModules is broken because it enforces no contract whatsoever, its the most worthless interface: the last fucking thing we need is 10-20 class instantiation mechanisms.

I don't know SpawnShittyModules... can't find it... but if you do, please go ahead and remove it (it sounds like...hmmm... a shitty class)

This issue however is about SpawnModules and its contract is as simple as "a module that spawns other modules"... one could have named it CompoundModule which is a module that is made out of other modules, but that's not the case... the name is SpawnModules. Would have been nice if this would be ingrained in the AbstractModule class, but it isn't. Guice's AbstractModule#install method comes close, but it is meant to be used within the configure method. The SpawnModules enables adding its sub-modules before the injector is created and therefore works well with the PreProcessModule construct.

If you want to remove it, sure... go ahead, but at the same time provide a good alternative that doesn't force a single place in the code knowing about all the services in the codebase(s). In other words, If modules A depends on a1 and a2 modules, and a1 defines an internal service s1, there's not reason for A to directly know about s1- it's an implementation detail of a1 and a1 only.

And if you do provide an alternative that enables same level of modularity, that'd be awesome. But then it's a cleanup as it doesn't really fix any bugs, and therefore I don't see any reason for it to be a blocker for beta1. The referenced bug in this issue has nothing to do with the sole existence of SpawnModules class. It's an implementation bug that can be implemented with or without this class (if you load classes by names from settings, you're bound to hit it, regardless of where you do it)

[rmuir]

If you want to remove it, sure... go ahead, but at the same time provide a good alternative that doesn't force a single place in the code knowing about all the services in the codebase(s). In other words, If modules A depends on a1 and a2 modules, and a1 defines an internal service s1, there's not reason for A to directly know about s1- it's an implementation detail of a1 and a1 only.

I care very very little about this.

The top priority is making sure that the code we load up and use, the classes themselves, are correct. This is fundamental to a working application.

[rjernst]

The underlying issue (which took me a lot more reading to understand thanks to tons of concepts with modules, services, components, etc) is Settings.getAsClass. This is used as a "plugin entry point by naming convention". While I still agree SpawnModules is shitty (it is too hard to comprehend the recursive nature of module loading with it), I will rename this issue to reflect the blocker: Settings.getAsClass must go.

[costin]

As a side comment, the problem with Settings object acting as a ClassLoader is problematic because Settings tend to be global, for the entire ES instance while the ClassLoader clearly per-plugin (though in ES 1.x it was also global).
We can have Guice inject plugin-aware Settings but still, one can leak the Settings object to a different class by accident and one ends up with GC and CNFE issues.

Long story short, removing any class loading functionality from Settings is cleaner since as it stands right now, there are mixed concerns with different lifecycles.
We could simply inject the ClassLoader to the plugin starting hook or simply ask the plugin to determine that itself (getClass().getClassLoader()). It doesn't introduce any global registry (which is problematic in terms of cleanup) and further more, makes it difficult for plugins to get a hold of each-others ClassLoaders.

[uboness]

@costin absolutely... agree that Settings should not hold a class loader at all. Even with this PR in things will still be broken for the exactly same reason you mention and we should not let anyone get a class loader from Settings (as it's bound to fail for plugins)