4 Replies Latest reply on Oct 29, 2007 1:32 PM by kabirkhan

Thread-Safety problems GeneratedClassAdvisor

jason.greene Oct 25, 2007 3:00 PM

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

1. Re: Thread-Safety problems GeneratedClassAdvisor

flavia.rainone Oct 26, 2007 1:29 AM (in response to jason.greene)

Jason, thanks for the input. Both issues you have raised are already fixed in the trunk. :)

Unfortunately, none of them issues are the cause of bug JBAOP-480.

What is happening is that a thread is trying to generate a joinpoint class for a joinpoint whose info interceptor chain is null. Here is how this happens: in an if-statement, thread 1 checks that the info object of joinpoint A contains a non-empty interceptor chain; thread 1 is preempted by thread2; thread2 removes the last interceptor bound to joinpoint A, recreating a new info field for joinnpoint A with a null interceptor chain; thread1 resumes, generating code to intercept joinpoint A, without being aware that its info contains now a null interceptor chain.

To avoid that, all I should do is to synchronize the thread on the joinpoint itself. This, added to some synchronization I have put on JoinPointInfo (I haven't commited this part since, alone, it is not a solution to anything), would do the trick.

But the problem is which object should be picked for the synchronization on the joinpoint? If we generated only one joinpoint info during all the JBoss AOP execution, this info would be a no-brain choice. However, since a new info is created at every rebuildInterceptors execution, we have no such object.

The interesting part is that I was already going to propose a change in this structure before Jason reported this concurrency isse :). My change would allow us to solve this problem easily, as well as bug JBAOP-380.

Kabir, I'll let you know a more detailed picture of this new approach tomorrow morning.
Actions
2. Re: Thread-Safety problems GeneratedClassAdvisor

flavia.rainone Oct 29, 2007 12:49 PM (in response to jason.greene)

To fix that, I will also need to generate synchronized code.

When I tried to use a synchronized block on our javassist generated code I've code a VerifyError. Kabir, do you know if it is possible to use synchronized blocks on javassist generated code?
Actions
3. Re: Thread-Safety problems GeneratedClassAdvisor

flavia.rainone Oct 29, 2007 12:52 PM (in response to jason.greene)

I've code a VerifyError.

I meant "I've gotten a VerifyError"
Actions
4. Re: Thread-Safety problems GeneratedClassAdvisor

kabirkhan Oct 29, 2007 1:32 PM (in response to jason.greene)

As far as I know it should be possible, but I am not 100% sure. Try a simple test
Actions

Go to original post