Is this method thread safe?

Question

Are these methods getNewId() & fetchIdsInReserve() thread safe ?

public final class IdManager {

    private static final int NO_OF_USERIDS_TO_KEEP_IN_RESERVE = 200;

    private static final AtomicInteger regstrdUserIdsCount_Cached = new AtomicInteger(100);
    private static int noOfUserIdsInReserveCurrently = 0;

    public static int getNewId(){                   
          synchronized(IdManager.class){
              if (noOfUserIdsInReserveCurrently <= 20)
                      fetchIdsInReserve();    

              noOfUserIdsInReserveCurrently--;
          }
          return regstrdUserIdsCount_Cached.incrementAndGet();
    }


    private static synchronized void fetchIdsInReserve(){
        int reservedInDBTill = DBCountersReader.readCounterFromDB(....); // read column from DB 

        if (noOfUserIdsInReserveCurrently + regstrdUserIdsCount_Cached.get() != reservedInDBTill) throw new Exception("Unreserved ids alloted by app before reserving from DB");

        if (DBUpdater.incrementCounter(....)) //if write back to DB is successful
              noOfUserIdsInReserveCurrently += NO_OF_USERIDS_TO_KEEP_IN_RESERVE;
    }

}

Answer 1

You may still have a logic problem. The first time getNewId() is called noOfUserIdsInReserveCurrently = 0 so fetchIdsInReserve() is called and noOfUserIdsInReserveCurrently = 199.

Now imagine the thread gets parked before returning.

Now imagine this happens to 179 more threads. So that we are now at noOfUserIdsInReserveCurrently = 20 and triggering fetchIdsInReserve() again. at this point there is no guarantee that return regstrdUserIdsCount_Cached.incrementAndGet() has ever been called so regstrdUserIdsCount_Cached.get() = 0.

Given this, does the check if (noOfUserIdsInReserveCurrently + regstrdUserIdsCount_Cached.get() != reservedInDBTill) fail?

This is not an issue if there are a fixed number of threads that can call this, like from a FixedThreadPool. But if it is unconstrained then there might be a failure mode.

Answer 2

Imagine this scenario: thread A arrives first and executes the entire synchronized(IdManager.class) block, but is interrupted before the row return regstrdUserIdsCount_Cached.incrementAndGet(); so that the counter is not incremented.

The arrives thread B. It enters the synchronized(IdManager.class) block and gets the same Id as thread A!!!

This could happen because you have put the regstrdUserIdsCount_Cached.incrementAndGet(); outside the syncronized block. If you put it inside, your code should be thread safe.

UPDATE:

The scenario described above is not possibile, because incrementAndGet is an atomic operation. Indeed, the code is thread-safe. My apologies.