Is the following incrementation code thread safe in java?

Question

Java Code:

public class IncreaseTest {
    public static int value = 0;

    public synchronized int increment() {
        return value++;
    }
}

Is method increment() thread-safe? Do I have to add the modifier keyword volatile as follows：

  public static volatile int value = 0;

Solution 1

This code is not thread-safe. The instance method will synchronize on an instance, if you have multiple instances they will not use the same monitor and therefor the updates can interleave.

You either need to remove the static from the value field or add static to the increment() method.

Also, as you have made value public, there is the additional problem that value can be changed or read outside of this method without using synchronisation which could result in reading old values.

So changing your code to the following will make it thread-safe:

public class IncreaseTest {
    private int value = 0;

    public synchronized int increment() {
        return value++;
    }
}

Solution 2

I do not think this is thread safe since the static variable is public and can be accessed by other threads in a non-thread safe manner. In order to be thread safe you must declare the variable as follows:

public static volatile int value;

Now value being volatile, will be accessed in a synchronized block.

Solution 3

You should probably use atomicvars

Comments

[Mark Rotteveel]

The synchronized keyword in the method declaration is the same as a having a lock(this) {...} in C#.

[JB Nizet]

Making the variable volatile is different from accessing it from a synchronized block. The ++ operation is still not an atomic operation, even if the variable is volatile.

[codeisee]

private int value = 0; //if remove static and change public to private,is that thread safe??

[niculare]

"Access to the variable acts as though it is enclosed in a synchronized block, synchronized on itself." according to this

[Mark Rotteveel]

@codeisee It will be threadsafe if you do that. I also included an example in my answer

[JB Nizet]

The comparison in the article is a bad, error-prone one. In particular, value++ makes two different accesses to value: one to read the value, and another to write it. Making value volatile won't ensure that the read and the write are enclosed in a single, atomic operation. Only synchronization ensures that. That means that you could have two threads reading in parallel, then writing in parallel, and the value would be incremented by 1 instead of being incremented by 2.

[JB Nizet]

@codeisee: note that you could simply use an AtomicInteger and its incrementAndGet() method, which does exactly what your code does, deals with thread-safety for you, and is more efficient.

[Mark Rotteveel]

@JBNizet getAndIncrement() for identical behavior as the code in the question.

[codeisee]

private volatile int value = 0; // @Mark Rotteveel,Is it necessary to add modifier keyword volatile?

[JB Nizet]

@MarkRotteveel: Yes, you're right :-) I always have to think twice and end up writing the code using two different instructions to make the intent clearer: int tmp = value; value++; return tmp;. Sounds dumb, but at least you can't misunderstand what the code does.

[Raul Luna]

Atomic vars are not a vacine for threading problems per se. IMHO it's better understand what are going on under the hood.