3

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?

Improve this question
9
  • I think failField == null is considered a single operation in Java. Not many little operations. Commented Apr 16, 2014 at 20:44
  • 2
    It's exceedingly hard to force a condition like this (unless you employ a fool to do it, of course -- fools are pretty darned ingenious at times). Commented Apr 16, 2014 at 20:45
  • It's not recommended to throw NullPointerException by your own. Commented Apr 16, 2014 at 20:45
  • String is immutable in Java, and so it makes it Thread safe. Any immutable class is thread safe. Commented Apr 16, 2014 at 20:45
  • 3
    Folks: He's trying to cause a failure in (clearly) buggy code. Has nothing to do with whether Strings are immutable or imortal, or whether == on a String reference is a single atomic operation. Commented Apr 16, 2014 at 20:50

4 Answers 4

Reset to default
4

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.

Improve this answer
Sign up to request clarification or add additional context in comments.

7 Comments

Thanks, but how can I prove this? Can I check the created java byte code to verify that this is the case?
This would be fairly easily verified with javap. (Note that if the compiler does not do this, the JVM/JITC in theory is not allowed to.)
@HotLicks: Can you elaborate on that? (What "theory" are you referring to?) Because I was pretty sure that the JVM/JITC was absolutely allowed to make this sort of transformation . . .
@HotLicks: I don't understand your comment. The JIT compiler can do all kinds of transformations, as long as the programs still behaves as specified by the JLS.
@user3542880: Why do you want to prove, that there is a problem with this code? Is it just about this code? Or are you interested in seeing problems of data races in general?
|
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.

Improve this answer

Comments

0

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.

Improve this answer

2 Comments

He specifically mentions that BuggyClass should not be modified.
I won't tell anyone if you don't.
0

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

Improve this answer

4 Comments

What do you mean with "reference on 64 bit machines aren't as threadsafe as you think". AFAIK references in Java are exactly as thread-safe on 64 bit machines as on 32 bit machines.
Aha! It's all about machine cycles and instruction interruptability. A CPU is able to interrupt some of the longer running instructions. Have you actually checked the link I provided above before you commented?
I'm tempted to -1 for your last paragraph, which betrays a serious misunderstanding of the Java memory model. Reads of references and writes of references are each thread-safe and atomic, regardless of the machine; java.util.concurrent.AtomicReference is only needed for compare-and-set (and the more sophisticated access patterns that it underlies). And this is explicitly mentioned in the answer that you link to.
My apologies - references are in fact atomic in the JVM spec. However, non-nolatile double and floats aren't atomic. See stackoverflow.com/questions/3463658/…

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.