views:

229

answers:

3

I have a small app that has a Render thread. All this thread does is draw my objects at their current location.

I have some code like:

public void render()
{
    // ... rendering various objects

    if (mouseBall != null) mouseBall.draw()

}

Then I also have some mouse handler that creates and sets mouseBall to a new ball when the user clicks the mouse. The user can then drag the mouse around and the ball will follow where the mouse goes. When the user releases the ball I have another mouse event that sets mouseBall = null.

The problem is, my render loop is running fast enough that at random times the conditional (mouseBall != null) will return true, but in that split second after that point the user will let go of the mouse and I'll get a nullpointer exception for attempting .draw() on a null object.

What is the solution to a problem like this?

+10  A: 

The problem lies in the fact that you are accessing mouseBall twice, once to check whether it is not null and another to call a function on it. You can avoid this problem by using a temporary like this:

public void render()
{
    // ... rendering various objects
    tmpBall = mouseBall;
    if (tmpBall != null) tmpBall.draw();
}
Greg Hewgill
@Greg Hewgill, Thank you.. I appreciate it.
Simucal
Exactly. Just make sure that mouseBall is volatile if you're going to do this without a synchronized block.
Dave L.
+8  A: 

You have to synchronize the if and draw statements so that they are guaranteed to be run as one atomic sequence. In java, this would be done like so:

    
public void render()
{
    // ... rendering various objects
    synchronized(this) {
        if (mouseBall != null) mouseBall .draw();
   }
}
Nathaniel Flath
equally good answer, thank you aseraphim
Simucal
This ends up holding the lock on "this" for the entire duration of the mouseBall.draw() method call, which may be more time than you need to hold the lock. Also, this is atomic ONLY if all other accesses (both reads and writes) to mouseBall are also surrounded by a synchronized (this) { ... } block.
Greg Hewgill
+1  A: 

I know you've already accepted other answers, but a third option would be to use the java.util.concurrent.atomic package's AtomicReference class. This provides retrieval, update and compare operations that act atomically without you needing any supporting code. So in your example:

public void render()
{
    AtomicReference<MouseBallClass> mouseBall = ...;

    // ... rendering various objects
    MouseBall tmpBall = mouseBall.get();
    if (tmpBall != null) tmpBall.draw();
}

This looks very similar to Greg's solution, and conceptually they are similar in that behind the scenes both use volatility to ensure freshness of values, and take a temporary copy in order to apply a conditional before using the value.

Consequently the exact example used here isn't that good for showing the power of the AtomicReferences. Consider instead that your other thread will update the mouseball vairable only if it was already null - a useful idiom for various initialisation-style blocks of code. In this case, it would usually be essential to use synchronization, to ensure that if you checked and found the ball was null, it would still be null when you tried to set it (otherwise you're back in the realms of your original problem). However, with the AtomicReference you can simply say:

mouseBall.compareAndSet(null, possibleNewBall);

because this is an atomic operation, so if one thread "sees" the value as null it will also set it to the possibleNewBall reference before any other threads get a chance to read it.

Another nice idiom with atomic references is if you are unconditionally setting something but need to perform some kind of cleanup with the old value. In which case you can say:

   MouseBall oldBall = mouseBall.getAndSet(newMouseBall);
   // Cleanup code using oldBall

AtomicIntegers have these benefits and more; the getAndIncrement() method is wonderful for globally shared counters as you can guarantee each call to it will return a distinct value, regardless of the interleaving of threads. Thread safety with a minimum of fuss.

Andrzej Doyle