Tools for finding Shared Mutable data bugs in Java

Question

I have a large legacy system to maintain. The codebase uses threads all over the place and those threads share a lot of mutable data. I know, sounds bad. Anyway, don't answer "rewrite the whole application from scratch" or I'll vote you down :-) I have tried to run some static analysis tools on the codebase, but none of those seem to catch this case which occurs a lot in our source code: multiple threads are reading and writing variables which are not marked as volatile or synchronized at all. Typically this occurs on "runFlag"-type variables. An example of this is on Effective Java 2nd edition page 260:

public class StopThread
{
    private static boolean stopRequested;
    public static void main(String[] args) throws InterruptedException
    {
        Thread backgroundThread = new Thread(new Runnable()
        {
            public void run()
            {
                int i = 0;
                while (!stopRequested)
                {
                    i++;
                }
            }
        });
        backgroundThread.start();
        Thread.sleep(1000);
        stopRequested = true;
    }
}

This example never finishes on Windows/Linux with "-server" startup parameter given to Sun JVM. So, is there any (semi-)automatic way to find these issues, or do I have to rely totally on code reviews?

Answer 1

Chris Grindstaff wrote an article FindBugs, Part 2: Writing custom detectors in which he describes how to use the BCEL to add your own rules. (BCEL isn't the only bytecode library - but it is the one used by FindBugs.)

The code below emits any cases where a method accesses a static method or field. You could run it on any type that implements Runnable.

public class StaticInvocationFinder extends EmptyVisitor {

    @Override
    public void visitMethod(Method obj) {
        System.out.println("==========================");
        System.out.println("method:" + obj.getName());

        Code code = obj.getCode();
        InstructionList instructions = new InstructionList(code.getCode());
        for (Instruction instruction : instructions.getInstructions()) {
            // static field or method
            if (Constants.INVOKESTATIC == instruction.getOpcode()) {
                if (instruction instanceof InvokeInstruction) {
                    InvokeInstruction invokeInstruction = (InvokeInstruction) instruction;
                    ConstantPoolGen cpg = new ConstantPoolGen(obj
                            .getConstantPool());
                    System.out.println("static access:"
                            + invokeInstruction.getMethodName(cpg));
                    System.out.println("      on type:"
                            + invokeInstruction.getReferenceType(cpg));
                }
            }
        }
        instructions.dispose();
    }

    public static void main(String[] args) throws Exception {
        JavaClass javaClass = Repository.lookupClass("StopThread$1");

        StaticInvocationFinder visitor = new StaticInvocationFinder();
        DescendingVisitor classWalker = new DescendingVisitor(javaClass,
                visitor);
        classWalker.visit();
    }

}

This code emits the following:

==========================
method:<init>
==========================
method:run
static access:access$0
      on type:StopThread

It would be possible to then scan the type StopThread, find the field and check to see if it is volatile.

Checking for synchronization is possible, but might get tricky due to multiple MONITOREXIT conditions. Walking up call stacks could be difficult too, but then this isn't a trivial problem. However, I think it would be relatively easy to check for a bug pattern if it has been implemented consistently.

BCEL looks scantly documented and really hairy until you find the BCELifier class. If you run it on a class, it spits out Java source of how you would build the class in BCEL. Running it on StopThread gives this for generating the access$0 synthetic accessor:

  private void createMethod_2() {
    InstructionList il = new InstructionList();
    MethodGen method = new MethodGen(ACC_STATIC | ACC_SYNTHETIC, Type.BOOLEAN, Type.NO_ARGS, new String[] {  }, "access$0", "StopThread", il, _cp);

    InstructionHandle ih_0 = il.append(_factory.createFieldAccess("StopThread", "stopRequested", Type.BOOLEAN, Constants.GETSTATIC));
    il.append(_factory.createReturn(Type.INT));
    method.setMaxStack();
    method.setMaxLocals();
    _cg.addMethod(method.getMethod());
    il.dispose();
  }

Answer 2

The latest version of FindBugs will attempt to check that fields marked with the @GuardedBy annotation are accessed only within the appropriate guard code.

Answer 3

Coverity Thread Analyzer does the job, but that is quite expensive. IBM Multi-Thread Run-time Analysis Tool for Java seems to be able to detect those but it appears somewhat more difficult to set up. These are dynamical analysis tools that detect which actual variables were accessed from different threads without proper synchronization or volatility, so results are more accurate than with static analysis, and are able to find lots of issues that static analysis is unable to detect.

If your code is mostly or at least partly properly synchronized, fixing FindBugs (or other static analysis) concurrency checks might help too, at least rules IS2_INCONSISTENT_SYNC and UG_SYNC_SET_UNSYNC_GET might be good ones to start with.

Answer 4

FindBugs and professional tools based on it are your best hope, but don't count on them finding all of the concurrency woes in your code.

If things are in that bad a shape, then you should supplement the tooling with analysis by a human java concurrency expert.

This is a hard problem because conclusively prooving the correctness of an existing, but modified, code base is probably going to be unrealistic - especially in the face of concurrent usage.