You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@wicket.apache.org by Igor Vaynberg <ig...@gmail.com> on 2010/10/23 19:24:38 UTC
Re: svn commit: r1026650 - /wicket/trunk/wicket/src/main/java/org/apache/wicket/application/DefaultClassResolver.java
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, <mg...@apache.org> wrote:
> Author: mgrigorov
> Date: Sat Oct 23 16: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));
> }
> }
>
>
>
Re: svn commit: r1026650 - /wicket/trunk/wicket/src/main/java/org/apache/wicket/application/DefaultClassResolver.java
Posted by Martin Grigorov <mg...@apache.org>.
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 <ig...@gmail.com>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, <mg...@apache.org> 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));
> > }
> > }
> >
> >
> >
>