Breaking Concurrent Code

Question

Let's say I have the following class:

public class BuggyClass {

    private String failField = null;

    public void create() {
        destroy();
        synchronized (this) {
            failField = new String("Ou! la la!");
        }
    }

    public void destroy() {
        synchronized (this) {
            failField = null;
        }
    }

    public long somethingElse() {
        if (failField == null) {
            return -1;
        }
        return failField.length();
    }

}

It's easy to see that in a multithreaded execution of the above code we could get a NullPointerExeption in somethingElse. For example, it could be that failField != null and before returning failField.length() destroy gets called therefore making failField to null.

I want to create a multithreaded program that is going to be able to "throw" a NullPointerException when using BuggyClass. I know, that since the program is multithreaded, it could be that this never happens but I guess there should be some better test that increases the probability of getting an exception. Right?

I tried the following:

final BuggyClass bc = new BuggyClass();
final int NUM_OF_INV = 10000000;
int NUM_OF_THREADS = 5;
ExecutorService executor = Executors.newFixedThreadPool(3 * NUM_OF_THREADS);

for (int i = 0; i < (NUM_OF_THREADS); ++i) {
    executor.submit(new Runnable() {
        public void run() {
            for(int i = 0; i< NUM_OF_INV; i++){
                bc.create();
            }
        }
    });
}


for (int i = 0; i < (NUM_OF_THREADS); ++i) {
    executor.submit(new Runnable() {
        public void run() {
            for(int i = 0; i< NUM_OF_INV; i++){
                bc.destroy();
        }}
    });
}

for (int i = 0; i < (NUM_OF_THREADS); ++i) {
    executor.submit(new Runnable() {
        public void run() {
            for(int i = 0; i< NUM_OF_INV; i++){
                bc.somethingElse();
        }}
    });
}   
executor.shutdown(); executor.awaitTermination(1, TimeUnit.DAYS);  

I executed the above code (method) multiple times with different NUM_OF_INV and NUM_OF_THREADS but NEVER managed to get a NullPointerException.

Any ideas on how I can create a test that increases my chances of getting an exception without changing BuggyClass?

Answer 1

It does fail... on my machine at least. The thing is that the Runnable swallows the exception. Try instead:

            executor.submit(new Runnable() {
                public void run() {
                    for (int i = 0; i < NUM_OF_INV; i++) {
                        try {
                            bc.somethingElse();
                        } catch (NullPointerException e) {
                            e.printStackTrace();
                        }
                    }
                }
            });

I get NPE's every time I run it.

Answer 2

Although there is a data race in your code, it might be impossible to see any problems, that are caused by this data race. Most likely, the JIT compiler will transform the method somethingElse into something like this:

public long somethingElse() {
    String reg = failField; // load failField into a CPU register
    if (reg == null) {
        return -1;
    }
    return reg.length();
}

That means, the compiler will not load the reference failField after the condition. And it is impossible to trigger the NullPointerException.

---

Update: I have compiled the method somethingElse with GCJ to see some real and optimized assembler output. It looks as follows:

long long BuggyClass::somethingElse():
    movq    8(%rdi), %rdi
    testq   %rdi, %rdi
    je      .L14
    subq    $8, %rsp
    call    int java::lang::String::length()
    cltq
    addq    $8, %rsp
    ret
.L14:
    movq    $-1, %rax
    ret

You can see from this code, that the reference failField is loaded once. Of course, there is no guarantee, that all implementations will use the same optimization now and forever. So, you shouldn't rely on it.

Answer 3

If you just want to see the problem, you could add short sleeps before you call failField.length() and also immediately after the failField = null in the destroy() method. This would widen the window for the somethingElse() method to access the variable in a null state.

Answer 4

I'm surprised that nobody has noticed the fact that "failedField" was not prefixed with a volatile keyword.

While it's true there is an opportunity for a race to occur in the create() method, the reason why it probably works on other people's machines is the fact that "failedField" was not in shared memory and a cached value of "failedField" was used instead.

Also, references on 64 bit machines aren't as threadsafe as you think. That's why AtomicReference exists in java.util.concurrent