Ist diese Methode sicher?

Question

Sind diese Methoden getNewId() & fetchIdsInReserve() fadensicher?

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

No.

Wenn 21 Fäden

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

      noOfUserIdsInReserveCurrently--; 
     } 

hier kommt und warten, während weitere 180 Fäden durch den oberen und durch die Leitung unten, dann durch die Zeit ablaufen erreicht der Faden 21 die Zeile unter keine Benutzer-IDs in Reserve sein, wird es, wenn der 21. Faden aus der ersten Gruppe

 return regstrdUserIdsCount_Cached.incrementAndGet(); 

EDIT ruft:

Hier ist der Ausgangszustand auf Klasse Last:

regstrdUserIdsCount_Cached = 100 
noOfUserIdsInReserveCurrently = 0 

annehmen lassen, dass das Schreiben in die DB zurück ist immer erfolgreich. Wenn dies nicht der Fall ist, ist dieser Code eindeutig gebrochen, da er in diesem Fall immer noch eine ID zuweist.

Der erste Thread kommt durch und ruft Fetch auf, da keine IDs in Reserve sind.

regstrdUserIdsCount_Cached = 100 
noOfUserIdsInReserveCurrently = 0 

die DB unter der Annahme liefert 100 als anfängliche ID, nach dem Verfahren ohne Streit beendet

regstrdUserIdsCount_Cached = 101 
noOfUserIdsInReserveCurrently = 199 

Jetzt wollen wir 178 mehr Threads ohne Konkurrenz durchnehmen

regstrdUserIdsCount_Cached = 279 
noOfUserIdsInReserveCurrently = 21 

wenn das Thread wird von einem anderen verdrängt, der durchkommt, nachdem er den synchronized Block verlassen hat, aber bevor er das atomare int, das preempting thre, dekrementiert Die Anzeige löst einen Abruf aus.

Da noOfUserIdsInReserveCurrently nicht durch den Faden verringert worden ist, die vorbelegten war,

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

wird falsch sein.

Angenommen, die Ausnahme weist auf einen Fehlermodus hin, haben wir während eines Interleaving einen Fehler, der bei anderen Interleavings nicht ausgelöst wird. Daher ist der Code nicht Thread-sicher.

Die Lösung besteht darin, innerhalb des kritischen Abschnitts konsistent auf regstrdUserIdsCount_Cached zuzugreifen.In diesem Fall muss es kein atomarer int sein, sondern kann einfach ein nicht-intakter sein.

Answer 2

Sie das Feld noOfUserIdsInReserveCurrently nicht von anderswo zugegriffen wird, dann ja - Zugriff darauf ist Thread-sicher.

P.S. Wenn fetchIdsInReserve nur innerhalb des synchronisierten Blocks in der getNewId-Methode aufgerufen wird, müssen Sie die Methode nicht synchronisieren.

UPDATE: Solange die Frage bearbeitet wurde, ist es jetzt nicht Thread-sicher. Sie müssen die return-Anweisung in der ersten Methode INSIDE des synchronisierten Blocks haben. Und es muss kein AtomicInteger sein, es kann in diesem Fall einfach ein einfacher int sein.

Answer 3

Hier ist ein Beispieltest, um zu zeigen, dass es threadsicher ist. Ersetzen Sie den AtomicInteger mit einem int und entfernen Sie die Synchronisierungen und der Test sollte fehlschlagen.

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