You may be right. There is a comment at the top of this class: /** * Usually class loaders implement more efficient caching strategies than we could possibly do, * but we experienced synchronization issue resulting in stack traces like: * java.lang.LinkageError: duplicate class definition: * * <pre> * wicket/examples/repeater/RepeatingPage at java.lang.ClassLoader.defineClass1(Native Method) * </pre> * * This problem has gone since we synchronize the access. */ private final ConcurrentHashMap<String, WeakReference<Class<?>>> classes = new ConcurrentHashMap<String, WeakReference<Class<?>>>();
I understood this as: use ConcurrentHashMap instead of HashMap. Maybe this comment is actually meant for the 'synchronized' block in resolveClass() and since 'classes' is the only member field of this class it is used as monitor. Now, here is the history: https://fisheye6.atlassian.com/browse/wicket/trunk/wicket/src/main/java/org/apache/wicket/application/DefaultClassResolver.java#r899413 In https://fisheye6.atlassian.com/browse/~raw,r=560279/wicket/trunk/jdk-1.4/wicket/src/main/java/org/apache/wicket/application/DefaultClassResolver.javaall looks easy: synchronize on 'classes' and 'classes.put()' is in the synchronized block. in https://fisheye6.atlassian.com/browse/~raw,r=561780/wicket/trunk/jdk-1.4/wicket/src/main/java/org/apache/wicket/application/DefaultClassResolver.java Johan moved 'classes.put()' out of the synch block, i.e. the just loading of class X must be synchronized otherwise LinkageError may occur later due to WICKET-2672 Juergen changed loader.loadClass(classname); with clazz = Class.forName(classname, false, loader); https://fisheye6.atlassian.com/browse/wicket/trunk/wicket/src/main/java/org/apache/wicket/application/DefaultClassResolver.java?r2=899413&r1=824877 and now 'synchronized' *may* not be needed anymore. To be on the safe side I think we should revert my change. I see some more issues: - Class.forName() doesn't work fine for primitive types and void (see its javadoc). We have checks for the primitives, but not for void. I doubt someone will ever try to load void but still ... - we use classes#put(), but maybe we should use #putIfAbsent() since there is no synchronization to prevent second addition Both are minor. On Sat, Oct 23, 2010 at 7:24 PM, Igor Vaynberg <[email protected]>wrote: > i havent reviewed the code, but the findbugs rule here may be > misleading. it is possible that someone is using the "classes" > variable as the lock object to synchornize code that has nothing to do > with what the "classes" variable actually is. just because it happens > to be a concurrent collection doesnt make it any less of a candidate > to synchronize on. so with that in mind review your change and make > sure it still makes sense. > > -igor > > On Sat, Oct 23, 2010 at 9:57 AM, <[email protected]> wrote: > > Author: mgrigorov > > Date: Sat Oct 23 166:57:04 2010 > > New Revision: 1026650 > > > > URL: http://svn.apache.org/viewvc?rev=1026650&view=rev > > Log: > > Findbugs warnings: > .../wicket/src/main/java/org/apache/wicket/pageStore/AsynchronousDataStore.java:285 > Synchronization performed on java.util.concurrent.ConcurrentHashMap > > > > Remove not needed synchronization: > > 1) the synchronized object is concurrent > > 2) the #put() actually is out of the synchronized block > > > > Modified: > > > > wicket/trunk/wicket/src/main/java/org/apache/wicket/application/DefaultClassResolver.java > > > > Modified: > wicket/trunk/wicket/src/main/java/org/apache/wicket/application/DefaultClassResolver.java > > URL: > http://svn.apache.org/viewvc/wicket/trunk/wicket/src/main/java/org/apache/wicket/application/DefaultClassResolver.java?rev=1026650&r1=1026649&r2=1026650&view=diff > > > ============================================================================== > > --- > wicket/trunk/wicket/src/main/java/org/apache/wicket/application/DefaultClassResolver.java > (original) > > +++ > wicket/trunk/wicket/src/main/java/org/apache/wicket/application/DefaultClassResolver.java > Sat Oct 23 16:57:04 2010 > > @@ -101,21 +101,19 @@ public final class DefaultClassResolver > > } > > else > > { > > - synchronized (classes) > > + ClassLoader loader = > Thread.currentThread().getContextClassLoader(); > > + if (loader == null) > > { > > - ClassLoader loader = > Thread.currentThread().getContextClassLoader(); > > - if (loader == null) > > - { > > - loader = > DefaultClassResolver.class.getClassLoader(); > > - } > > - // see > http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6500212 > > - // clazz = > loader.loadClass(classname); > > - clazz = Class.forName(classname, > false, loader); > > - if (clazz == null) > > - { > > - throw new > ClassNotFoundException(classname); > > - } > > + loader = > DefaultClassResolver.class.getClassLoader(); > > } > > + // see > http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6500212 > > + // clazz = loader.loadClass(classname); > > + clazz = Class.forName(classname, false, > loader); > > + if (clazz == null) > > + { > > + throw new > ClassNotFoundException(classname); > > + } > > + > > classes.put(classname, new > WeakReference<Class<?>>(clazz)); > > } > > } > > > > > > >
