Comportement inattendu dans concurrency (Java)

Question

J'ai écrit la source la plus simple de reproduire le problème comme suit

package concurrency.test; 

import java.util.concurrent.Executors; 
import java.util.concurrent.TimeUnit; 
import java.util.concurrent.atomic.AtomicLong; 

public class Main 
{ 
    public static void main(String[] args) 
    { 
     new Main().start(); 
    } 

    private void start() 
    { 
     for (int i = 0; i < 20; i++) 
     { 
      Executors.newSingleThreadScheduledExecutor().scheduleAtFixedRate(new SequencePrinter(), 1, 1, TimeUnit.SECONDS); 
     } 
    } 

    private class SequencePrinter implements Runnable 
    { 
     @Override 
     public void run() 
     { 
      System.out.println(IdGenerator.instance().nextId()); 
     } 
    } 

    private static class IdGenerator 
    { 
     private static IdGenerator instance; 
     private final AtomicLong idSequence = new AtomicLong(0); 

     private IdGenerator() 
     { 
     } 

     public static IdGenerator instance() 
     { 
      if (instance == null) 
      { 
       instance = new IdGenerator(); 
      } 

      return instance; 
     } 

     synchronized public long nextId() 
     { 
      return idSequence.incrementAndGet(); 
     } 
    } 
} 

Ce que je pense: Identifiants uniques dans aucun ordre

Ce que j'ai trouvé: Plusieurs 1s (tout autre nombre est unique mais pas '1')

Il semble que je n'ai pas compris certains concepts de base de la concurrence. Pouvez-vous me dire ce que je fais mal?

Answer 1

La méthode instance() de votre classe IdGenerator n'est pas thread-safe, si plusieurs instances de cette classe peuvent être créés, chacun avec leur propre variable membre idSequence, lorsque plusieurs threads appellent la méthode simultanément.

Vous devrez utiliser la méthode instance() pour les threads, par exemple synchronized.

Answer 2

Quel est le point de synchronisation de l'incrément synchronisé? La prochaine fois, toujours, utilisez static Type singlenote = new ... pour créer des singletones.

import java.util.concurrent.Executors; 
import java.util.concurrent.TimeUnit; 
import java.util.concurrent.atomic.AtomicLong; 

public class SequencePrinter implements Runnable { 

    public void run() { 
     System.out.println("produces " + idSequence.incrementAndGet()); 
    } 

    public static void main(String[] args) { 

     for (int i = 0; i < 200; i++) 
      Executors.newSingleThreadScheduledExecutor().scheduleAtFixedRate(
       new SequencePrinter(), 1, 10, TimeUnit.SECONDS); 
    } 

    private static final AtomicLong idSequence = new AtomicLong(0); 

} 

Le code devient beaucoup plus simple et est éliminé. Peut-être voudriez-vous créer une instance dans le getNext et c'est pourquoi vous l'avez synthétisé? Ne créez pas des tonnes de méthodes getter inutiles.