Skip to content

Commit 788750b

Browse files
Load stable plugins as synthetic modules (#91869)
* Load nonmodular stable plugins as ubermodules We can use the ubermodule classloader to load stable plugins if the plugin descriptor does not give us a module to load from the plugin bundle. We create the synthetic module name by munging the plugin name into a suitable form. For testing, we have to provide the uber module classloader read access to the app classloader's unnamed module, where the stable api modules are loaded by default. * Update docs/changelog/91869.yaml
1 parent 4026ff2 commit 788750b

File tree

4 files changed

+66
-0
lines changed

4 files changed

+66
-0
lines changed

docs/changelog/91869.yaml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 91869
2+
summary: Load stable plugins as synthetic modules
3+
area: Infra/Plugins
4+
type: enhancement
5+
issues: []

server/src/main/java/org/elasticsearch/plugins/PluginsService.java

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -523,12 +523,33 @@ static LayerAndLoader createPlugin(
523523
extendedPlugins.stream().map(LoadedPlugin::layer)
524524
).toList();
525525
return createPluginModuleLayer(bundle, pluginParentLoader, parentLayers);
526+
} else if (plugin.isStable()) {
527+
logger.debug(() -> "Loading bundle: " + plugin.getName() + ", non-modular as synthetic module");
528+
return LayerAndLoader.ofLoader(
529+
UberModuleClassLoader.getInstance(
530+
pluginParentLoader,
531+
ModuleLayer.boot(),
532+
"synthetic." + toModuleName(plugin.getName()),
533+
bundle.allUrls,
534+
Set.of("org.elasticsearch.server") // TODO: instead of denying server, allow only jvm + stable API modules
535+
)
536+
);
526537
} else {
527538
logger.debug(() -> "Loading bundle: " + plugin.getName() + ", non-modular");
528539
return LayerAndLoader.ofLoader(URLClassLoader.newInstance(bundle.urls.toArray(URL[]::new), pluginParentLoader));
529540
}
530541
}
531542

543+
// package-visible for testing
544+
static String toModuleName(String name) {
545+
String result = name.replaceAll("\\W+", ".") // replace non-alphanumeric character strings with dots
546+
.replaceAll("(^[^A-Za-z_]*)", "") // trim non-alpha or underscore characters from start
547+
.replaceAll("\\.$", "") // trim trailing dot
548+
.toLowerCase(Locale.getDefault());
549+
assert ModuleSupport.isPackageName(result);
550+
return result;
551+
}
552+
532553
private static void checkDeprecations(
533554
PluginIntrospector inspector,
534555
List<PluginDescriptor> pluginDescriptors,

server/src/main/java/org/elasticsearch/plugins/UberModuleClassLoader.java

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -280,6 +280,13 @@ protected Class<?> loadClass(String cn, boolean resolve) throws ClassNotFoundExc
280280
}
281281
}
282282

283+
// For testing in cases where code must be given access to an unnamed module
284+
void addReadsSystemClassLoaderUnnamedModule() {
285+
moduleController.layer()
286+
.modules()
287+
.forEach(module -> moduleController.addReads(module, ClassLoader.getSystemClassLoader().getUnnamedModule()));
288+
}
289+
283290
/**
284291
* Returns the package name for the given class name
285292
*/

server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717
import org.elasticsearch.env.Environment;
1818
import org.elasticsearch.env.TestEnvironment;
1919
import org.elasticsearch.index.IndexModule;
20+
import org.elasticsearch.plugin.analysis.api.CharFilterFactory;
21+
import org.elasticsearch.plugins.scanners.PluginInfo;
2022
import org.elasticsearch.plugins.spi.BarPlugin;
2123
import org.elasticsearch.plugins.spi.BarTestService;
2224
import org.elasticsearch.plugins.spi.TestService;
@@ -823,6 +825,23 @@ public Reader create(Reader reader) {
823825
assertThat(pluginInfos.get(0).descriptor().getName(), equalTo("stable-plugin"));
824826
assertThat(pluginInfos.get(0).descriptor().isStable(), is(true));
825827

828+
// check ubermodule classloader usage
829+
Collection<PluginInfo> stablePluginInfos = pluginService.getStablePluginRegistry()
830+
.getPluginInfosForExtensible("org.elasticsearch.plugin.analysis.api.CharFilterFactory");
831+
assertThat(stablePluginInfos, hasSize(1));
832+
ClassLoader stablePluginClassLoader = stablePluginInfos.stream().findFirst().orElseThrow().loader();
833+
assertThat(stablePluginClassLoader, instanceOf(UberModuleClassLoader.class));
834+
835+
if (CharFilterFactory.class.getModule().isNamed() == false) {
836+
// test frameworks run with stable api classes on classpath, so we
837+
// have no choice but to let our class read the unnamed module that
838+
// owns the stable api classes
839+
((UberModuleClassLoader) stablePluginClassLoader).addReadsSystemClassLoaderUnnamedModule();
840+
}
841+
842+
Class<?> stableClass = stablePluginClassLoader.loadClass("p.A");
843+
assertThat(stableClass.getModule().getName(), equalTo("synthetic.stable.plugin"));
844+
826845
// TODO should we add something to pluginInfos.get(0).pluginApiInfo() ?
827846
} finally {
828847
closePluginLoaders(pluginService);
@@ -838,6 +857,20 @@ public void testCanCreateAClassLoader() {
838857
assertEquals(this.getClass().getClassLoader(), loader.getParent());
839858
}
840859

860+
public void testToModuleName() {
861+
assertThat(PluginsService.toModuleName("module.name"), equalTo("module.name"));
862+
assertThat(PluginsService.toModuleName("module-name"), equalTo("module.name"));
863+
assertThat(PluginsService.toModuleName("module-name1"), equalTo("module.name1"));
864+
assertThat(PluginsService.toModuleName("1module-name"), equalTo("module.name"));
865+
assertThat(PluginsService.toModuleName("module-name!"), equalTo("module.name"));
866+
assertThat(PluginsService.toModuleName("module!@#name!"), equalTo("module.name"));
867+
assertThat(PluginsService.toModuleName("!module-name!"), equalTo("module.name"));
868+
assertThat(PluginsService.toModuleName("module_name"), equalTo("module_name"));
869+
assertThat(PluginsService.toModuleName("-module-name-"), equalTo("module.name"));
870+
assertThat(PluginsService.toModuleName("_module_name"), equalTo("_module_name"));
871+
assertThat(PluginsService.toModuleName("_"), equalTo("_"));
872+
}
873+
841874
static final class Loader extends ClassLoader {
842875
Loader(ClassLoader parent) {
843876
super(parent);

0 commit comments

Comments
 (0)