unitsofmeasurement / indriya

JSR 385 - Reference Implementation
Other
115 stars 40 forks source link

Error loading NumberSystem ServiceProvider with multiple ClassLoaders #383

Closed etatara closed 1 year ago

etatara commented 1 year ago

The Calculus class makes calls to ServiceLoader.load(NumberSystem.class) that will fail when using multiple ClassLoaders and Calculus.class is loaded using a ClassLoader other than the default ClassLoader. I suggest providing the additional ClassLoader argument that will use whatever ClassLoader actually loaded the Calculus class:

ServiceLoader<NumberSystem> loader = ServiceLoader.load(NumberSystem.class, Calculus.class.getClassLoader());
keilw commented 1 year ago

Thanks, is there a scenario to replicate that, e.g. in an app server or other cases with multiple class loaders?

blackdrag commented 1 year ago

ServiceLoader.load uses the context class loader. The context class loader usually is a child or identical to the class loader used as default.

In Open Webstart the webstart application is loaded by its own class loader, which is not the default. The context class loader is set, but in the case of the application, that did bring me here, there are a lot of libraries involved. Which means some library somewhere is spawning a thread, which then tries to load (in the end) DefaultNumberSystem and fails to do so.

In our case if you would not try only the context class loader, but also the loader of NumberSystem (NumberSystem.class.getClassLoader()) the issue would be resolved.

evanjs commented 1 year ago

Hey, so we have been stuck witth this problem for a while now, but have not had the time to make a PR. This completely breaks things like Swagger, so we had to patch locally and repeat with several sister libraries.

It's been a while since I've dealt with the issue, but I believe it only applies to the Gradle plugin (swagger-gradle), not the Maven plugin.

As mentioned in the original post, the solution appears to be fairly simple, potential side effects notwithstanding.

With this patch applied, Swagger/OpenAPI 3 is able to generate documentation and properly process annotations with Unit-based types.

ClassLoader Patch ```diff diff --git a/src/main/java/tech/units/indriya/function/Calculus.java b/src/main/java/tech/units/indriya/function/Calculus.java index 89bd23c3..5688edf4 100644 --- a/src/main/java/tech/units/indriya/function/Calculus.java +++ b/src/main/java/tech/units/indriya/function/Calculus.java @@ -75,10 +75,8 @@ public final class Calculus { */ public static List getAvailableNumberSystems() { List systems = new ArrayList<>(); - ServiceLoader loader = ServiceLoader.load(NumberSystem.class); - loader.forEach(NumberSystem -> { - systems.add(NumberSystem); - }); + ServiceLoader loader = ServiceLoader.load(NumberSystem.class, NumberSystem.class.getClassLoader()); + loader.forEach(systems::add); return systems; } @@ -107,7 +105,7 @@ public final class Calculus { * Returns the given {@link NumberSystem} used for Number arithmetic by (class) name. */ public static NumberSystem getNumberSystem(String name) { - final ServiceLoader loader = ServiceLoader.load(NumberSystem.class); + final ServiceLoader loader = ServiceLoader.load(NumberSystem.class, NumberSystem.class.getClassLoader()); final Iterator it = loader.iterator(); while (it.hasNext()) { NumberSystem provider = it.next(); ```

Thanks, is there a scenario to replicate that, e.g. in an app server or other cases with multiple class loaders?

@keilw We are using this library in a private project, but if I understand correctly, you should be able to reproduce this issue if you try to generate documentation for a basic Gradle-based Swagger/OpenAPI 3 project.

Something like specifying a Unit-based type as an @ApiOperation's response like SampleData.class is here should be enough to prevent Swagger's resolve task from running, which then renders Swagger unusable.


Related issue for swagger-gradle-plugin: https://github.com/swagger-api/swagger-core/issues/3726


cc @addict3d

keilw commented 1 year ago

This is applied, thanks @evanjs for the patch. Could everyone (also @addict3d, maybe @andi-huber) check it out in the 2.2-SNAPSHOT build of Indriya? Changing the status to "in Review", if the solution works feel free to close it.

evanjs commented 1 year ago

@keilw indeed, the latest SNAPSHOT appears to work without issue, and I am able to generate OpenAPI documentation (with our project that depends on Indriya) utilizing the Swagger Gradle plugin

Thank you!