-
Notifications
You must be signed in to change notification settings - Fork 53
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
Bug#180_Stuck_threads_due_to_WeakHashMap #181
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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; | ||
|
||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
@@ -221,4 +248,5 @@ public List<ValidationProvider<?>> getValidationProviders() { | |
return Collections.emptyList(); | ||
} | ||
} | ||
|
||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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. YourValidatorFactory
should be created once and for all and shared. Creating a newValidatorFactory
each time will cost you as it won't have the bean metadata cached.