You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2022/03/18 07:45:23 UTC
[GitHub] [logging-log4j2] ppkarwasz opened a new pull request #804: [LOG4J2-3427] Replace ServiceLoader calls with ServiceLoaderUtil
ppkarwasz opened a new pull request #804:
URL: https://github.com/apache/logging-log4j2/pull/804
In an attempt to uniformize the calls to `ServiceLoader` and work around its limitations, this replaces the `ServiceLoader#load` call sites with `ServiceLoaderUtil#loadServices`.
`ServiceLoaderUtil` is also modified to return `Stream` instead of a `List`, hence allowing for a more comfortable filtering and sorting of the results.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a change in pull request #804: [LOG4J2-3427] Replace ServiceLoader calls with ServiceLoaderUtil
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on a change in pull request #804:
URL: https://github.com/apache/logging-log4j2/pull/804#discussion_r831830675
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/util/ServiceLoaderUtil.java
##########
@@ -110,28 +135,64 @@ private ServiceLoaderUtil() {
* @param logger An action to perform for each broken service.
* @param services A list to add the services.
*/
- private static <T> void addServices(final Class<T> serviceType, final ClassLoader classLoader,
- final Consumer<Throwable> logger, final Collection<T> services) {
- final Iterator<T> iterator = ServiceLoader.load(serviceType, classLoader).iterator();
- while (iterator.hasNext()) {
- try {
- final T service = iterator.next();
- if (classLoader.equals(service.getClass().getClassLoader())) {
- services.add(service);
- }
- } catch (ServiceConfigurationError e) {
- if (logger != null) {
- logger.accept(e);
- }
- }
- }
+ private static <T> Stream<T> loadClassloaderServices(final Class<T> serviceType, final ClassLoader classLoader,
+ final boolean verbose) {
+ return StreamSupport.stream(new ServiceLoaderSpliterator<T>(serviceType, classLoader, verbose), false);
}
- private static <T> void addOsgiServices(final Class<T> serviceType, final Collection<T> services) {
+ private static <T> Stream<? extends T> loadOsgiServices(final Class<T> serviceType) {
final Iterable<? extends T> iterable = org.glassfish.hk2.osgiresourcelocator.ServiceLoader
.lookupProviderInstances(serviceType);
- if (iterable != null) {
- iterable.forEach(services::add);
+ return iterable != null ? StreamSupport.stream(iterable.spliterator(), false) : Stream.empty();
+ }
+
+ private static class ServiceLoaderSpliterator<S> implements Spliterator<S> {
Review comment:
I used the `stream()` method in the Java 9 specific class, so we don't forget.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory commented on a change in pull request #804: [LOG4J2-3427] Replace ServiceLoader calls with ServiceLoaderUtil
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #804:
URL: https://github.com/apache/logging-log4j2/pull/804#discussion_r830027870
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/util/LoaderUtil.java
##########
@@ -120,6 +120,22 @@ public ClassLoader run() {
return classLoaders.toArray(EMPTY_CLASS_LOADER_ARRAY);
}
+ /**
+ * Gets the the give classloader and its ancestors.
Review comment:
Give -> given?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] rgoers commented on a change in pull request #804: [LOG4J2-3427] Replace ServiceLoader calls with ServiceLoaderUtil
Posted by GitBox <gi...@apache.org>.
rgoers commented on a change in pull request #804:
URL: https://github.com/apache/logging-log4j2/pull/804#discussion_r830447038
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/util/ServiceLoaderUtil.java
##########
@@ -110,28 +139,64 @@ private ServiceLoaderUtil() {
* @param logger An action to perform for each broken service.
* @param services A list to add the services.
*/
- private static <T> void addServices(final Class<T> serviceType, final ClassLoader classLoader,
- final Consumer<Throwable> logger, final Collection<T> services) {
- final Iterator<T> iterator = ServiceLoader.load(serviceType, classLoader).iterator();
- while (iterator.hasNext()) {
- try {
- final T service = iterator.next();
- if (classLoader.equals(service.getClass().getClassLoader())) {
- services.add(service);
- }
- } catch (ServiceConfigurationError e) {
- if (logger != null) {
- logger.accept(e);
- }
- }
- }
+ private static <T> Stream<T> loadClassloaderServices(final Class<T> serviceType, final ClassLoader classLoader,
+ final boolean verbose) {
+ return StreamSupport.stream(new ServiceLoaderSpliterator<T>(serviceType, classLoader, verbose), false);
}
- private static <T> void addOsgiServices(final Class<T> serviceType, final Collection<T> services) {
+ private static <T> Stream<? extends T> loadOsgiServices(final Class<T> serviceType) {
final Iterable<? extends T> iterable = org.glassfish.hk2.osgiresourcelocator.ServiceLoader
.lookupProviderInstances(serviceType);
- if (iterable != null) {
- iterable.forEach(services::add);
+ return iterable != null ? StreamSupport.stream(iterable.spliterator(), false) : Stream.empty();
+ }
+
+ private static class ServiceLoaderSpliterator<S> implements Spliterator<S> {
+
+ private final Iterator<S> serviceIterator;
+ private final ClassLoader classLoader;
+ private final Logger logger;
+ private final String serviceName;
+
+ public ServiceLoaderSpliterator(final Class<S> serviceType, final ClassLoader classLoader,
+ final boolean verbose) {
+ this.serviceIterator = ServiceLoader.load(serviceType, classLoader).iterator();
Review comment:
This code will not work for applications using the module path. Currently, all calls to ServiceLoader happen in the jar that needs the service. However, according to https://openjdk.java.net/projects/jigsaw/spec/sotms/#services, if an automatic module declares a service it behaves as if it had a corresponding provides clause. Note that log4j-api has a module-info.java, so it will act as an explicitly named module. Currently, it declares uses clauses for the services log4j-api needs. This change will mean it will require a uses clauses for any use of ServiceLoader, which makes no sense and certainly won't work as expected. This code needs to be modified to ensure ServiceLoader is always called from the module/jar that requires the service as is done in master.
I should add that while 2.x does not fully support JPMS it currently should tolerate being used in a JPMS application as all the relevant jars, except for log4j-api, declare their automatic module name. We cannot break that.
I should also note that the ServiceLoaderUtil previously committed has the same problem. So this either has to be addressed or the previous commit will need to be reverted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a change in pull request #804: [LOG4J2-3427] Replace ServiceLoader calls with ServiceLoaderUtil
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on a change in pull request #804:
URL: https://github.com/apache/logging-log4j2/pull/804#discussion_r830463798
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/util/ServiceLoaderUtil.java
##########
@@ -110,28 +139,64 @@ private ServiceLoaderUtil() {
* @param logger An action to perform for each broken service.
* @param services A list to add the services.
*/
- private static <T> void addServices(final Class<T> serviceType, final ClassLoader classLoader,
- final Consumer<Throwable> logger, final Collection<T> services) {
- final Iterator<T> iterator = ServiceLoader.load(serviceType, classLoader).iterator();
- while (iterator.hasNext()) {
- try {
- final T service = iterator.next();
- if (classLoader.equals(service.getClass().getClassLoader())) {
- services.add(service);
- }
- } catch (ServiceConfigurationError e) {
- if (logger != null) {
- logger.accept(e);
- }
- }
- }
+ private static <T> Stream<T> loadClassloaderServices(final Class<T> serviceType, final ClassLoader classLoader,
+ final boolean verbose) {
+ return StreamSupport.stream(new ServiceLoaderSpliterator<T>(serviceType, classLoader, verbose), false);
}
- private static <T> void addOsgiServices(final Class<T> serviceType, final Collection<T> services) {
+ private static <T> Stream<? extends T> loadOsgiServices(final Class<T> serviceType) {
final Iterable<? extends T> iterable = org.glassfish.hk2.osgiresourcelocator.ServiceLoader
.lookupProviderInstances(serviceType);
- if (iterable != null) {
- iterable.forEach(services::add);
+ return iterable != null ? StreamSupport.stream(iterable.spliterator(), false) : Stream.empty();
+ }
+
+ private static class ServiceLoaderSpliterator<S> implements Spliterator<S> {
+
+ private final Iterator<S> serviceIterator;
+ private final ClassLoader classLoader;
+ private final Logger logger;
+ private final String serviceName;
+
+ public ServiceLoaderSpliterator(final Class<S> serviceType, final ClassLoader classLoader,
+ final boolean verbose) {
+ this.serviceIterator = ServiceLoader.load(serviceType, classLoader).iterator();
Review comment:
@rgoers,
Thanks for the explanations.
In `master` you use a lambda that calls [`ServiceLoader#load(ModuleLayer, Class)`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/ServiceLoader.html#load(java.lang.ModuleLayer,java.lang.Class)) to work around the caller semantics. This obviously can not work in 2.x, since `ModuleLayer` does not exist and we would need a Java9 version of each class that uses the `ServiceLoaderUtil`. Do you know any tricks that can be applied to Java 8? Would adding a `MethodHandles.Lookup` parameter work?
If it works, we could have a
```lang-java
ServiceLoaderUtil#loadServices(Class<T> serviceType, MethodHandles.Lookup lookup, boolean useTccl)
```
signature and look for services in the caller's classloader and the thread context classloader (if `useTccl=true`).
@jvz, any suggestions?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a change in pull request #804: [LOG4J2-3427] Replace ServiceLoader calls with ServiceLoaderUtil
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on a change in pull request #804:
URL: https://github.com/apache/logging-log4j2/pull/804#discussion_r830193883
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/util/ServiceLoaderUtil.java
##########
@@ -47,10 +57,20 @@ private ServiceLoaderUtil() {
*
* @param <T> The service type.
* @param serviceType The class of the service.
- * @return List of available services.
+ * @return Stream of available services.
*/
- public static <T> List<T> loadServices(final Class<T> serviceType) {
- return loadServices(serviceType, null);
+ public static <T> Stream<T> loadServices(final Class<T> serviceType) {
Review comment:
I added the `ServiceLoaderUtil` class after the 2.17.2 release, so there is no API break for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #804: [LOG4J2-3427] Replace ServiceLoader calls with ServiceLoaderUtil
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #804:
URL: https://github.com/apache/logging-log4j2/pull/804#discussion_r830207585
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/util/ServiceLoaderUtil.java
##########
@@ -60,43 +80,48 @@ private ServiceLoaderUtil() {
* @param serviceType The class of the service.
* @return A service of the given type or {@code null} if none is available.
*/
- public static <T> T getService(final Class<T> serviceType) {
+ public static <T> Optional<T> getService(final Class<T> serviceType) {
+ return getService(serviceType, LoaderUtil.getClassLoaders());
+ }
+
+ public static <T> Optional<T> getService(final Class<T> serviceType, ClassLoader classLoader) {
+ return getService(serviceType, LoaderUtil.getClassLoaders(classLoader));
+ }
+
+ private static <T> Optional<T> getService(final Class<T> serviceType, final ClassLoader[] classLoaders) {
+ final Logger logger = StatusLogger.getLogger();
// 1. Check for a system property with the FQCN of serviceType
- String serviceClassName = PropertiesUtil.getProperties().getStringProperty(serviceType.getName());
- if (Strings.isNotEmpty(serviceClassName)) {
- try {
- @SuppressWarnings("unchecked")
- final Class<T> serviceClass = (Class<T>) Class.forName(serviceClassName);
- if (serviceType.isAssignableFrom(serviceClass)) {
- return LoaderUtil.newInstanceOf(serviceClass);
- }
- getLogger().error("Unable to load service {}: it is not of the required type {}", serviceClass,
- serviceType);
- } catch (ClassNotFoundException | InstantiationException | IllegalAccessException
- | InvocationTargetException e) {
- getLogger().error("Unable to load service class {}", serviceClassName, e);
+ try {
+ final T service = LoaderUtil.newCheckedInstanceOfProperty(serviceType.getName(), serviceType);
+ if (service != null) {
+ return Optional.of(service);
}
+ } catch (ReflectiveOperationException | ClassCastException e) {
+ logger.error("Unable to load service class {}", serviceType.getName(), e);
}
// 2. Use the ServiceLoader:
- final List<T> services = loadServices(serviceType,
- t -> getLogger().info("Unable to load service class for service {}", serviceType.getName(), t));
+ final List<T> services = loadServices(serviceType, classLoaders, true).collect(Collectors.toList());
if (services.size() > 0) {
if (services.size() > 1) {
- getLogger().warn("Multiple versions of service {} are available on the classpath: {}", serviceType,
+ logger.warn("Multiple versions of service {} are available on the classpath: {}", serviceType,
services);
}
- return services.get(0);
+ return Optional.of(services.get(0));
}
- return null;
+ return Optional.empty();
}
- static <T> List<T> loadServices(final Class<T> serviceType, final Consumer<Throwable> logger) {
- final List<T> services = new ArrayList<T>();
- for (final ClassLoader loader : LoaderUtil.getClassLoaders()) {
- addServices(serviceType, loader, logger, services);
- }
+ static <T> Stream<T> loadServices(final Class<T> serviceType, ClassLoader[] classLoaders, final boolean verbose) {
+ final Stream<T> services = Arrays.stream(classLoaders)
+ .flatMap(classLoader -> loadClassloaderServices(serviceType, classLoader, verbose));
if (USE_HK2_SERVICE_LOCATOR) {
- addOsgiServices(serviceType, services);
+ try {
+ return Stream.concat(services, loadOsgiServices(serviceType));
+ } catch (Throwable e) {
+ if (verbose) {
+ StatusLogger.getLogger().error("Unable to load OSGI services for service {}", serviceType, e);
+ }
+ }
}
return services;
Review comment:
`.distinct()` rather (a real stream method!)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a change in pull request #804: [LOG4J2-3427] Replace ServiceLoader calls with ServiceLoaderUtil
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on a change in pull request #804:
URL: https://github.com/apache/logging-log4j2/pull/804#discussion_r830290065
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/util/ServiceLoaderUtil.java
##########
@@ -110,28 +135,64 @@ private ServiceLoaderUtil() {
* @param logger An action to perform for each broken service.
* @param services A list to add the services.
*/
- private static <T> void addServices(final Class<T> serviceType, final ClassLoader classLoader,
- final Consumer<Throwable> logger, final Collection<T> services) {
- final Iterator<T> iterator = ServiceLoader.load(serviceType, classLoader).iterator();
- while (iterator.hasNext()) {
- try {
- final T service = iterator.next();
- if (classLoader.equals(service.getClass().getClassLoader())) {
- services.add(service);
- }
- } catch (ServiceConfigurationError e) {
- if (logger != null) {
- logger.accept(e);
- }
- }
- }
+ private static <T> Stream<T> loadClassloaderServices(final Class<T> serviceType, final ClassLoader classLoader,
+ final boolean verbose) {
+ return StreamSupport.stream(new ServiceLoaderSpliterator<T>(serviceType, classLoader, verbose), false);
}
- private static <T> void addOsgiServices(final Class<T> serviceType, final Collection<T> services) {
+ private static <T> Stream<? extends T> loadOsgiServices(final Class<T> serviceType) {
final Iterable<? extends T> iterable = org.glassfish.hk2.osgiresourcelocator.ServiceLoader
.lookupProviderInstances(serviceType);
- if (iterable != null) {
- iterable.forEach(services::add);
+ return iterable != null ? StreamSupport.stream(iterable.spliterator(), false) : Stream.empty();
+ }
+
+ private static class ServiceLoaderSpliterator<S> implements Spliterator<S> {
Review comment:
I'll add `ServiceLoader#stream()` to the `ServiceLoaderUtil` in master, when this version reaches its final form.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] rgoers commented on a change in pull request #804: [LOG4J2-3427] Replace ServiceLoader calls with ServiceLoaderUtil
Posted by GitBox <gi...@apache.org>.
rgoers commented on a change in pull request #804:
URL: https://github.com/apache/logging-log4j2/pull/804#discussion_r830447038
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/util/ServiceLoaderUtil.java
##########
@@ -110,28 +139,64 @@ private ServiceLoaderUtil() {
* @param logger An action to perform for each broken service.
* @param services A list to add the services.
*/
- private static <T> void addServices(final Class<T> serviceType, final ClassLoader classLoader,
- final Consumer<Throwable> logger, final Collection<T> services) {
- final Iterator<T> iterator = ServiceLoader.load(serviceType, classLoader).iterator();
- while (iterator.hasNext()) {
- try {
- final T service = iterator.next();
- if (classLoader.equals(service.getClass().getClassLoader())) {
- services.add(service);
- }
- } catch (ServiceConfigurationError e) {
- if (logger != null) {
- logger.accept(e);
- }
- }
- }
+ private static <T> Stream<T> loadClassloaderServices(final Class<T> serviceType, final ClassLoader classLoader,
+ final boolean verbose) {
+ return StreamSupport.stream(new ServiceLoaderSpliterator<T>(serviceType, classLoader, verbose), false);
}
- private static <T> void addOsgiServices(final Class<T> serviceType, final Collection<T> services) {
+ private static <T> Stream<? extends T> loadOsgiServices(final Class<T> serviceType) {
final Iterable<? extends T> iterable = org.glassfish.hk2.osgiresourcelocator.ServiceLoader
.lookupProviderInstances(serviceType);
- if (iterable != null) {
- iterable.forEach(services::add);
+ return iterable != null ? StreamSupport.stream(iterable.spliterator(), false) : Stream.empty();
+ }
+
+ private static class ServiceLoaderSpliterator<S> implements Spliterator<S> {
+
+ private final Iterator<S> serviceIterator;
+ private final ClassLoader classLoader;
+ private final Logger logger;
+ private final String serviceName;
+
+ public ServiceLoaderSpliterator(final Class<S> serviceType, final ClassLoader classLoader,
+ final boolean verbose) {
+ this.serviceIterator = ServiceLoader.load(serviceType, classLoader).iterator();
Review comment:
This code will not work for applications using the module path. Currently, all calls to ServiceLoader happen in the jar that needs the service. However, according to https://openjdk.java.net/projects/jigsaw/spec/sotms/#services, if an automatic module declares a service it behaves as if it had a corresponding provides clause. Note that log4j-api has a module-info.java, so it will act as an explicitly named module. Currently, it declares uses clauses for the services log4j-api needs. This change will mean it will require a uses clauses for any use of ServiceLoader, which makes no sense and certainly won't work as expected. This code needs to be modified to ensure ServiceLoader is always called from the module/jar that requires the service as is done in master.
I should add that while 2.x does not fully support JPMS it currently should tolerate being used in a JPMS application as all the relevant jars, except for log4j-api, declare their automatic module name. We cannot break that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] rgoers commented on a change in pull request #804: [LOG4J2-3427] Replace ServiceLoader calls with ServiceLoaderUtil
Posted by GitBox <gi...@apache.org>.
rgoers commented on a change in pull request #804:
URL: https://github.com/apache/logging-log4j2/pull/804#discussion_r830447038
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/util/ServiceLoaderUtil.java
##########
@@ -110,28 +139,64 @@ private ServiceLoaderUtil() {
* @param logger An action to perform for each broken service.
* @param services A list to add the services.
*/
- private static <T> void addServices(final Class<T> serviceType, final ClassLoader classLoader,
- final Consumer<Throwable> logger, final Collection<T> services) {
- final Iterator<T> iterator = ServiceLoader.load(serviceType, classLoader).iterator();
- while (iterator.hasNext()) {
- try {
- final T service = iterator.next();
- if (classLoader.equals(service.getClass().getClassLoader())) {
- services.add(service);
- }
- } catch (ServiceConfigurationError e) {
- if (logger != null) {
- logger.accept(e);
- }
- }
- }
+ private static <T> Stream<T> loadClassloaderServices(final Class<T> serviceType, final ClassLoader classLoader,
+ final boolean verbose) {
+ return StreamSupport.stream(new ServiceLoaderSpliterator<T>(serviceType, classLoader, verbose), false);
}
- private static <T> void addOsgiServices(final Class<T> serviceType, final Collection<T> services) {
+ private static <T> Stream<? extends T> loadOsgiServices(final Class<T> serviceType) {
final Iterable<? extends T> iterable = org.glassfish.hk2.osgiresourcelocator.ServiceLoader
.lookupProviderInstances(serviceType);
- if (iterable != null) {
- iterable.forEach(services::add);
+ return iterable != null ? StreamSupport.stream(iterable.spliterator(), false) : Stream.empty();
+ }
+
+ private static class ServiceLoaderSpliterator<S> implements Spliterator<S> {
+
+ private final Iterator<S> serviceIterator;
+ private final ClassLoader classLoader;
+ private final Logger logger;
+ private final String serviceName;
+
+ public ServiceLoaderSpliterator(final Class<S> serviceType, final ClassLoader classLoader,
+ final boolean verbose) {
+ this.serviceIterator = ServiceLoader.load(serviceType, classLoader).iterator();
Review comment:
This code will not work for applications using the module path. Currently, all calls to ServiceLoader happen in the jar that needs the service. However, according to https://openjdk.java.net/projects/jigsaw/spec/sotms/#services, if an automatic module declares a service it behaves as if it had a corresponding provides clause. Note that log4j-api has a module-info.java, so it will act as an explicitly named module. Currently, it declares uses clauses for the services log4j-api needs. This change will mean it will require a uses clauses for any use of ServiceLoader, which makes no sense and certainly won't work as expected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory commented on a change in pull request #804: [LOG4J2-3427] Replace ServiceLoader calls with ServiceLoaderUtil
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #804:
URL: https://github.com/apache/logging-log4j2/pull/804#discussion_r830027870
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/util/LoaderUtil.java
##########
@@ -120,6 +120,22 @@ public ClassLoader run() {
return classLoaders.toArray(EMPTY_CLASS_LOADER_ARRAY);
}
+ /**
+ * Gets the the give classloader and its ancestors.
Review comment:
Give -> given?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on pull request #804: [LOG4J2-3427] Replace ServiceLoader calls with ServiceLoaderUtil
Posted by GitBox <gi...@apache.org>.
jvz commented on pull request #804:
URL: https://github.com/apache/logging-log4j2/pull/804#issuecomment-1075350519
> * OSGI support was also removed from `LoaderServiceUtil`, because it uses an artifact (`osgi-resource-locator`) that seems to be abandoned (it is still supported by JAXB API and similar, but I found no other references). Log4j registers (and probably should register) its services as native OSGI services.
This is something I still struggle with. In master, I've refactored the OSGi service registration to mostly use ServiceLoader invoked from a bundle activator class for each module. That then calls `ServiceRegistry` (a new class for holding known services) which can add and remove services.
My problem with either approach is that it requires OSGi users to do something additional to support their own custom plugins that we can't exactly automate for them. It's already annoying enough that users currently need to manually update their module-info.java to export the service class; having to do even more manual work for OSGi means that most plugins out there won't bother supporting OSGi which is a bummer.
> * the number of `ServiceLoader#load` calls has been reduced to at most 2: one for the caller's classloader and possibly one for the thread context classloader,
This is a great change!
> * most `ServiceLoaderUtil#loadServices` call sites store the result in a static variable. To prevent memory leaks those calls don't use the TCCL.
This is an interesting limitation. I think we'll need to examine master to see any similar issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #804: [LOG4J2-3427] Replace ServiceLoader calls with ServiceLoaderUtil
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #804:
URL: https://github.com/apache/logging-log4j2/pull/804#discussion_r830104483
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/util/ProviderUtil.java
##########
@@ -63,13 +63,10 @@
private static volatile ProviderUtil instance;
private ProviderUtil() {
- for (final ClassLoader classLoader : LoaderUtil.getClassLoaders()) {
- try {
- loadProviders(classLoader);
- } catch (final Throwable ex) {
- LOGGER.debug("Unable to retrieve provider from ClassLoader {}", classLoader, ex);
- }
- }
+ ServiceLoaderUtil.loadServices(Provider.class)
+ .filter(provider -> validVersion(provider.getVersions()) && !PROVIDERS.contains(provider))
+ .forEach(PROVIDERS::add);
Review comment:
No need to check contains, add will return false and no-op if the element already exists
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/util/ServiceLoaderUtil.java
##########
@@ -47,10 +57,20 @@ private ServiceLoaderUtil() {
*
* @param <T> The service type.
* @param serviceType The class of the service.
- * @return List of available services.
+ * @return Stream of available services.
*/
- public static <T> List<T> loadServices(final Class<T> serviceType) {
- return loadServices(serviceType, null);
+ public static <T> Stream<T> loadServices(final Class<T> serviceType) {
Review comment:
This is an api/abi break moving from `List<T>` to `Stream<T>`. The Stream allows us to avoid unused service-loader lookups, but should be added as a new method where we deprecate the original.
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/util/ServiceLoaderUtil.java
##########
@@ -110,28 +135,64 @@ private ServiceLoaderUtil() {
* @param logger An action to perform for each broken service.
* @param services A list to add the services.
*/
- private static <T> void addServices(final Class<T> serviceType, final ClassLoader classLoader,
- final Consumer<Throwable> logger, final Collection<T> services) {
- final Iterator<T> iterator = ServiceLoader.load(serviceType, classLoader).iterator();
- while (iterator.hasNext()) {
- try {
- final T service = iterator.next();
- if (classLoader.equals(service.getClass().getClassLoader())) {
- services.add(service);
- }
- } catch (ServiceConfigurationError e) {
- if (logger != null) {
- logger.accept(e);
- }
- }
- }
+ private static <T> Stream<T> loadClassloaderServices(final Class<T> serviceType, final ClassLoader classLoader,
+ final boolean verbose) {
+ return StreamSupport.stream(new ServiceLoaderSpliterator<T>(serviceType, classLoader, verbose), false);
}
- private static <T> void addOsgiServices(final Class<T> serviceType, final Collection<T> services) {
+ private static <T> Stream<? extends T> loadOsgiServices(final Class<T> serviceType) {
final Iterable<? extends T> iterable = org.glassfish.hk2.osgiresourcelocator.ServiceLoader
.lookupProviderInstances(serviceType);
- if (iterable != null) {
- iterable.forEach(services::add);
+ return iterable != null ? StreamSupport.stream(iterable.spliterator(), false) : Stream.empty();
+ }
+
+ private static class ServiceLoaderSpliterator<S> implements Spliterator<S> {
Review comment:
jdk9+ introduced similar functionality with https://docs.oracle.com/javase/9/docs/api/java/util/ServiceLoader.html#stream--
The upside is that `Provider.get()` (instantiating a default instance) can fail (classpath/linker issues) without breaking the entire call.
Probably not worth adding jdk11 mr-jar code to handle, but we should add a comment describing that the feature should be used when jdk11 is the minimum supported version
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/util/ServiceLoaderUtil.java
##########
@@ -60,43 +80,48 @@ private ServiceLoaderUtil() {
* @param serviceType The class of the service.
* @return A service of the given type or {@code null} if none is available.
*/
- public static <T> T getService(final Class<T> serviceType) {
+ public static <T> Optional<T> getService(final Class<T> serviceType) {
+ return getService(serviceType, LoaderUtil.getClassLoaders());
+ }
+
+ public static <T> Optional<T> getService(final Class<T> serviceType, ClassLoader classLoader) {
+ return getService(serviceType, LoaderUtil.getClassLoaders(classLoader));
+ }
+
+ private static <T> Optional<T> getService(final Class<T> serviceType, final ClassLoader[] classLoaders) {
+ final Logger logger = StatusLogger.getLogger();
// 1. Check for a system property with the FQCN of serviceType
- String serviceClassName = PropertiesUtil.getProperties().getStringProperty(serviceType.getName());
- if (Strings.isNotEmpty(serviceClassName)) {
- try {
- @SuppressWarnings("unchecked")
- final Class<T> serviceClass = (Class<T>) Class.forName(serviceClassName);
- if (serviceType.isAssignableFrom(serviceClass)) {
- return LoaderUtil.newInstanceOf(serviceClass);
- }
- getLogger().error("Unable to load service {}: it is not of the required type {}", serviceClass,
- serviceType);
- } catch (ClassNotFoundException | InstantiationException | IllegalAccessException
- | InvocationTargetException e) {
- getLogger().error("Unable to load service class {}", serviceClassName, e);
+ try {
+ final T service = LoaderUtil.newCheckedInstanceOfProperty(serviceType.getName(), serviceType);
+ if (service != null) {
+ return Optional.of(service);
}
+ } catch (ReflectiveOperationException | ClassCastException e) {
+ logger.error("Unable to load service class {}", serviceType.getName(), e);
}
// 2. Use the ServiceLoader:
- final List<T> services = loadServices(serviceType,
- t -> getLogger().info("Unable to load service class for service {}", serviceType.getName(), t));
+ final List<T> services = loadServices(serviceType, classLoaders, true).collect(Collectors.toList());
if (services.size() > 0) {
if (services.size() > 1) {
- getLogger().warn("Multiple versions of service {} are available on the classpath: {}", serviceType,
+ logger.warn("Multiple versions of service {} are available on the classpath: {}", serviceType,
services);
}
- return services.get(0);
+ return Optional.of(services.get(0));
}
- return null;
+ return Optional.empty();
}
- static <T> List<T> loadServices(final Class<T> serviceType, final Consumer<Throwable> logger) {
- final List<T> services = new ArrayList<T>();
- for (final ClassLoader loader : LoaderUtil.getClassLoaders()) {
- addServices(serviceType, loader, logger, services);
- }
+ static <T> Stream<T> loadServices(final Class<T> serviceType, ClassLoader[] classLoaders, final boolean verbose) {
+ final Stream<T> services = Arrays.stream(classLoaders)
+ .flatMap(classLoader -> loadClassloaderServices(serviceType, classLoader, verbose));
if (USE_HK2_SERVICE_LOCATOR) {
- addOsgiServices(serviceType, services);
+ try {
+ return Stream.concat(services, loadOsgiServices(serviceType));
+ } catch (Throwable e) {
+ if (verbose) {
+ StatusLogger.getLogger().error("Unable to load OSGI services for service {}", serviceType, e);
+ }
+ }
}
return services;
Review comment:
I wonder if we want something like `return services.unique()` here to avoid loading the same instance from multiple related classloaders?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on pull request #804: [LOG4J2-3427] Replace ServiceLoader calls with ServiceLoaderUtil
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on pull request #804:
URL: https://github.com/apache/logging-log4j2/pull/804#issuecomment-1074844988
After several fixes, this should work now. A summary of changes:
- I removed `LoaderServiceUtil#getService` that I introduced in a previous commit, since it's logic is not really needed,
- OSGI support was also removed from `LoaderServiceUtil`, because it uses an artifact (`osgi-resource-locator`) that seems to be abandoned (it is still supported by JAXB API and similar, but I found no other references). Log4j registers (and probably should register) its services as native OSGI services.
- all `ServiceLoader#load` call sites have been refactored to use `ServiceLoaderUtil`,
- the number of `ServiceLoader#load` calls has been reduced to at most 2: one for the caller's classloader and possibly one for the thread context classloader,
- most `ServiceLoaderUtil#loadServices` call sites store the result in a static variable. To prevent memory leaks those calls don't use the TCCL.
- a Java 9 version of `ServiceLoaderUtil` was introduced to take advantage of the new `stream()` method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a change in pull request #804: [LOG4J2-3427] Replace ServiceLoader calls with ServiceLoaderUtil
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on a change in pull request #804:
URL: https://github.com/apache/logging-log4j2/pull/804#discussion_r831575308
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/util/ServiceLoaderUtil.java
##########
@@ -110,28 +78,73 @@ private ServiceLoaderUtil() {
* @param logger An action to perform for each broken service.
* @param services A list to add the services.
*/
- private static <T> void addServices(final Class<T> serviceType, final ClassLoader classLoader,
- final Consumer<Throwable> logger, final Collection<T> services) {
- final Iterator<T> iterator = ServiceLoader.load(serviceType, classLoader).iterator();
- while (iterator.hasNext()) {
- try {
- final T service = iterator.next();
- if (classLoader.equals(service.getClass().getClassLoader())) {
- services.add(service);
- }
- } catch (ServiceConfigurationError e) {
- if (logger != null) {
- logger.accept(e);
- }
+ static <T> Stream<T> loadClassloaderServices(final Class<T> serviceType, final Lookup lookup,
+ final ClassLoader classLoader, final boolean verbose) {
+ return StreamSupport.stream(new ServiceLoaderSpliterator<T>(serviceType, lookup, classLoader, verbose), false);
+ }
+
+ static <T> Iterable<T> callServiceLoader(Lookup lookup, Class<T> serviceType, ClassLoader classLoader,
+ boolean verbose) {
+ try {
+ final MethodHandle handle = lookup.findStatic(ServiceLoader.class, "load", LOAD_CLASS_CLASSLOADER);
+ final ServiceLoader<T> serviceLoader = (ServiceLoader<T>) handle.invokeExact(serviceType, classLoader);
+ return serviceLoader;
+ } catch (Throwable e) {
+ if (verbose) {
+ StatusLogger.getLogger().error("Unable to load services for service {}", serviceType, e);
}
}
+ return Collections.emptyList();
+
}
- private static <T> void addOsgiServices(final Class<T> serviceType, final Collection<T> services) {
- final Iterable<? extends T> iterable = org.glassfish.hk2.osgiresourcelocator.ServiceLoader
- .lookupProviderInstances(serviceType);
- if (iterable != null) {
- iterable.forEach(services::add);
Review comment:
This is not the right way to find OSGI services in Log4j: Log4j artifacts register OSGI services properly, so no need for the (probably) unsupported `osgi-resource-locator` or other tricks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a change in pull request #804: [LOG4J2-3427] Replace ServiceLoader calls with ServiceLoaderUtil
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on a change in pull request #804:
URL: https://github.com/apache/logging-log4j2/pull/804#discussion_r830102145
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/util/LoaderUtil.java
##########
@@ -120,6 +120,22 @@ public ClassLoader run() {
return classLoaders.toArray(EMPTY_CLASS_LOADER_ARRAY);
}
+ /**
+ * Gets the the give classloader and its ancestors.
Review comment:
Yes, that didn't come right.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] rgoers commented on a change in pull request #804: [LOG4J2-3427] Replace ServiceLoader calls with ServiceLoaderUtil
Posted by GitBox <gi...@apache.org>.
rgoers commented on a change in pull request #804:
URL: https://github.com/apache/logging-log4j2/pull/804#discussion_r830502914
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/util/ServiceLoaderUtil.java
##########
@@ -110,28 +139,64 @@ private ServiceLoaderUtil() {
* @param logger An action to perform for each broken service.
* @param services A list to add the services.
*/
- private static <T> void addServices(final Class<T> serviceType, final ClassLoader classLoader,
- final Consumer<Throwable> logger, final Collection<T> services) {
- final Iterator<T> iterator = ServiceLoader.load(serviceType, classLoader).iterator();
- while (iterator.hasNext()) {
- try {
- final T service = iterator.next();
- if (classLoader.equals(service.getClass().getClassLoader())) {
- services.add(service);
- }
- } catch (ServiceConfigurationError e) {
- if (logger != null) {
- logger.accept(e);
- }
- }
- }
+ private static <T> Stream<T> loadClassloaderServices(final Class<T> serviceType, final ClassLoader classLoader,
+ final boolean verbose) {
+ return StreamSupport.stream(new ServiceLoaderSpliterator<T>(serviceType, classLoader, verbose), false);
}
- private static <T> void addOsgiServices(final Class<T> serviceType, final Collection<T> services) {
+ private static <T> Stream<? extends T> loadOsgiServices(final Class<T> serviceType) {
final Iterable<? extends T> iterable = org.glassfish.hk2.osgiresourcelocator.ServiceLoader
.lookupProviderInstances(serviceType);
- if (iterable != null) {
- iterable.forEach(services::add);
+ return iterable != null ? StreamSupport.stream(iterable.spliterator(), false) : Stream.empty();
+ }
+
+ private static class ServiceLoaderSpliterator<S> implements Spliterator<S> {
+
+ private final Iterator<S> serviceIterator;
+ private final ClassLoader classLoader;
+ private final Logger logger;
+ private final String serviceName;
+
+ public ServiceLoaderSpliterator(final Class<S> serviceType, final ClassLoader classLoader,
+ final boolean verbose) {
+ this.serviceIterator = ServiceLoader.load(serviceType, classLoader).iterator();
Review comment:
@ppkarwasz No matter how you slice it, so long as the call originates from the log4j-api module it won't work in JPMS. I would suggest that you use `ServiceLoader.load(Class<S> service, ClassLoader classLoader)` from the lambda. You will end up calling it once for each ClassLoader you have. In master I don't have to do this since anything running on the module path is going to have a ModuleLayer.
Another option, of course, is to create a Java 9 specific version of this that supports using ModuleLayer. Then the code you currently have would work. That is probably the route I would go.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] rgoers commented on a change in pull request #804: [LOG4J2-3427] Replace ServiceLoader calls with ServiceLoaderUtil
Posted by GitBox <gi...@apache.org>.
rgoers commented on a change in pull request #804:
URL: https://github.com/apache/logging-log4j2/pull/804#discussion_r830447038
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/util/ServiceLoaderUtil.java
##########
@@ -110,28 +139,64 @@ private ServiceLoaderUtil() {
* @param logger An action to perform for each broken service.
* @param services A list to add the services.
*/
- private static <T> void addServices(final Class<T> serviceType, final ClassLoader classLoader,
- final Consumer<Throwable> logger, final Collection<T> services) {
- final Iterator<T> iterator = ServiceLoader.load(serviceType, classLoader).iterator();
- while (iterator.hasNext()) {
- try {
- final T service = iterator.next();
- if (classLoader.equals(service.getClass().getClassLoader())) {
- services.add(service);
- }
- } catch (ServiceConfigurationError e) {
- if (logger != null) {
- logger.accept(e);
- }
- }
- }
+ private static <T> Stream<T> loadClassloaderServices(final Class<T> serviceType, final ClassLoader classLoader,
+ final boolean verbose) {
+ return StreamSupport.stream(new ServiceLoaderSpliterator<T>(serviceType, classLoader, verbose), false);
}
- private static <T> void addOsgiServices(final Class<T> serviceType, final Collection<T> services) {
+ private static <T> Stream<? extends T> loadOsgiServices(final Class<T> serviceType) {
final Iterable<? extends T> iterable = org.glassfish.hk2.osgiresourcelocator.ServiceLoader
.lookupProviderInstances(serviceType);
- if (iterable != null) {
- iterable.forEach(services::add);
+ return iterable != null ? StreamSupport.stream(iterable.spliterator(), false) : Stream.empty();
+ }
+
+ private static class ServiceLoaderSpliterator<S> implements Spliterator<S> {
+
+ private final Iterator<S> serviceIterator;
+ private final ClassLoader classLoader;
+ private final Logger logger;
+ private final String serviceName;
+
+ public ServiceLoaderSpliterator(final Class<S> serviceType, final ClassLoader classLoader,
+ final boolean verbose) {
+ this.serviceIterator = ServiceLoader.load(serviceType, classLoader).iterator();
Review comment:
This code will not work for applications using the module path. Currently, all calls to ServiceLoader happen in the jar that needs the service. However, according to https://openjdk.java.net/projects/jigsaw/spec/sotms/#services, if an automatic module declares a service it behaves as if it had a corresponding provides clause. Note that log4j-api has a module-info.java, so it will act as an explicitly named module. Currently, it declares uses clauses for the services log4j-api needs. This change will mean it will require a uses clauses for any use of ServiceLoader, which makes no sense and certainly won't work as expected. This code needs to be modified to ensure ServiceLoader is always called from the module/jar that requires the service as is done in master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a change in pull request #804: [LOG4J2-3427] Replace ServiceLoader calls with ServiceLoaderUtil
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on a change in pull request #804:
URL: https://github.com/apache/logging-log4j2/pull/804#discussion_r830288798
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/util/ServiceLoaderUtil.java
##########
@@ -60,43 +80,48 @@ private ServiceLoaderUtil() {
* @param serviceType The class of the service.
* @return A service of the given type or {@code null} if none is available.
*/
- public static <T> T getService(final Class<T> serviceType) {
+ public static <T> Optional<T> getService(final Class<T> serviceType) {
+ return getService(serviceType, LoaderUtil.getClassLoaders());
+ }
+
+ public static <T> Optional<T> getService(final Class<T> serviceType, ClassLoader classLoader) {
+ return getService(serviceType, LoaderUtil.getClassLoaders(classLoader));
+ }
+
+ private static <T> Optional<T> getService(final Class<T> serviceType, final ClassLoader[] classLoaders) {
+ final Logger logger = StatusLogger.getLogger();
// 1. Check for a system property with the FQCN of serviceType
- String serviceClassName = PropertiesUtil.getProperties().getStringProperty(serviceType.getName());
- if (Strings.isNotEmpty(serviceClassName)) {
- try {
- @SuppressWarnings("unchecked")
- final Class<T> serviceClass = (Class<T>) Class.forName(serviceClassName);
- if (serviceType.isAssignableFrom(serviceClass)) {
- return LoaderUtil.newInstanceOf(serviceClass);
- }
- getLogger().error("Unable to load service {}: it is not of the required type {}", serviceClass,
- serviceType);
- } catch (ClassNotFoundException | InstantiationException | IllegalAccessException
- | InvocationTargetException e) {
- getLogger().error("Unable to load service class {}", serviceClassName, e);
+ try {
+ final T service = LoaderUtil.newCheckedInstanceOfProperty(serviceType.getName(), serviceType);
+ if (service != null) {
+ return Optional.of(service);
}
+ } catch (ReflectiveOperationException | ClassCastException e) {
+ logger.error("Unable to load service class {}", serviceType.getName(), e);
}
// 2. Use the ServiceLoader:
- final List<T> services = loadServices(serviceType,
- t -> getLogger().info("Unable to load service class for service {}", serviceType.getName(), t));
+ final List<T> services = loadServices(serviceType, classLoaders, true).collect(Collectors.toList());
if (services.size() > 0) {
if (services.size() > 1) {
- getLogger().warn("Multiple versions of service {} are available on the classpath: {}", serviceType,
+ logger.warn("Multiple versions of service {} are available on the classpath: {}", serviceType,
services);
}
- return services.get(0);
+ return Optional.of(services.get(0));
}
- return null;
+ return Optional.empty();
}
- static <T> List<T> loadServices(final Class<T> serviceType, final Consumer<Throwable> logger) {
- final List<T> services = new ArrayList<T>();
- for (final ClassLoader loader : LoaderUtil.getClassLoaders()) {
- addServices(serviceType, loader, logger, services);
- }
+ static <T> Stream<T> loadServices(final Class<T> serviceType, ClassLoader[] classLoaders, final boolean verbose) {
+ final Stream<T> services = Arrays.stream(classLoaders)
+ .flatMap(classLoader -> loadClassloaderServices(serviceType, classLoader, verbose));
if (USE_HK2_SERVICE_LOCATOR) {
- addOsgiServices(serviceType, services);
+ try {
+ return Stream.concat(services, loadOsgiServices(serviceType));
+ } catch (Throwable e) {
+ if (verbose) {
+ StatusLogger.getLogger().error("Unable to load OSGI services for service {}", serviceType, e);
+ }
+ }
}
return services;
Review comment:
I'll go for a single instance per class for now. The `Provider` service overrides `equals/hashCode`, but the other service does not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a change in pull request #804: [LOG4J2-3427] Replace ServiceLoader calls with ServiceLoaderUtil
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on a change in pull request #804:
URL: https://github.com/apache/logging-log4j2/pull/804#discussion_r831826513
##########
File path: log4j-api/src/test/java/org/apache/logging/log4j/util/ServiceLoaderUtilTest.java
##########
@@ -86,42 +85,4 @@ public void testBrokenServiceFile() {
assertEquals(warnings + 2, counter.get());
}
- @Test
- public void testMultipleServicesPresentWarning() {
- final int warnings = counter.get();
Review comment:
The methods used in these tests were unused and removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] rgoers commented on a change in pull request #804: [LOG4J2-3427] Replace ServiceLoader calls with ServiceLoaderUtil
Posted by GitBox <gi...@apache.org>.
rgoers commented on a change in pull request #804:
URL: https://github.com/apache/logging-log4j2/pull/804#discussion_r830502914
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/util/ServiceLoaderUtil.java
##########
@@ -110,28 +139,64 @@ private ServiceLoaderUtil() {
* @param logger An action to perform for each broken service.
* @param services A list to add the services.
*/
- private static <T> void addServices(final Class<T> serviceType, final ClassLoader classLoader,
- final Consumer<Throwable> logger, final Collection<T> services) {
- final Iterator<T> iterator = ServiceLoader.load(serviceType, classLoader).iterator();
- while (iterator.hasNext()) {
- try {
- final T service = iterator.next();
- if (classLoader.equals(service.getClass().getClassLoader())) {
- services.add(service);
- }
- } catch (ServiceConfigurationError e) {
- if (logger != null) {
- logger.accept(e);
- }
- }
- }
+ private static <T> Stream<T> loadClassloaderServices(final Class<T> serviceType, final ClassLoader classLoader,
+ final boolean verbose) {
+ return StreamSupport.stream(new ServiceLoaderSpliterator<T>(serviceType, classLoader, verbose), false);
}
- private static <T> void addOsgiServices(final Class<T> serviceType, final Collection<T> services) {
+ private static <T> Stream<? extends T> loadOsgiServices(final Class<T> serviceType) {
final Iterable<? extends T> iterable = org.glassfish.hk2.osgiresourcelocator.ServiceLoader
.lookupProviderInstances(serviceType);
- if (iterable != null) {
- iterable.forEach(services::add);
+ return iterable != null ? StreamSupport.stream(iterable.spliterator(), false) : Stream.empty();
+ }
+
+ private static class ServiceLoaderSpliterator<S> implements Spliterator<S> {
+
+ private final Iterator<S> serviceIterator;
+ private final ClassLoader classLoader;
+ private final Logger logger;
+ private final String serviceName;
+
+ public ServiceLoaderSpliterator(final Class<S> serviceType, final ClassLoader classLoader,
+ final boolean verbose) {
+ this.serviceIterator = ServiceLoader.load(serviceType, classLoader).iterator();
Review comment:
@ppkarwasz No matter how you slice it, so long as the call originates from the log4j-api module it won't work in JPMS. I would suggest that you use ServiceLoader.load(Class<S> service, ClassLoader classLoader) from the lambda. You will end up calling it once for each ClassLoader you have. In master I don't have to do this since anything running on the module path is going to have a ModuleLayer.
Another option, of course, is to create a Java 9 specific version of this that supports using ModuleLayer. Then the code you currently have would work. That is probably the route I would go.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org