CODEWITHSUNDEEP
javamultithreadingatomic
gurubelli
gurubelli

Reputation: 1329

Conditional checking on Atomic Integer thread safe?

Sample code

@ApplicationScoped
public class AtomicIntegerSample {

private final AtomicInteger successFullSyncCount = new AtomicInteger(0);
private final AtomicLong overallSyncTimeTaken  = new AtomicLong(0);

public void incrementSuccessFullSyncCount() {
    this.successFullSyncCount.incrementAndGet();
}


public void addOverallSyncTimeTaken(final Long syncTimeTaken) {
    overallSyncTimeTaken.addAndGet(syncTimeTaken);
}
/**
 * Can be called by multiple threads
 * @return
 */
public long getAverageSuccessfulSyncTimeTaken() {
    if (successFullSyncCount.get() == 0) {
        return 0;
    } else {
        return overallSyncTimeTaken.get() / successFullSyncCount.get();
    }
}

In method getAverageSuccessfulSyncTimeTaken() there is conditional check to avoid arithmetic exception.

The class ApplicationScoped, and this method can be called by multiple threads concurrently.

Is the method thread safe?. If I replace the code as follows, is it thread safe?

 /**
 * Can be called by multiple threads
 * @return
 */
public long getAverageSuccessfulSyncTimeTaken() {
       return overallSyncTimeTaken.get() / successFullSyncCount.get();

}

Then arithmetic exception will be thrown. How to optimally synchronize this code?. I mean only if successFullSyncCount.get() == 0

Upvotes: 1

Views: 612

Answers (2)

Andreas
Andreas

Reputation: 5093

Below is a version that does not use atomic objects.

If you expect times and counts to be updated together, a synchronized block might be a better fit. This will prevent your average method from being off by more than one. Admittedly a synchronized block is slower that the native atomic implementations, but I'm not sure optimization wouldn't be premature.

@ApplicationScoped
public class AtomicIntegerSample {

private int successFullSyncCount = 0;
private long overallSyncTimeTaken  = 0;


synchronized public void addSyncTimeTakenAndIncrementSyncCount(final Long syncTimeTaken) {
    overallSyncTimeTaken+=syncTimeTaken;
    successFullSyncCount++;
}
/**
 * Can be called by multiple threads
 * @return
 */
public long getAverageSuccessfulSyncTimeTaken() {
    if (successFullSyncCount.get() == 0) {
        return 0;
    } else {
        return overallSyncTimeTaken.get() / (float) successFullSyncCount.get();
    }
}

Upvotes: 0

JB Nizet
JB Nizet

Reputation: 691635

It won't throw any exception, but it could return incorrect results:

  • the time and the count are incremented separately. They're not incremented both at once, in a single atomic operation
  • the time and the count are read separately, instead of being read both at once in a single atomic operation.

So, you could very well read a time of 100 in a thread, then have another thread increment the count a few times, then read the count and perform the division. That can thus lead to an average time per sync that is lower than it is in reality. It could be a good enough approximation, or not.

Upvotes: 3

Related Questions