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.

Sign up for GitHub

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

Bug#180_Stuck_threads_due_to_WeakHashMap #181

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Bug#180_Stuck_threads_due_to_WeakHashMap #181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 4 additions & 2 deletions src/main/java/jakarta/validation/Validation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,10 @@
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.ServiceConfigurationError;
import java.util.ServiceLoader;
import java.util.WeakHashMap;
Expand Down Expand Up @@ -327,8 +329,8 @@ private static class GetValidationProviderListAction implements PrivilegedAction

//cache per classloader for an appropriate discovery
//keep them in a weak hash map to avoid memory leaks and allow proper hot redeployment
private final WeakHashMap<ClassLoader, SoftReference<List<ValidationProvider<?>>>> providersPerClassloader =
new WeakHashMap<>();
private final Map<ClassLoader, SoftReference<List<ValidationProvider<?>>>> providersPerClassloader =
Collections.synchronizedMap(new WeakHashMap<ClassLoader, SoftReference<List<ValidationProvider<?>>>>());
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure you're not doing something wrong in your code? Because write operations to this map should be extremely rare and if you have issues, I wonder if you are not doing something wrong.
That being said, I think the change makes sense.

@yrodiere could you have a look at this change too?

Copy link
Author

Choose a reason for hiding this comment

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

@gsmet thank you for the comments
there are few issues related to WeakHashMap not being synchronized , I have checked with JDK team too

eclipse-ee4j/metro-jax-ws#61
spring-projects/spring-loaded#194
https://www.adam-bien.com/roller/abien/entry/endless_loops_in_unsychronized_weakhashmap

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not saying this is not an issue. I'm saying that you shouldn't have it as you shouldn't call the method accessing this often: my wild guess is that, if you're hitting this, you are creating ValidatorFactory instances far too often. Your ValidatorFactory should be created once and for all and shared. Creating a new ValidatorFactory each time will cost you as it won't have the bean metadata cached.


public static synchronized List<ValidationProvider<?>> getValidationProviderList() {
if ( System.getSecurityManager() != null ) {
Expand Down
28 changes: 28 additions & 0 deletions src/test/java/jakarta/validation/ValidationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@

import java.io.IOException;
import java.lang.ref.SoftReference;
import java.lang.reflect.Constructor;
import java.lang.reflect.Field;
import java.lang.reflect.InvocationTargetException;
import java.net.URL;
import java.net.URLClassLoader;
import java.util.ArrayList;
Expand All @@ -27,6 +30,7 @@
import jakarta.validation.Validation;
import jakarta.validation.ValidationProviderResolver;
import jakarta.validation.ValidatorFactory;
import jakarta.validation.bootstrap.GenericBootstrap;
import jakarta.validation.NonRegisteredValidationProvider.NonRegisteredConfiguration;
import jakarta.validation.spi.ValidationProvider;

Expand Down Expand Up @@ -165,6 +169,29 @@ private int countInMemoryProviders() {
}
return count;
}

@Test
public void testWeakHashMapSynchronization() {
try {
Validation validation = new Validation();
GenericBootstrap f1 = Validation.byDefaultProvider();
System.out.println(f1.getClass().getDeclaredField("defaultResolver"));
Class<?>[] classes = validation.getClass().getDeclaredClasses();
for (int i = 0; i < classes.length; i++) {
if (classes[i].getSimpleName().equals("GetValidationProviderListAction")) {
Field field = classes[i].getDeclaredField("providersPerClassloader");
field.setAccessible(true);
Constructor<?> constructor = classes[i].getDeclaredConstructor();
constructor.setAccessible(true);
Object obj = constructor.newInstance();
assertTrue(field.get(obj).getClass().getName().equals("java.util.Collections$SynchronizedMap"));
}
}
} catch (Exception e) {
e.printStackTrace();
fail();
}
}
Comment on lines +173 to +194
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks but you can drop the test, it really has no value. If we were doing that for everything, it would just be a big nightmare to maintain.

Copy link
Author

Choose a reason for hiding this comment

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

okay i will remove the test


private static class CustomValidationProviderClassLoader extends ClassLoader {
private static final String SERVICES_FILE = "META-INF/services/" + ValidationProvider.class.getName();
Expand Down Expand Up @@ -221,4 +248,5 @@ public List<ValidationProvider<?>> getValidationProviders() {
return Collections.emptyList();
}
}

}