Support concurrent on DefaultClassResolver #46
Conversation
kazuki43zoo
changed the title
| @@ -41,28 +42,30 @@ | |||
| */ | |||
| public class DefaultClassResolver extends Object implements ClassResolver | |||
| { | |||
| private Map classes = new HashMap(101); | |||
| private Map classes = new ConcurrentHashMap(101); | |||
There was a problem hiding this comment.
Can you use generics here? This will reduce casting below.
There was a problem hiding this comment.
I've changed to use generics. and add final.
| try { | ||
| result = Class.forName(className); | ||
| } catch (ClassNotFoundException ex) { | ||
| if (className.indexOf('.') == -1) { |
There was a problem hiding this comment.
I think when we are here, this if clause can be moved out of the catch and implemented as follow (classForName already throws ClassNotFoundException):
if (className.indexOf('.') == -1) {
result = Class.forName("java.lang." + className);
classes.put("java.lang." + className, result);
} else {
classes.put(className, Class.forName(className));
}then result and additional check for null is not needed.
There was a problem hiding this comment.
I refactored this code with your suggestion as reference.
|
@lukaszlenart Thanks for your review! Thanks! |
| } else { | ||
| result = Class.forName(className); | ||
| } | ||
| classes.put(className, result); |
There was a problem hiding this comment.
This will duplicate langClassName, so we will end up with two entries for the same class.
There was a problem hiding this comment.
Thanks for your comment.
I think this behavior is same as the current version. Should change this behavior in this change?
There was a problem hiding this comment.
Right, which means the java.lang. classes will be always located by the short version (Integer instead of java.lang.Integer). I think .put in line 61 can be thrown aways.
There was a problem hiding this comment.
I've fixed your comment. please review again.
Thanks.
| if (result != null) { | ||
| return result; | ||
| } | ||
| result = (className.indexOf('.') == -1) ? Class.forName("java.lang." + className) : Class.forName(className); |
There was a problem hiding this comment.
I do prefer pure if clause but that's ok :)
|
Do you want to port this fix into previous branches? |
|
LGTM 👍 |
|
Thanks for merging! |
|
3.2.5 is out and under way to the Central. |
|
Wow, It's great!! |
…on-DefaultClassResolver Support concurrent on DefaultClassResolver (cherry picked from commit 26f9c15)
The
DefaultClassResolveris not thread-safe.I will propose to support concurrent using
ConcurrentHashMapon theDefaultClassResolver.WDYT?