Prof. Dr. Eric Bodden

A few days ago I blogged about a few really subtle data races that can easily be triggered in the JDK, when invoking methods like containsAll on synchronized (!) collections. In the following code you can get a race on sl2 because sl1.containsAll(sl2) synchronizes on sl1 only, not on the argument sl2!

List sl1 = Collections.synchronizedList(new ArrayList());
List sl2 = Collections.synchronizedList(new ArrayList());
sl1.containsAll(sl2);

Here are now two easy aspect-oriented solutions to this problem, first in form of a tracematch, then in form of a normal AspectJ aspect. You can download the tracematch here and the plain AspectJ aspect here.

public aspect TMASyncContainsAll {

        Object tracematch(Collection t, Collection a) {
                sym sync1 after returning(t):
                	!within(ASyncContainsAll) && call(* Collections.synch*(..));
                sym sync2 after returning(a):
                	!within(ASyncContainsAll) && call(* Collections.synch*(..));
                sym starAll around(t,a):
                	!within(ASyncContainsAll) && call(* Collection.*All(Collection))
                	&& target(t) && args(a);

                sync1 sync2 starAll {
                	//if we don't have the lock on a
                        if(t!=a && !Thread.holdsLock(a)) {
                                //give error
                                System.err.println("Must synchronize argument collection at: " +
                                                thisJoinPointStaticPart.getSourceLocation());
                                //actually *do* synchronize and proceed
                                synchronized(a) {
                                        return proceed(t,a);
                                }
                        }
                        return proceed(t,a);
                }
        }
}

In the tracematch solution we first state (line 3) that we want to reason about two Collections, t (the call target) and a (the argument of the call to containsAll). Then we declare the symbol (or event) sync1 which occurs when we first return t from a call to Collections.synch*(..). Note how this matches synchronizedCollection(..) but also synchronizedList(..) etc. The symbol sync2 binds the argument a. The final symbol starAll matches all calls to Collection.*All(Collection) where t is the call target and a the argument. Again, this matches calls to containsAll but also addAll, etc. Line 12 states the tracematch pattern. This is the crux of the tracematch: “sync1 sync2 starAll” matches whenever we first synchronize returning the collection t, then synchronize returning a and last but not least we call *All with the respective arguments.

In the body we check whether t is actually different from a and if the current thread already correctly synchronized on a, i.e. holds a lock on a. (Note that if t==a then the call to *All is already correctly synchronized.) If not, we print the faulty source location, do synchronize and proceed with the original method call. If the thread already holds the lock, we just proceed. Pretty elegant, isn’t it?

Solution in plain AspectJ

The following is the respective solution in plain AspectJ.

public aspect ASyncContainsAll {

	//TODO should be a map with WEAK keys actually, for memory safety
	Set synched = Collections.newSetFromMap(new IdentityHashMap());

	after() returning(Collection c):
		!within(ASyncContainsAll) &&
		call(* Collections.synch*(..)) {
		synched.add(c);
	}

	Object around(Collection t, Collection a):
		!within(ASyncContainsAll) &&
		call(* Collection.*All(Collection)) && target(t) && args(a) {
		if(t!=a &&	//we don't invoke c.containsAll(c) (in this case we have the lock on c already)
		   synched.contains(t) && synched.contains(a) &&	//t and a were both synchronized
		   !Thread.holdsLock(a)) {	//we don't have the lock on a
			//give error
			System.err.println("Must synchronize argument collection at: " +
					thisJoinPointStaticPart.getSourceLocation());
			//actually *do* synchronize and proceed
			synchronized(a) {
				return proceed(t,a);
			}
		}
		//in any other case, just proceed as usual
		return proceed(t,a);
	}

}

For the test program at the top of the post this would yield:

Must synchronize argument collection at: TestClass.java:3