Should I synchronize method in my example?

Question

I'm not sure if I should synchronize method methodOne() in my example. I think not but I'm not 100% sure. Could you please give me advice what to do?

public class SynchroIssue {

class Test {
    private double a = 0;

    void methodOne() { 
        a++;
    }

    void go() {
        new Thread(new Runnable() {

            @Override
            public void run() {
                for (int i = 0; i < Integer.MAX_VALUE; i++) {
                    methodOne();
                    System.out.println(Thread.currentThread().getName() + ", a = " + a);
                }
            }
        }).start();
    }
}

public static void main(String... args) {
    SynchroIssue mainObj = new SynchroIssue();

    SynchroIssue.Test object1 = mainObj.new Test();
    SynchroIssue.Test object2 = mainObj.new Test();

    object1.go();
    object2.go();
}
}

var is not `static` so each `Test` will have its own `a` variable. — Fran Montero, Jan 28 '16 at 13:06
you'd get an issue only if you called `go()` more than once on the same `Test`. — Thilo, Jan 28 '16 at 13:12

score 4 · Answer 1 · answered Jan 28 '16 at 13:08

4

Assuming that you are actually going to use instances of the SynchroIssue class concurrently, which you are not doing currently, the answer is yes.

The increment operator is not atomic. It is actually 3 instructions:

Get the current value.
Add 1 to that.
Store new value.

If you are not synchronized, concurrent threads can overlap those steps resulting in strange behavior.

Another option, if you are truly only interested in integers, would be the use of AtomicInteger, which has methods to atomically increment.

answered Jan 28 '16 at 13:08

Brett Okken

6,210
1
19
25

AtomicInteger for the win. If you are not synchronized, and `a` is not volatile, then the results are even more unpredictable, I think (updates not properly shared between threads, it may jump back and forth). But I don't understand `volatile` in detail, and I don't have to, thanks to higher-level constructs like AtomicInteger. – Thilo Jan 28 '16 at 13:13

score 1 · Answer 2 · answered Jan 28 '16 at 13:15

1

object1 and object2 are different objects, each start only one thread, and the variable "a" is private and not static, so "a" are different objects too, and there is no interaction between threads. So there is no need to synchronise methodOne().

answered Jan 28 '16 at 13:15

Tilman Hausherr

17,731
7
58
97

score 1 · Answer 3 · answered Jan 28 '16 at 13:15

In this specific example there's no value to be gained by synchronising the methods because only a single thread ever actually interacts with a given instance.

If you called object1.go() twice you'd have a problem. Using synchronized would not be the best solution to that problem though, you should instead use a java.util.concurrent.atomic.DoubleAccumulator, although AtomicInteger would function just as well given that you start at 0 and only ever increment by 1.

In general, you should be wary of using synchronized to roll your own synchronisation protocols. Prefer instead known thread-safe classes where they're available. java.util.concurrent is a good place to look.

ZetQ · Answer 4 · 2016-01-28T13:29:11.477

You should, but it wouldn't solve your problem. If you would synchronize the method, only one thread would be able to increase the variable at a time. But the following System.out.println could still print another value, since by the time you call it, another thread may already have increased a. The solution for your problem would be, that methodOne would also have to return the variable. Something like this:

synchronized double methodOne() { 
    return ++a;
}

And the thread should do:

for (int i = 0; i < Integer.MAX_VALUE; i++) {
            System.out.println(Thread.currentThread().getName() + ", a = " + methodOne());
    }

EDIT: as others already pointed out, you only have to do this if you intend to make the variable static. Otherwise you can leave your code as it is.

score 0 · Answer 5 · answered Jan 28 '16 at 13:15

I want to add some hints to Brett Okken's answer:

Most of the times, when you have a member variable in your class which is modified by the methods of your class in a concurrent context by more than one thread, you should think about one of the synchronization scopes.
Always go for the smallest available scope of synchronization.

Hope this would be helpful.

Should I synchronize method in my example?

5 Answers5