How to fix race conditions without using synchronized (Lock free sequence counter implementation)?

Question

Have a scenario where multiple threads have race condition on comparison code.

private int volatile maxValue;
private AtomicInteger currentValue;

public void constructor() {
   this.current = new AtomicInteger(getNewValue());
}

public getNextValue() {
  while(true) {
     int latestValue = this.currentValue.get();
     int nextValue = latestValue + 1;
     if(latestValue == maxValue) {//Race condition 1 
       latestValue = getNewValue();
     }
    if(currentValue.compareAndSet(latestValue, nextValue) {//Race condition 2
      return latestValue;
    }
  }
}

private int getNewValue() {
    int newValue = getFromDb(); //not idempotent
    maxValue = newValue + 10;
    return newValue;
}

Questions :

The obvious way to fix this would be add synchronized block/method around the if condition. What are other performant way to fix this using concurrent api without using any kind of locks ?

How to get rid of the while loop so we can get the next value with no or less thread contention ?

Constraints :

The next db sequences will be in increasing order not necessarily evenly distributed. So it could be 1, 11, 31 where 21 may be have asked by other node. The requested next value will always be unique. Also need to make sure all the sequences are used and once we reach the max for previous range then only request to db for another starting sequence and so on.

Example :

for db next sequences 1,11,31 with 10 increment, the output next sequence should be 1-10, 11-20, 31-40 for 30 requests.

Answer 1

First of all: I would recommend thinking one more time about using synchronized, because:

1. look at how simple such code is:

    private int maxValue;
    private int currentValue;
   
    public constructor() {
      requestNextValue();
    }
   
    public synchronized int getNextValue() {
      currentValue += 1;
      if (currentValue == maxValue) {
        requestNextValue();
      }
      return currentValue;
    }
   
    private void requestNextValue() {
      currentValue = getFromDb(); //not idempotent
      maxValue = currentValue + 10;
    }
   

2. locks in java actually are pretty intelligent and have pretty good performance.
3. you talk to DB in your code — the performance cost of that alone can be orders of magnitude higher than the performance cost of locks.

But in general, your race conditions happen because you update maxValue and currentValue independently.
You can combine these 2 values into a single immutable object and then work with the object atomically:

private final AtomicReference<State> stateHolder = new AtomicReference<>(newStateFromDb());

public int getNextValue() {
  while (true) {
    State oldState = stateHolder.get();
    State newState = (oldState.currentValue == oldState.maxValue)
        ? newStateFromDb()
        : new State(oldState.currentValue + 1, oldState.maxValue);
    if (stateHolder.compareAndSet(oldState, newState)) {
      return newState.currentValue;
    }
  }
}

private static State newStateFromDb() {
  int newValue = getFromDb(); // not idempotent
  return new State(newValue, newValue + 10);
}


private static class State {

  final int currentValue;
  final int maxValue;

  State(int currentValue, int maxValue) {
    this.currentValue = currentValue;
    this.maxValue = maxValue;
  }
}

After fixing that you will probably have to solve the following problems next:

• how to prevent multiple parallel getFromDb(); (especially after taking into account that the method is idempotent)
• when one thread performs getFromDb();, how to prevent other threads from busy spinning inside while(true) loop and consuming all available cpu time
• more similar problems

Solving each of these problems will probably make your code more and more complicated.

So, IMHO it is almost never worth it — locks work fine and keep the code simple.

Answer 2

You cannot completely avoid locking with the given constraints: since (1) every value returned by getFromDb() must be used and (2) calling getFromDb() is only allowed once maxValue has been reached, you need to ensure mutual exclusion for calls to getFromDb().

Without either of the constraints (1) or (2) you could resort to optimistic locking though:

• Without (1) you could allow multiple threads calling getFromDb() concurrently and choose one of the results dropping all others.

• Without (2) you could allow multiple threads calling getFromDb() concurrently and choose one of the results. The other results would be "saved for later".

Answer 3

The obvious way to fix this would be add synchronized block around the if condition

That is not going to work. Let me try and explain.

When you hit the condition: if(latestValue == maxValue) { ... }, you want to update both maxValue and currentValue atomically. Something like this:

latestValue = getNewValue();
currentValue.set(latestValue);

getNewValue will get your next starting value from the DB and update maxValue, but at the same time, you want to set currentValue to that new starting one now. Suppose the case:

• you first read 1 from the DB. As such maxValue = 11, currentValue = 1.

• when you reach the condition if(latestValue == maxValue), you want to go to the DB to get the new starting position (let's say 21), but at the same time you want every thread to now start from 21. So you must also set currentValue.

Now the problem is that if you write to currentValue under a synchronized block, for example:

if(latestValue == maxValue) {
   synchronized (lock) {
       latestValue = getNewValue();
       currentValue.set(latestValue);
   }
}

you also need to read under the same lock, otherwise you have race. Initially I thought I can be a bit smarter and do something like:

if(latestValue == maxValue) {
    synchronized (lock) {
       if(latestValue == maxValue) {
           latestValue = getNewValue();
           currentValue.set(latestValue);
       } else {
          continue;
       }
    }
}

So that all threads that wait on a lock do not override the previously written value to maxValue when the lock is released. But that still is a race and will cause problems elsewhere, in a different case, rather trivially. For example:

• ThreadA does latestValue = getNewValue();, thus maxValue == 21. Before it does currentValue.set(latestValue);

• ThreadB reads int latestValue = this.currentValue.get();, sees 11 and of course this will be false : if(latestValue == maxValue) {, so it can write 12 (nextValue) to currentValue. Which breaks the entire algorithm.

I do not see any other way then to make getNextValue synchronized or somehow else protected by a mutex/spin-lock.

Answer 4

I don't really see a way around synchonizing the DB call - unless calling the DB multiple times is not an issue (i.e. retrieving several "new values").

To remove the need to synchronize the getNextValue method, you could use a BlockingQueue which will remove the need to atomically update 2 variables. And if you really don't want to use the synchronize keyword, you can use a flag to only let one thread call the DB.

It could look like this (looks ok, but not tested):

private final BlockingQueue<Integer> nextValues = new ArrayBlockingQueue<>(10);
private final AtomicBoolean updating = new AtomicBoolean();

public int getNextValue() {
  while (true) {
    Integer nextValue = nextValues.poll();
    if (nextValue != null) return nextValue;
    else getNewValues();
  }
}

private void getNewValues() {
  if (updating.compareAndSet(false, true)) {
    //we hold the "lock" to run the update
    if (!nextValues.isEmpty()) {
      updating.set(false);
      throw new IllegalStateException("nextValues should be empty here");
    }
    try {
      int newValue = getFromDb(); //not idempotent
      for (int i = 0; i < 10; i++) {
        nextValues.add(newValue + i);
      }
    } finally {
      updating.set(false);
    }
  }
}

But as mentioned in other comments, there is a high chance that the most costly operation here is the DB call, which remains synchronized, so you may as well synchronize everything and keep it simple, with very little difference performance wise.

Answer 5

As getFromDb hits the database you really want some locking - the other threads should block not also go for the database or spin. Really, if you are doing that every 10 iterations, you can probably synchronize the lot. However, that is no fun.

Any reasonable, non-microcontroller platform should support AtomicLong as lock-free. So we can conveniently pack the two ints into one atomic.

private final AtomicLong combinedValue;

public getNextValue() {
    for (;;) {
        long combined = combinedValue.get();
        int latestValue = (int)combined;
        int maxValue = (int)(combined>>32);

        int nextValue = latestValue + 1;

        long nextCombined = (newValue&0xffffffff) | (maxValue<<32)

        if (latestValue == maxValue) { 
            nextValue();
        } else if (currentValue.compareAndSet(combined, nextCombined)) {
            return latestValue;
        }
    }
}

private synchronized void nextValue() {
    // Yup, we need to double check with this locking.
    long combined = combinedValue.get();
    int latestValue = (int)combined;
    int maxValue = (int)(combined>>32);

    if (latestValue == maxValue) {
        int newValue = getFromDb(); //not idempotent
        int maxValue = newValue + 10;

        long nextCombined = (newValue&0xffffffff) | (maxValue<<32)

        combinedValue.set(nextCombined);
    }
}

An alternative with memory allocation would be to lump both values into one object and use AtomicReference. However, we can observe that the value changes more frequently than the maximum, so we can use a slow changing object and a fast offset.

private static record Segment(
    int maxValue, AtomicInteger currentValue
) {
}
private volatile Segment segment;

public getNextValue() {
    for (;;) {
        Segment segment = this.segment;
        int latestValue = segment.currentValue().get();
        int nextValue = latestValue + 1;

        if (latestValue == segment.maxValue()) { 
            nextValue();
        } else if (segment.currentValue().compareAndSet(
            latestValue, nextValue
        )) {
            return latestValue;
        }
    }
}

private synchronized void nextValue() {
    // Yup, we need to double check with this locking.
    Segment segment = this.segment;
    int latestValue = segment.currentValue().get();

    if (latestValue == segment.maxValue()) {
        int newValue = getFromDb(); //not idempotent
        int maxValue = newValue + 10;
        segment = new Segment(maxValue, new AtomicInteger(newValue));
    }
}

(Standard disclaimer: Code not so much as compiled, tested or thought about much. records require a quite new at time of writing JDK. Constructors elided.)

Answer 6

What an interesting question. As others have said you get round with your problem by using synchronized keyword.

public synchronized int getNextValue() { ... }

But because you didn't want to use that keyword and at the same time want to avoid race condition, this probably helps. No guarantee though. And please don't ask for explanations, I'll throw you with OutOfBrainException.

private volatile int maxValue;
private volatile boolean locked = false; //For clarity.
private AtomicInteger currentValue;
    
public int getNextValue() {
    int latestValue = this.currentValue.get();
    int nextValue = latestValue + 1;
        
    if(!locked && latestValue == maxValue) {
        locked = true; //Only one thread per time.
        latestValue = getNewValue();
        currentValue.set(latestValue);
        locked = false;
    }
    while(locked) { latestValue = 0; } //If a thread running in the previous if statement, we need this to buy some time.
    //We also need to reset "latestValue" so that when this thread runs the next loop, 
    //it will guarantee to call AtomicInteger.get() for the updated value.
    while(!currentValue.compareAndSet(latestValue, nextValue)) {
        latestValue = this.currentValue.get();
        nextValue = latestValue + 1;
    }
    return nextValue;
}

Or you can use Atomic to fight Atomic.

private AtomicBoolean locked = new AtomicBoolean(false);
        
public int getNextValue() {
...
if(locked.compareAndSet(false, true)) { //Only one thread per time.
    if(latestValue == maxValue) {
        latestValue = getNewValue();
        currentValue.set(latestValue);
    }
    locked.set(false);
}
...

Answer 7

I can't think of a way to remove all locking since the underlying problem is accessing a mutable value from several threads. However there several improvements that can be done to the code you provided, basically taking advantage of the fact that when data is read by multiple threads, there is no need to lock the reads unless a write has to be done, so using Read/Write locks will reduce the contention. Only 1/10 times there will be a "full" write lock

So the code could be rewritten like this (leaving bugs aside):

import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.locks.ReentrantReadWriteLock;

public class Counter {

    private final ReentrantReadWriteLock reentrantLock = new ReentrantReadWriteLock(true);
    private final ReentrantReadWriteLock.ReadLock readLock = reentrantLock.readLock();
    private final ReentrantReadWriteLock.WriteLock writeLock = reentrantLock.writeLock();
    private AtomicInteger currentValue;
    private AtomicInteger maxValue;

    public Counter() {
        int initialValue = getFromDb();
        this.currentValue = new AtomicInteger(initialValue);
        this.maxValue = new AtomicInteger(initialValue + 10);
    }

    public int getNextValue() {
        readLock.lock();
        while (true){
            int nextValue = currentValue.getAndIncrement();
            if(nextValue<maxValue.get()){
                readLock.unlock();
                return nextValue;
            }
            else {
                readLock.unlock();
                writeLock.lock();
                reload();
                readLock.lock();
                writeLock.unlock();
            }
        }
    }

    private void reload(){
        int newValue = getFromDb();
        if(newValue>maxValue.get()) {
            this.currentValue.set(newValue);
            this.maxValue.set(newValue + 10);
        }
    }

    private int getFromDb(){
        // your implementation
    }

}

Answer 8

What is the business use case you are trying to solve? Can the next scenario work for you:

1. Create SQL sequence (based your database) with counter requirements in the database;
2. Fetch counters from the database as a batch like 50-100 ids
3. Once 50-100 are used on the app level, fetch 100 values more from db ...

?

Answer 9

Slightly modified version of Tom Hawtin - tackline's answer and also the suggestion by codeflush.dev in the comments of the question

Code

I have added a working version of code and simulated a basic multithreaded environment.

Disclaimer: Use with your own discretion

import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Random;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicLong;
import java.util.stream.Collectors;
import java.util.stream.IntStream;

class Seed {
    private static final int MSB = 32;
    private final int start;
    private final int end;
    private final long window;

    public Seed(int start, int end) {
        this.start = start;
        this.end = end;
        this.window = (((long) end) << MSB) | start;
    }

    public Seed(long window) {
        this.start = (int) window;
        this.end = (int) (window >> MSB);
        this.window = window;
    }

    public int getStart() {
        return start;
    }

    public int getEnd() {
        return end;
    }

    public long getWindow() {
        return window;
    }

    // this will not update the state, will only return the computed value
    public long computeNextInWindow() {
        return window + 1;
    }
}

// a mock external seed service to abstract the seed generation and window logic
class SeedService {

    private static final int SEED_INIT = 1;
    private static final AtomicInteger SEED = new AtomicInteger(SEED_INIT);
    private static final int SEQ_LENGTH = 10;

    private static final int JITTER_FACTOR = 5;
    private final boolean canAddRandomJitterToSeed;
    private final Random random;

    public SeedService(boolean canJitterSeed) {
        this.canAddRandomJitterToSeed = canJitterSeed;
        this.random = new Random();
    }

    public int getSeqLengthForTest() {
        return SEQ_LENGTH;
    }

    public Seed getDefaultWindow() {
        return new Seed(1, 1);
    }

    public Seed getNextWindow() {
        int offset = SEQ_LENGTH;

        // trying to simulate multiple machines with interleaved start seed
        if (canAddRandomJitterToSeed) {
            offset += random.nextInt(JITTER_FACTOR) * SEQ_LENGTH;
        }

        final int start = SEED.getAndAdd(offset);
        return new Seed(start, start + SEQ_LENGTH);
    }

    // helper to validate generated ids
    public boolean validate(List<Integer> ids) {
        Collections.sort(ids);
        // unique check
        if (ids.size() != new HashSet<>(ids).size()) {
            return false;
        }

        for (int startIndex = 0; startIndex < ids.size(); startIndex += SEQ_LENGTH) {
            if (!checkSequence(ids, startIndex)) {
                return false;
            }
        }
        return true;
    }

    // checks a sequence
    // relies on 'main' methods usage of SEQ_LENGTH
    protected boolean checkSequence(List<Integer> ids, int startIndex) {
        final int startRange = ids.get(startIndex);
        return IntStream.range(startRange, startRange + SEQ_LENGTH).boxed()
            .collect(Collectors.toList())
            .containsAll(ids.subList(startIndex, startIndex + SEQ_LENGTH));
    }

    public void shutdown() {
        SEED.set(SEED_INIT);
        System.out.println("See you soon!!!");
    }
}

class SequenceGenerator {

    private final SeedService seedService;
    private final AtomicLong currentWindow;

    public SequenceGenerator(SeedService seedService) {
        this.seedService = seedService;

        // initialize currentWindow using seedService
        // best to initialize to an old window so that every instance of SequenceGenerator
        // will lazy load from seedService during the first getNext() call
        currentWindow = new AtomicLong(seedService.getDefaultWindow().getWindow());
    }

    public synchronized boolean requestSeed() {
        Seed seed = new Seed(currentWindow.get());
        if (seed.getStart() == seed.getEnd()) {
            final Seed nextSeed = seedService.getNextWindow();
            currentWindow.set(nextSeed.getWindow());
            return true;
        }
        return false;
    }

    public int getNext() {
        while (true) {
            // get current window
            Seed seed = new Seed(currentWindow.get());

            // exhausted and need to seed again
            if (seed.getStart() == seed.getEnd()) {
                // this will loop at least one more time to return value
                requestSeed();
            } else if (currentWindow.compareAndSet(seed.getWindow(), seed.computeNextInWindow())) {
                // successfully incremented value for next call. so return current value

                return seed.getStart();
            }
        }
    }
}

public class SequenceGeneratorTest {

    public static void test(boolean canJitterSeed) throws Exception {
        // just some random multithreaded invocation

        final int EXECUTOR_THREAD_COUNT = 10;
        final Random random = new Random();
        final int INSTANCES = 500;
        final SeedService seedService = new SeedService(canJitterSeed);
        final int randomRps = 500;
        final int seqLength = seedService.getSeqLengthForTest();

        ExecutorService executorService = Executors.newFixedThreadPool(EXECUTOR_THREAD_COUNT);
        Callable<List<Integer>> callable = () -> {
            final SequenceGenerator generator = new SequenceGenerator(seedService);
            int rps = (1 + random.nextInt(randomRps)) * seqLength;
            return IntStream.range(0, rps).parallel().mapToObj(i -> generator.getNext())
                .collect(Collectors.toList());
        };

        List<Future<List<Integer>>> futures = IntStream.range(0, INSTANCES).parallel()
            .mapToObj(i -> executorService.submit(callable))
            .collect(Collectors.toList());

        List<Integer> ids = new ArrayList<>();
        for (Future<List<Integer>> f : futures) {
            ids.addAll(f.get());
        }

        executorService.shutdown();

        // validate generated ids for correctness
        if (!seedService.validate(ids)) {
            throw new IllegalStateException();
        }

        seedService.shutdown();

        // summary
        System.out.println("count: " + ids.size() + ", unique count: " + new HashSet<>(ids).size());
        Collections.sort(ids);
        System.out.println("min id: " + ids.get(0) + ", max id: " + ids.get(ids.size() - 1));
    }

    public static void main(String[] args) throws Exception {
        test(true);
        System.out.println("Note: ids can be interleaved. if continuous sequence is needed, initialize SeedService with canJitterSeed=false");
        final String ruler = Collections.nCopies( 50, "-" ).stream().collect( Collectors.joining());
        System.out.println(ruler);

        test(false);
        System.out.println("Thank you!!!");
        System.out.println(ruler);
    }
}

Answer 10

Slightly modified version of user15102975's answer with no while-loop and getFromDb() mock impl.

/**
 * Lock free sequence counter implementation
 */
public class LockFreeSequenceCounter {

    private static final int BATCH_SIZE = 10;
    private final AtomicReference<Sequence> currentSequence;
    private final ConcurrentLinkedQueue<Integer> databaseSequenceQueue;

    public LockFreeSequenceCounter() {
        this.currentSequence = new AtomicReference<>(new Sequence(0,0));
        this.databaseSequenceQueue = new ConcurrentLinkedQueue<>();
    }

    /**
     * Get next unique id (threadsafe)
     */
    public int getNextValue() {
        return currentSequence.updateAndGet((old) -> old.next(this)).currentValue;
    }

    /**
     * Immutable class to handle current and max value
     */
    private static final class Sequence {
        private final int currentValue;
        private final int maxValue;

        public Sequence(int currentValue, int maxValue) {
            this.currentValue = currentValue;
            this.maxValue = maxValue;
        }

        public Sequence next(LockFreeSequenceCounter counter){
            return isMaxReached() ? fetchDB(counter) : inc();
        }

        private boolean isMaxReached(){
            return currentValue == maxValue;
        }

        private Sequence inc(){
            return new Sequence(this.currentValue + 1, this.maxValue);
        }

        private Sequence fetchDB(LockFreeSequenceCounter counter){
            counter.databaseSequenceQueue.add(counter.getFromDb());
            int newValue = counter.databaseSequenceQueue.poll();
            int maxValue = newValue + BATCH_SIZE -1;
            return new Sequence(newValue, maxValue);
        }
    }

    /**
     * Get unique id from db (mocked)
     * return on call #1: 1
     * return on call #2: 11
     * return on call #3: 31
     * Note: this function is not idempotent
     */
    private int getFromDb() {
        if (dbSequencer.get() == 21){
            return dbSequencer.addAndGet(BATCH_SIZE);
        } else{
            return dbSequencer.getAndAdd(BATCH_SIZE);
        }
    }
    private final AtomicInteger dbSequencer = new AtomicInteger(1);
}