Continuing after ServiceLoader ServiceConfigurationError. #4340
Comments
|
I think this is screaming out for a ServiceLoaderUtil method. |
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
this prevents errors where jetty-util must declare it uses the provider class in module.info Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
|
Maybe we just need a coding pattern? For example the AnnotationConfiguration loads ServletContainerInitializers from the ServiceLoader like this: ServiceLoader<ServletContainerInitializer> loader = ServiceLoader.load(ServletContainerInitializer.class);
Iterator<ServletContainerInitializer> iter = loader.iterator();
while (iter.hasNext())
{
ServletContainerInitializer sci;
try
{
sci = iter.next();
}
catch (Error e)
{
// Probably a SCI discovered on the system classpath that is hidden by the context classloader
LOG.info("Error: {} for {}", e.getMessage(), context);
LOG.debug(e);
continue;
}
//do stuff
} |
|
@janbartel the problem with that pattern is that it doesn't catch the exceptions that are thrown from |
|
@gregw ah yes, I remember the problem now. |
- increase MAX_ERRORS to 1000 - introduce a loadFirst method Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
|
I am not sure this is a good idea. The problem is that With JPMS using We have had similar issues with I think this should be carefully analyzed before moving it to |
|
@sbordet this is why the call to ServiceLoader.load is not done within the utility method, but in the caller scope. The utility method is essentially only wrapping the iterator that is returned, which should be allowed by any caller-sensitive code. |
|
@gregw the iterator is allocated in the call site, but it's a lazy operator. It's when you call However, I checked the JDK code and the caller check is only performed at creation time ( |
|
@sbordet the first iteration of this had the load in the util and it did not pass tests. This passes tests. |
|
@gregw returning a |
|
I threw together a simple experiment project with ServiceLoader as I was curious as to how the stream API worked. Project can be found at https://github.com/joakime/serviceloader-experiments The stream API usage can be found at https://github.com/joakime/serviceloader-experiments/blob/master/drinker/src/main/java/org/eclipse/jetty/demo/Drinker.java Looking like this ... ServiceLoader.load(BarService.class).stream().forEach((barProvider) ->
{
try
{
BarService service = barProvider.get();
if (service == null)
{
System.out.println("No service found.");
}
System.out.printf("Service (%s) is type %s%n", service.getClass().getName(), service.getType());
}
catch (ServiceConfigurationError error)
{
System.out.printf("Service failed to load: %s%n", error.getMessage());
error.getCause().printStackTrace(System.out);
}
});The entire error handling is contained within the call to |
|
This works too, to skip errors / nulls during iteration. Optional<BarService> service = ServiceLoader.load(BarService.class).stream()
.map((provider) ->
{
// attempt to load service
try
{
// will either return a service, throw an error, or return null
return provider.get();
}
catch (ServiceConfigurationError error)
{
LOG.warn("BarService failed to load", error);
}
return null;
})
.filter((bar) -> bar != null) // filter out empty / error services
.findFirst();
BarService barService = service.orElseThrow(() -> new IllegalStateException("Unable to find a BarService"));
LOG.info("Using Service ({}) is type [{}]", barService.getClass().getName(), barService.getType()); |
|
@joakime I tried using a standard iterator for this test, none of the examples throw in the call to
When we reach the call to |
|
@lachlan-roberts I'm thinking that the It's when trying to instantiate the service that an exception may happen, because it's the wrong class version (e.g. ALPN), or some dependency is not available, etc. I think we should just follow the idiom of using the |
|
Would a utility method that mapped an Provider to an Optional help with a nice usage of streams, without the need for a verbose try{}catch{}? |
|
I went ahead and added more testcases to https://github.com/joakime/serviceloader-experiments that @gregw asked about. In that example project we have the following jar files, which all have
Using the old school iterator method ... ServiceLoader serviceLoader = ServiceLoader.load(BarService.class);
Iterator<BarService> serviceIterator = serviceLoader.iterator();
while (serviceIterator.hasNext())
{
try
{
BarService barService = serviceIterator.next();
if (barService == null)
{
LOG.warn("BarService returned null");
}
else
{
LOG.info("Using Service ({}) is type [{}]", barService.getClass().getName(), barService.getType());
}
}
catch (ServiceConfigurationError error)
{
LOG.warn("BarService failed to load", error);
}
}We see ... Using the newer ServiceLoader.load(BarService.class).stream().forEach((barProvider) ->
{
try
{
BarService barService = barProvider.get();
if (barService == null)
{
LOG.warn("BarService returned null");
}
else
{
LOG.info("Using Service ({}) is type [{}]", barService.getClass().getName(), barService.getType());
}
}
catch (ServiceConfigurationError error)
{
LOG.warn("BarService failed to load", error);
}
});We see ... Which is interesting, as some of the intentionally bad services don't even appear to be passed to the
|
|
The extra |
|
Also, even with the old school |
|
@joakime I'm sure we have had problems with exceptions thrown from hasNext, that is what started this process in the first place. Perhaps it is only on some JVMs |
|
The Example: The loading of And websocket would log at debug/info for Extension load issues, but warn/error (possibly even fatal exception reported) for default Configurator issues. |
|
There's only 1 reported issue with ServiceLoader and hasNext(). The If this is a issue for us, using Optional isn't going to fix things, or let us limp along, we have to adjust our usage of the same ServiceLoader (ones for the same service interface) to not access the ServiceLoader from multiple threads. |
|
Open question, will ServiceLoader on OSGi with SpiFly still work if we create a utility class? |
|
A utility mapping method can make the stream code a little nicer: public static <T> Optional<T> provide(ServiceLoader.Provider<T> provider)
{
try
{
return Optional.of(provider.get());
}
catch(Throwable e)
{
LOG.warn(e);
return Optional.empty();
}
}
public void test()
{
List<Configuration> configurations = ServiceLoader.load(Configuration.class).stream()
.map(Configurations::provide)
.filter(Optional::isPresent)
.map(Optional::get)
.collect(Collectors.toList());
}But the map->filter->map->collect is still a bit verbose. Perhaps somebody with some better stream-foo than me can come up with a better way to do this, so we can collapse down the try catch but not have too many steps? |
|
This is better: public static <T> Stream<T> provide(ServiceLoader.Provider<T> provider)
{
try
{
return Stream.of(provider.get());
}
catch(Throwable e)
{
LOG.warn(e);
return Stream.empty();
}
}
public void test()
{
List<Configuration> configurations = ServiceLoader.load(Configuration.class).stream()
.flatMap(Configurations::provide)
.collect(Collectors.toList());
} |
|
No need for public static <T> T supplies(Supplier<T> supplier)
{
try
{
return supplier.get();
}
catch (Throwable e)
{
LOG.warn("Unable to get Service", e);
}
return null;
}
public void test()
{
List<Configuration> configurations = ServiceLoader.load(Configuration.class).stream()
.map(Configuration::supplies)
.filter(Objects::nonNull)
.collect(Collectors.toList());
}
public void testClassLoaderSpecific()
{
ClassLoader cl = getClassLoader();
List<Configuration> configurations = ServiceLoader.load(Configuration.class, cl).stream()
.map(Configuration::supplies)
.filter(Objects::nonNull)
.collect(Collectors.toList());
} |
|
@joakime, Greg's version is better, no |
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Issue #4340 - Continue after ServiceLoader ServiceConfigurationError
|
fixed with PR #4602 |
jetty-9.4 and beyond
When using the ServiceLoader, we would like to be able to catch Exceptions from each call to ServiceLoader.iterator().next() or ServiceLoader.iterator().hasNext() and continue on while there are other services to load.
Currently, most code does something like the following, which will stop iterating when the first exception is thrown:
However, according to the ServiceLoader javadoc, it is possible to continue iterating after an exception has occurred: https://docs.oracle.com/javase/10/docs/api/java/util/ServiceLoader.html#iterator(). It would be desirable to log those Services that cannot be loaded, and continue on with those that are. The problem is how to code that. One ugly attempt could be:
The text was updated successfully, but these errors were encountered: