When to use synchronized in Java

Question

I hope this is going to be enough information, so here it goes. If you need more info, lemme know in the comments.

I have a class that has two inner classes. The inner classes each have two methods that call a method in the outer class. So, it looks like this:

public OuterClass {
    private boolean outerMethodHasBeenCalled = false;

    private void outerMethod() {
        if(!outerMethodHasBeenCalled) {
            // do stuff
        }

        outerMethodHasBeenCalled = true;
    }

    private FirstInnerClass {
        public void someMethod() {
            outerMethod();
        }
    }

    private SecondInnerClass {
        public void someOtherMethod() {
            outerMethod();
        }
    }
}

It's important to note that:

• This is for an Android app. Instances of FirstInnerClass and SecondInnerClass are passed to a WebView as a JavaScript interface, so someMethod and someOtherMethod can be called at any time, in no particular order.
• I currently have a problem with the existing code (without the synchronized keyword) where outerMethod is called pretty much at the exact same time (I print out a log message, and they're timestamped to the 1000th of a second) by different objects. My app then 'does stuff' twice because outerMethodHasBeenCalled is still false when outerMethod was called. This is not okay, and it is exactly what I'm trying to prevent. My app should only 'do stuff' once and only once: the first time outerMethod is called.
• It might sound like I have multiple instances of OuterClass, but rest assured that it's only one instance of OuterClass.

It's important that my app 'does stuff' only the first time outerMethod gets called (I hope that's evident by now). All subsequent calls are essentially ignored. Whichever inner class calls outerMethod first -- doesn't matter.

So, is it appropriate to use the synchronized keyword in this case?

Solution 1

Yup, given what you've laid out above, I'd go with:

private synchronized void outerMethod() {
...
}

Note, this will have the side-effect of blocking one of the callers until the outerMethod() completes. If that is acceptable, cool. If the intent is merely that the code in outerMethod() is run once, and it is OK for the second caller not to be delayed if the first caller is running outerMethod(), you might consider:

public OuterClass {
    private AtomicBoolean outerMethodHasBeenCalled = new AtomicBoolean();

    private void outerMethod() {
        if (outerMethodHasBeenCalled.compareAndSet(false, true)) {
            // do stuff
        }
    }
...

See the JavaDoc for AtomicBoolean to grok what is going on there (assuming it is available in Android's Java).

Solution 2

Wrap everything in outerMethod that you want to run only once in a synchronized block:

private void outerMethod() {
    synchronized (this) {
        if(!outerMethodHasBeenCalled) {
            // do stuff
        }

        outerMethodHasBeenCalled = true;
    }
}

That way, the first time the method is called, only one thread will be allowed into the synchronized block at a time. The first one will execute the code in the if statement, then set outerMethodHasBeenCalled to true. The other threads will see that it is true, and skip the if code.

Comments

[hansvb]

don't you also need to make the flag volatile (so that other Threads can reliably see the changes)? Maybe use an AtomicBoolean to be on the safe side.

[Avi]

@Thilo: If all accesses to the flag are inside synchronized blocks, there is no need to use volatile. The Java Memory Model ensures that changes will be visible. AtomicBoolean and volatile are useful when you don't want to have the cost of a full synchronized block, though they are trickier to use.

[brettw]

@Thilo no you don't need to, if the only place that outerMethodHasBeenCalled is accessed is in outerMethod. The synchronized keyword denotes a memory synchronization barrier. Read the VM spec on the synchronized keyword. BTW, pls don't do stuff just 'to be on the safe side'. Understand exactly what you are writing and the implications. Other programmers that come into your code after you who DO understand will be temporarily side-tracked trying to figure out why a construct was used, e.g. "He must of used volatile for a reason, what am I not seeing?"

[hansvb]

"BTW, pls don't do stuff just 'to be on the safe side'. Understand exactly what you are writing and the implications. " I was not suggesting using volatile to be on the safe side. Quite the contrary: Because the low-level primitives are too complicated for mortals to understand (and buggy in their implementations at times), I like to stay away completely from synchronized or volatile and use the more user-friendly higher-level tools in java.util.concurrent "to be on the safe side".