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

adding Monetary class' class loader to ServiceLoader's load method #333

Conversation

utkanozyurek
Copy link

@utkanozyurek utkanozyurek commented Jun 4, 2020

… to fix error in Java11 parallel stream loading error


This change is Reviewable

… to fix error in Java11 parallel stream loading error
@@ -97,7 +98,7 @@ public int getPriority() {
LOG.log(Level.SEVERE, "could not create services of type: " + serviceType, e);
}
try{
for(T service:ServiceLoader.load(serviceType)){
for(T service:ServiceLoader.load(serviceType, Monetary.class.getClassLoader())){
Copy link

@jwgmeligmeyling jwgmeligmeyling Jun 27, 2020

Choose a reason for hiding this comment

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

I am not sure if the Java Money implementation will always come from the same class loader as the API. Both classes should be resolvable from the context classloader. And yes, that may cause issues with parallel operations that are executed inside fork join pools. But won't this fix cause issues with deployments where the implementation lives in a separate module (in terms of OSGI or for example JBoss modules)? Or perhaps inside the application itself?

@keilw keilw merged commit 671dbc2 into JavaMoney:master Jun 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants