[jboss-dev] [jboss-cvs] JBossAS SVN: r94875 - in projects/jboss-cl/trunk: classloader/src/main/java/org/jboss/classloader/spi/filter and 7 other directories.

[Adrian Brock]

You're probably right, although I don't trust volatile. 
It's my c and pre Java5 background. ;-)

The issue flagged by FindBugs was that some access to
this ivar was synchronized and some wasn't.
The gets and sets were synchronized.

The real issue was code like the following
in an unsynchronized method:

if (loadedClass != null)
   doStuff();

blah();

if (loadClass == null)
   doSomethingElse();

Which won't work with volatile. The two if statements could
potentially see different values (although in practice
they won't in this use case).

However, I also fixed that code to copy the ivar into a local variable
at the start of the method so it has a consistent view
throughout, which should mean volatile is ok. At least with
the lastest java memory model. ;-)

On Wed, 2009-10-14 at 11:07 -0500, David M. Lloyd wrote:
> On 10/14/2009 11:01 AM, jboss-cvs-commits at lists.jboss.org wrote:
> > Author: adrian at jboss.org
> > Date: 2009-10-14 12:01:45 -0400 (Wed, 14 Oct 2009)
> > New Revision: 94875
> 
> > Modified: projects/jboss-cl/trunk/classloader/src/main/java/org/jboss/classloader/spi/base/ClassLoadingTask.java
> > ===================================================================
> > --- projects/jboss-cl/trunk/classloader/src/main/java/org/jboss/classloader/spi/base/ClassLoadingTask.java	2009-10-14 15:23:40 UTC (rev 94874)
> > +++ projects/jboss-cl/trunk/classloader/src/main/java/org/jboss/classloader/spi/base/ClassLoadingTask.java	2009-10-14 16:01:45 UTC (rev 94875)
> > @@ -27,6 +27,7 @@
> >   import java.security.CodeSource;
> >   import java.security.PrivilegedAction;
> >   import java.security.ProtectionDomain;
> > +import java.util.concurrent.atomic.AtomicReference;
> [..]
> > +      return loadedClass.get();
> [..]
> > +      this.loadedClass.set(loadedClass);
> [..]
> > +      Class<?>  loadedClass = getLoadedClass();
> [..]
> > +      Class<?>  loadedClass = getLoadedClass();
> [..]
> > +         this.loadedClass.set(theClass);
> [..]
> > +         return loadedClass.get();
> [..]
> > +         Class<?>  loadedClass = getLoadedClass();
> 
> If you only do get & set, might as well just make it volatile instead of 
> using AtomicReference, and save a bit of memory.
> 
> - DML
> _______________________________________________
> jboss-cvs-commits mailing list
> jboss-cvs-commits at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/jboss-cvs-commits
-- 
xxxxxxxxxxxxxxxxxxxxxxxxxxxx
Adrian Brock
Chief Scientist
JBoss by Red Hat
xxxxxxxxxxxxxxxxxxxxxxxxxxxx