Is static int thread safe if it is incremented in a single method?

Admin 1
Viewed 691

I want to track getVariableAndLogAccess(RequestInfo requestInfo) using the code below. Will it be thread safe if only these two methods access variable? What is the standard way to make it thread safe? 

public class MyAccessLog(){

    private int recordIndex = 0;
    private int variableWithAccessTracking = 42; 
    private final Map<Integer, RequestInfo> requestsLog = new HashMap<>();
    public int getVariableAndLogAccess(RequestInfo requestInfo){
        Integer myID = recordIndex++;
        int variableValue = variableWithAccessTracking;
        requestInfo.saveValue(variableValue);
        requestLog.put(myID, requestInfo);
        return variableValue;
     }
     public void setValueAndLog(RequestInfo requestInfo, int newValue){
        Integer myID = recordIndex++;
        variableWithAccessTracking = variableValue;
        requestInfo.saveValue(variableValue);
        requestLog.put(myID, requestInfo);
     }

/*other methods*/
}
thread-safety multithreading java
4 Answers

Will it be thread safe if only these two methods access variable?

No.

For instance, if two threads call setValueAndLog, they might end up with the same myID value.

What is the standard way to make it thread safe?

You should either replace your int with an AtomicInteger, use a lock, or a syncrhonized block to prevent concurrent modifications.

As a rule of thumb, using an atomic variable such as the previously mentioned AtomicInteger is better than using locks since locks involve the operating system. Calling the operating system is like bringing in the lawyers - both are best avoided for things you can solve yourself.

Note that if you use locks or synchronized blocks, both the setter and getter need to use the same lock. Otherwise the getter could be accessed while the setter is still updating the variable, leading to concurrency errors.

user_31995950

Will it be thread safe if only these two methods access variable?

Nope.

Intuitively, there are two reasons:

  • An increment consists of a read followed by a write. The JLS does not guarantee that the two will be performed as an atomic operation. And indeed, neither to Java implementations do that.

  • Modern multi-core systems implement memory access with fast local memory caches and slower main memory. This means that one thread is not guaranteed to see the results of another thread's memory writes ... unless there are appropriate "memory barrier" instructions to force main-memory writes / reads.

    Java will only insert these instructions if the memory model says it is necessary. (Because ... they slow the code down!)

Technically, the JLS has a whole chapter describing the Java Memory Model, and it provides a set of rules that allow you to reason about whether memory is being used correctly. For the higher level stuff, you can reason based on the guarantees provided by AtomicInteger, etcetera.

What is the standard way to make it thread safe?

In this case, you could use either an AtomicInteger instance, or you could synchronize using a primitive object locking (i.e the synchronized keyword) or a Lock object.

user_1399850

@Malt is right. Your code is not even close to be thread safe. You can use AtomicInteger for your counter, but LongAdder would be more suitable for your case, as it is optimized for cases where you need counting things and read the result of your counting less often then you update it. LongAdder also has the same thread safety assurance of AtomicInteger

From java doc on LongAdder:

This class is usually preferable to AtomicLong when multiple threads update a common sum that is used for purposes such as collecting statistics, not for fine-grained synchronization control. Under low update contention, the two classes have similar characteristics. But under high contention, expected throughput of this class is significantly higher, at the expense of higher space consumption.

user_16260570

This is a common approach to log in a thread safe way:
For counter use AtomicInteger counter with counter.addAndGet(1) method.
Add data using public synchronized void putRecord(Data data){ /**/}
If you only use recordIndex as a handler for the record you can replace a map with a synchronized list: List list = Collections.synchronizedList(new LinkedList());

user_58986260
Related