CODEWITHSUNDEEP
java
Rajat Gupta
Rajat Gupta

Reputation: 26587

Is this method thread safe?

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

}

Upvotes: 2

Views: 154

Answers (3)

Eugene Retunsky
Eugene Retunsky

Reputation: 13139

You the field noOfUserIdsInReserveCurrently is not accessed from anywhere else, then yes - access to it is thread-safe.

P.S. if fetchIdsInReserve is called only from inside the synchronized block in the getNewId method, then you don't have to make the method synchronized.

UPDATE: as long as the question was edited, now it is not thread-safe. You have to have the return statement in the first method INSIDE the synchronized block. And it doesn't have to be an AtomicInteger, it can be just a simple int in this case.

Upvotes: 1

Mike Samuel
Mike Samuel

Reputation: 120496

No.

If 21 threads comes in here

      synchronized(IdManager.class){
          if (noOfUserIdsInReserveCurrently <= 20)
                  fetchIdsInReserve();    

          noOfUserIdsInReserveCurrently--;
      }

and wait while another 180 threads proceed through the top and through the line below, then by the time the 21st thread reaches the line below, there will be no user ids in reserve when the 21st thread from the first group calls 

      return regstrdUserIdsCount_Cached.incrementAndGet();

EDIT:

Here's the initial state on class load:

regstrdUserIdsCount_Cached = 100
noOfUserIdsInReserveCurrently = 0

Let's assume that the write back to the DB is always successful. If it isn't, this code is clearly broken, because it still allocates an ID in that case.

The first thread comes through, and calls fetch because there are no ids in reserve.

regstrdUserIdsCount_Cached = 100
noOfUserIdsInReserveCurrently = 0

assuming the DB returns 100 as the initial ID, after the method completes without contention

regstrdUserIdsCount_Cached = 101
noOfUserIdsInReserveCurrently = 199

Now, let's assume 178 more threads go through without contention

regstrdUserIdsCount_Cached = 279
noOfUserIdsInReserveCurrently = 21

if that thread is preempted by another that comes through after it exits the synchronized block but before it decrements the atomic int, the preempting thread will trigger a fetch.

Since noOfUserIdsInReserveCurrently has not been decremented by the thread that was pre-empted, 

(noOfUserIdsInReserveCurrently + regstrdUserIdsCount_Cached.get() != reservedInDBTill)

will be false.

Assuming that exception indicates a failure mode, we have a failure during one interleaving that is not thrown during other-interleavings. Therefore, the code is not thread-safe.

The solution is to consistently access regstrdUserIdsCount_Cached inside the critical section. In that case, it need not be an atomic int, but can simply be a non-final int.

Upvotes: 1

Java42
Java42

Reputation: 7706

Here is an example test to show it is thread safe. Replace the AtomicInteger with an int and get rid of the syncs and the test should fail.

public static void main(String... arg) throws Exception {
final int loops = 1042;
for (int l = 0; l != loops; l++) {
  reset(); // add this method to your class to reset the counters
  final int maxThreads = 113;
  final ArrayList<String> checker = new ArrayList<String>();
  final CountDownLatch cl1 = new CountDownLatch(maxThreads);
  final CountDownLatch cl2 = new CountDownLatch(maxThreads);
  for (int x = 0; x != maxThreads; x++) {
    Thread thread = new Thread(new Runnable() {
      public void run() {
        cl1.countDown();
        try {
          cl1.await(); // stack all threads here
        } catch (InterruptedException e) {
          e.printStackTrace();
        }
        int id = getNewId();
        synchronized(checker) {
          checker.add("" + id);
        }
        cl2.countDown();
      }
    });
    thread.start();
  }
  cl2.await();
  for (int x = 0; x != maxThreads; x++) {
    String key = "" + (101 + x); // 1st ID value
    if (!checker.contains(key)) {
      System.out.println("Checker 1 FAIL - missing id=" + key);
    } else {
      checker.remove(key);
      if (checker.contains(key)) {
        System.out.println("Checker 2 FAIL - extra id=" + key);
      }
    }
  }
  for (int x = 0; x != checker.size(); x++) {
    String key = "" + (101 + x);
    System.out.println("Checker 3 FAIL - extra id=" + key);
  }
}
}

Upvotes: 0

Related Questions