[jboss-dev-forums] [Design of AOP on JBoss (Aspects/JBoss)] - Thread-Safety problems GeneratedClassAdvisor

[jason.greene@jboss.com]

While glancing at the AOP code for possible explanations of JBAOP-480, I noticed 2 major thread safety issues in GeneratedClassAdvisor:

Problem #1 is inadequate  locking when using ConcurrentHashMap:

  |  public MethodJoinPointGenerator getJoinPointGenerator(MethodInfo info)
  |       {
  |          MethodJoinPointGenerator generator = (MethodJoinPointGenerator)joinPointGenerators.get(info.getJoinpoint());
  |          if (generator == null)
  |          {
  |             generator = new MethodJoinPointGenerator(GeneratedClassAdvisor.this, info);
  |             initJoinPointGeneratorsMap();
  |             joinPointGenerators.put(info.getJoinpoint(), generator);
  |          }
  |          return generator;
  |       }
  | 

There is a race between reading a value in a CHM and inserting one. So, in this case there is a strong possibility of multiple/different join-point generator instances being available to different threads.  For this reason CHM has a putIfAbsent() method that should be used instead.  If a race occurs a unneeded generator my be created, but it won't be seen by other threads, and gc'd soon.

Problem#2 - Usage of Double-Checked Locking

  | protected void initJoinPointGeneratorsMap()
  |    {
  |       if (joinPointGenerators == UnmodifiableEmptyCollections.EMPTY_CONCURRENT_HASHMAP)
  |       {
  |          lockWrite();
  |          try
  |          {
  |             if (joinPointGenerators == UnmodifiableEmptyCollections.EMPTY_CONCURRENT_HASHMAP)
  |             {
  |                joinPointGenerators = new ConcurrentHashMap();
  |             }
  |          }
  |          finally
  |          {
  |             unlockWrite();
  |          }
  |       }
  |    }
  | 

This is a known flawed approach to thread safety. 
http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
http://www.cs.umd.edu/~pugh/java/memoryModel/jsr-133-faq.html#dcl

This can be fixed by marking the joinPointGenerators map as volatile, or changing it to an AtomicReference.

-Jason

View the original post : http://www.jboss.com/index.html?module=bb&op=viewtopic&p=4098971#4098971

Reply to the post : http://www.jboss.com/index.html?module=bb&op=posting&mode=reply&p=4098971