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

Implement OSGi Service Loader Mediator Specification #295

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion bundles/org.eclipse.osgi/META-INF/MANIFEST.MF
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,9 @@ Provide-Capability: osgi.service; objectClass:List<String>="org.osgi.service.log
osgi.service; objectClass:List<String>="org.eclipse.osgi.signedcontent.SignedContentFactory"; uses:="org.eclipse.osgi.signedcontent",
osgi.service; objectClass:List<String>="org.osgi.service.condition.Condition"; osgi.condition.id="true"; uses:="org.osgi.service.condition",
osgi.serviceloader;osgi.serviceloader="org.osgi.framework.connect.ConnectFrameworkFactory";register:="org.eclipse.osgi.launch.EquinoxFactory",
osgi.serviceloader;osgi.serviceloader="org.osgi.framework.launch.FrameworkFactory";register:="org.eclipse.osgi.launch.EquinoxFactory"
osgi.serviceloader;osgi.serviceloader="org.osgi.framework.launch.FrameworkFactory";register:="org.eclipse.osgi.launch.EquinoxFactory",
osgi.extender;osgi.extender="osgi.serviceloader.registrar";version:Version="1.0",
osgi.extender;osgi.extender="osgi.serviceloader.processor";version:Version="1.0"
Bundle-Name: %systemBundle
Bundle-SymbolicName: org.eclipse.osgi; singleton:=true
Bundle-Activator: org.eclipse.osgi.internal.framework.SystemBundleActivator
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.eclipse.osgi.internal.framework.EquinoxContainer;
import org.eclipse.osgi.internal.hooks.DevClassLoadingHook;
import org.eclipse.osgi.internal.hooks.EclipseLazyStarter;
import org.eclipse.osgi.internal.hooks.ServiceLoaderMediatorHook;
import org.eclipse.osgi.internal.signedcontent.SignedBundleHook;
import org.eclipse.osgi.internal.weaving.WeavingHookConfigurator;
import org.eclipse.osgi.util.ManifestElement;
Expand Down Expand Up @@ -113,6 +114,7 @@ public void initialize() {
addClassLoaderHook(new DevClassLoadingHook(container.getConfiguration()));
addClassLoaderHook(new EclipseLazyStarter(container));
addClassLoaderHook(new WeavingHookConfigurator(container));
addClassLoaderHook(new ServiceLoaderMediatorHook());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be provided as a fragment of org.eclipse.osgi like:

Fragment-Host: org.eclipse.osgi;bundle-version="[3.10.0,4.0.0)"

and use a hookconfigurators.properties to specify an implementation of org.eclipse.osgi.internal.hookregistry.HookConfigurator like

hook.configurators= \
org.eclipse.equinox.weaving.hooks.WeavingHook

Then you can add your ClassLoaderHook with HookConfigurator.addHooks(HookRegistry)

We should keep this out of the framework as long as service loader mediator specification is not part of the OSGi Core specification.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered this as well, mainly for the point you mentioned below that it would allow later Java versions than 8 (9 would already be a big win).
But the big drawback of this approach is that one has to list the fragment as framework-extension in a system-property of the runtime. For example in a launch config:
-Dosgi.framework.extensions=reference:file:${project_loc:/org.eclipse.equinox.spi}, in a product definition this would usually be -Dosgi.framework.extensions=reference:file:org.eclipse.equinox.spi, assuming the spi and o.e.osgi jar are colocated.
Or is there another way to load a framework-extension with the framework's classloader so that the hookconfigurators.properties are found?
Because actually another goal of this was to avoid the need (if possible) for any configuration that it just works out of the box.
For the build-time some p2-instructions could be added to the fragment, but PDE does not yet have any help for that as far as I know.

configurators.add(SignedBundleHook.class.getName());
configurators.add(CDSHookConfigurator.class.getName());
loadConfigurators(configurators, errors);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,169 @@
package org.eclipse.osgi.internal.hooks;

import java.io.FileNotFoundException;
import java.io.IOException;
import java.lang.reflect.Method;
import java.net.URL;
import java.util.Arrays;
import java.util.Collections;
import java.util.Enumeration;
import java.util.HashSet;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.ServiceLoader;
import java.util.Set;
import java.util.function.Function;
import java.util.function.Supplier;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import java.util.stream.Stream;
import org.eclipse.osgi.internal.hookregistry.ClassLoaderHook;
import org.eclipse.osgi.internal.loader.ModuleClassLoader;
import org.osgi.framework.Constants;
import org.osgi.framework.wiring.BundleRequirement;
import org.osgi.framework.wiring.BundleWire;
import org.osgi.framework.wiring.BundleWiring;

public class ServiceLoaderMediatorHook extends ClassLoaderHook {

@SuppressWarnings({ "unchecked", "rawtypes" })
private Supplier<Optional<Class<?>>> getCallerClassComputer() {
try { // The Java-9+ way
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you place this in a framework fragment then we can make it depend on a higher osgi.ee than Java 8.

Class<?> stackWalkerClass = Class.forName("java.lang.StackWalker"); //$NON-NLS-1$
Class stackWalkerOption = Class.forName("java.lang.StackWalker$Option"); //$NON-NLS-1$
Enum<?> retainClassReference = Enum.valueOf(stackWalkerOption, "RETAIN_CLASS_REFERENCE"); //$NON-NLS-1$
Object stackWalker = stackWalkerClass.getMethod("getInstance", Set.class, int.class).invoke(null, //$NON-NLS-1$
new HashSet<>(Arrays.asList(retainClassReference)), 15);

Method stackWalkerWalk = stackWalkerClass.getMethod("walk", Function.class); //$NON-NLS-1$
Class<?> stackFrameClass = Class.forName("java.lang.StackWalker$StackFrame"); //$NON-NLS-1$
Method stackFrameGetDeclaringClass = stackFrameClass.getMethod("getDeclaringClass"); //$NON-NLS-1$

Function<Stream<?>, Optional<Class<?>>> function = frames -> frames.<Class<?>>map(f -> {
try {
return (Class<?>) stackFrameGetDeclaringClass.invoke(f);
} catch (ReflectiveOperationException e) {
return null;
}
}).filter(Objects::nonNull).limit(15)
.filter(c -> c == ServiceLoader.class || c.getEnclosingClass() == ServiceLoader.class).findFirst();

return () -> {
try {
return (Optional<Class<?>>) stackWalkerWalk.invoke(stackWalker, function);
} catch (ReflectiveOperationException e) {
return Optional.empty();
}
};
} catch (ReflectiveOperationException e) { // ignore
}
try { // Try the Java-1.8 way
Class<?> reflectionClass = Class.forName("sun.reflect.Reflection"); //$NON-NLS-1$
Method getCallerClass = Objects.requireNonNull(reflectionClass.getMethod("getCallerClass", int.class)); //$NON-NLS-1$
return () -> IntStream.range(0, 15).<Class<?>>mapToObj(i -> {
try {
return (Class<?>) getCallerClass.invoke(null, i);
} catch (ReflectiveOperationException e) {
return null;
}
}).filter(Objects::nonNull)
.filter(c -> c == ServiceLoader.class || c.getEnclosingClass() == ServiceLoader.class).findFirst();

} catch (ReflectiveOperationException e) { // ignore an try Java-8 way
}
throw new AssertionError("Neither the Java-8 nor the Java-9+ way to obtain the caller class is available"); //$NON-NLS-1$
}

private static final String SERVICE_NAME_PREFIX = "META-INF/services/"; //$NON-NLS-1$

private final ThreadLocal<String> latestServiceName = new ThreadLocal<>();

private final Supplier<Optional<Class<?>>> callerClass = getCallerClassComputer();

@Override
public Enumeration<URL> preFindResources(String name, ModuleClassLoader classLoader) throws FileNotFoundException {
if (name.startsWith(SERVICE_NAME_PREFIX)) {
Stream<URL> serviceProviderFiles = searchProviders(name, classLoader, cl -> {
try {
Enumeration<URL> resources = cl.getResources(name);
return Collections.list(resources).stream();
} catch (IOException e) {
return Stream.empty();
}
});
if (serviceProviderFiles != null) {
latestServiceName.set(name);
List<URL> services = serviceProviderFiles.collect(Collectors.toList());
return Collections.enumeration(services);
}
}
return null;
}

@Override
public Class<?> preFindClass(String name, ModuleClassLoader classLoader) throws ClassNotFoundException {
String serviceName = latestServiceName.get(); // This cannot be removed if subsequent class load other
// providers for the same service
if (serviceName != null) {
Stream<Class<?>> services = searchProviders(serviceName, classLoader, cl -> {
try {
return Stream.of(cl.loadClass(name));
} catch (ClassNotFoundException | NoClassDefFoundError e) { // ignore
return Stream.empty();
}
});
if (services != null) {
Optional<Class<?>> findFirst = services.findFirst();
return findFirst.orElse(null);
}
}
return null;
}

private <T> Stream<T> searchProviders(String name, ModuleClassLoader classLoader,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of dynamically searching this list I would use BundleTracker in the Hook that collects bundles and read their META-INF/services in a datastructure that can then be used to lookup current active providers, that has several advantages:

  1. no need to query the wiring and iterate the bundles
  2. easier to debug as there is some current state to inspect
  3. instead of the latestServiceName we can inspect if the bundle tries to load a class that is an SPI of another bundle and then inspect the stack, requiring a certain call order seems a bit fragile if something changes in the implementation of service loader.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of dynamically searching this list I would use BundleTracker in the Hook that collects bundles and read their META-INF/services in a datastructure that can then be used to lookup current active providers, that has several advantages:

IIRC the service loader mediator specification requires the impl to look at the wiring to allow a bundle to "use" only the providers it is wired to.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of dynamically searching this list I would use BundleTracker in the Hook that collects bundles and read their META-INF/services in a datastructure that can then be used to lookup current active providers, that has several advantages:

IIRC the service loader mediator specification requires the impl to look at the wiring to allow a bundle to "use" only the providers it is wired to.

I think so as well. However having a state also allows to avoid the ThreadLocal.
In general I try to have a somewhat hybrid approach that maintains a state with all services, but when there is a query for services checks the current wires if that bundle is permitted to really obtain them.
The goal is also a bit to demonstrate how this could be done totally without (or at least with minimal) metadata as intended in osgi/osgi#372. If that issue is once resolved all the checks for wires could simply be removed and everything possible could be supplied.

Function<ClassLoader, Stream<T>> mapper) {
BundleWiring wiring = classLoader.getBundle().adapt(BundleWiring.class);
if (requiresServiceLoaderProcessor(wiring) && isCalledFromServiceLoader()) {
return Stream.concat(Stream.of(classLoader), providerClassLoaders(name, wiring)).flatMap(mapper);
}
return null;
}

private boolean requiresServiceLoaderProcessor(BundleWiring wiring) {
List<BundleWire> requiredWires = wiring.getRequiredWires("osgi.extender"); //$NON-NLS-1$
if (requiredWires.isEmpty()) {
return false;
}
return requiredWires.stream().anyMatch(w -> w.getRequirement().getDirectives().getOrDefault("filter", "") //$NON-NLS-1$//$NON-NLS-2$
.contains("(osgi.extender=osgi.serviceloader.processor)") //$NON-NLS-1$
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More efficient and reliable to check the provider attribute osgi.extender equals osgi.serviceloader.processor I think and assume the filter matched it instead of trying to do a simple string compare here.

&& w.getProvider().getBundle().getBundleId() == Constants.SYSTEM_BUNDLE_ID);
}

private boolean isCalledFromServiceLoader() {
Optional<Class<?>> caller = callerClass.get();
// TODO: If Java-9+ is required one day, just use the following code
// ClassLoader thisLoader = ServiceLoaderMediatorHook.class.getClassLoader();
// Optional<Class<?>> caller = WALKER.walk(frames -> frames.<Class<?>>map(f -> f.getDeclaringClass())
// .dropWhile(c -> c.getName().startsWith("org.eclipse.osgi") && c.getClassLoader() == thisLoader) //$NON-NLS-1$
// .findFirst());
return caller.isPresent() && caller.get().getEnclosingClass() == ServiceLoader.class;
}

private static Stream<ClassLoader> providerClassLoaders(String name, BundleWiring wiring) {
List<BundleWire> requiredWires = wiring.getRequiredWires("osgi.serviceloader"); //$NON-NLS-1$
if (!requiredWires.isEmpty()) {
String serviceName = name.substring(SERVICE_NAME_PREFIX.length());
return requiredWires.stream().filter(wire -> {
BundleRequirement requirement = wire.getRequirement();
String filterDirective = requirement.getDirectives().get("filter"); //$NON-NLS-1$
Object serviceLoaderAttribute = wire.getCapability().getAttributes().get("osgi.serviceloader"); //$NON-NLS-1$
return serviceName.equals(serviceLoaderAttribute)
&& ("(osgi.serviceloader=" + serviceName + ")").equals(filterDirective); //$NON-NLS-1$ //$NON-NLS-2$
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this extra check on filter is needed.

}).map(wire -> wire.getProviderWiring().getClassLoader());

}
return Stream.empty();
}

}
Loading