1
\$\begingroup\$

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;
    }

}
\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thankyou so much! To solve this issue, should I include regstrdUserIdsCount_Cached inside the synchronized block & make it as a non final int instead of atomicInteger ? \$\endgroup\$ – Rajat Gupta Apr 3 '12 at 19:48
  • \$\begingroup\$ To be honest I'm not sure what you are trying to accomplish. But synchronizing the getNewId() method would make the whole system safe. \$\endgroup\$ – Clint Apr 4 '12 at 3:36
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Wanted to add that you might as well synchronize the method. When you synchronize(IdManager.class), you prevent any other thread from accessing any other synchronized static methods for that class. \$\endgroup\$ – Apprentice Queue Apr 1 '12 at 20:55
  • \$\begingroup\$ I disagree with your reasoning, @vitalik. The decision on what ID to return to a thread is based solely on the result of regstrdUserIdsCount_Cached.incrementAndGet() - this is not influenced by the reservation logic, so the code is in fact threadsafe. \$\endgroup\$ – sgmorrison Apr 1 '12 at 21:02
  • \$\begingroup\$ I guess you are right @sgmorrison. I corrected my answer. \$\endgroup\$ – Виталий Олегович Apr 1 '12 at 21:08
  • \$\begingroup\$ @ApprenticeQueue: In that case, since I am calling a synchronized method(fetchIdsInReserve()) from a synchronized block that could lead to a deadlock, right? \$\endgroup\$ – Rajat Gupta Apr 2 '12 at 5:29
  • \$\begingroup\$ @Raj no deadlock because Java uses reentrant synchronization; if you have the lock already, you don't need to acquire the same lock again. \$\endgroup\$ – Apprentice Queue Apr 2 '12 at 16:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.