Affectation d'un objet à un champ défini en dehors d'un bloc synchronisé - est-il sûr pour les threads?

Question

Y a-t-il quelque chose qui cloche avec la sécurité du thread de ce code java? Les threads 1-10 ajoutent des nombres via sample.add(), et Threads 11-20 appellent removeAndDouble() et impriment les résultats sur stdout. Je me rappelle du fond de mon esprit que quelqu'un a dit que l'assignation d'un item de la même manière que dans removeAndDouble() en l'utilisant en dehors du bloc synchronisé peut ne pas être thread safe. Que le compilateur peut optimiser les instructions afin qu'elles se produisent hors séquence. est-ce le cas ici? Ma méthode removeAndDouble() est-elle dangereuse?

Y a-t-il autre chose qui ne va pas dans une perspective de concurrence avec ce code? J'essaie d'avoir une meilleure compréhension de la concurrence et du modèle de mémoire avec Java (1.6 vers le haut).

import java.util.*; 
import java.util.concurrent.*; 

public class Sample { 

    private final List<Integer> list = new ArrayList<Integer>(); 

    public void add(Integer o) { 
     synchronized (list) { 
      list.add(o); 
      list.notify(); 
     } 
    } 

    public void waitUntilEmpty() { 
     synchronized (list) { 
      while (!list.isEmpty()) { 
       try { 
        list.wait(10000); 
       } catch (InterruptedException ex) { } 
      } 
     } 
    } 

    public void waitUntilNotEmpty() { 
     synchronized (list) { 
      while (list.isEmpty()) { 
       try { 
        list.wait(10000); 
       } catch (InterruptedException ex) { } 
      } 
     } 
    } 

    public Integer removeAndDouble() { 
     // item declared outside synchronized block 
     Integer item; 
     synchronized (list) { 
      waitUntilNotEmpty(); 
      item = list.remove(0); 
     } 
     // Would this ever be anything but that from list.remove(0)? 
     return Integer.valueOf(item.intValue() * 2); 
    } 

    public static void main(String[] args) { 
     final Sample sample = new Sample(); 

     for (int i = 0; i < 10; i++) { 
      Thread t = new Thread() { 
       public void run() { 
        while (true) { 
         System.out.println(getName()+" Found: " + sample.removeAndDouble()); 
        } 
       } 
      }; 
      t.setName("Consumer-"+i); 
      t.setDaemon(true); 
      t.start(); 
     } 

     final ExecutorService producers = Executors.newFixedThreadPool(10); 
     for (int i = 0; i < 10; i++) { 
      final int j = i * 10000; 
      Thread t = new Thread() { 
       public void run() { 
        for (int c = 0; c < 1000; c++) { 
         sample.add(j + c); 
        } 
       } 
      }; 
      t.setName("Producer-"+i); 
      t.setDaemon(false); 
      producers.execute(t); 
     } 

     producers.shutdown(); 
     try { 
      producers.awaitTermination(600, TimeUnit.SECONDS); 
     } catch (InterruptedException e) { 
      e.printStackTrace(); 
     } 

     sample.waitUntilEmpty();   
     System.out.println("Done."); 
    } 
} 

Answer 1

Votre synchronisation est correcte et n'entraînera aucun problème d'exécution dans le désordre.

Cependant, je remarque quelques problèmes.

D'abord, votre méthode waitUntilEmpty serait beaucoup plus en temps opportun si vous ajoutez un list.notifyAll() après la list.remove(0) en removeAndDouble. Cela permettra d'éliminer un délai allant jusqu'à 10 secondes dans votre wait(10000).

Deuxièmement, votre list.notify en devrait être un notifyAll, parce notify ne se réveille que un fil, et il peut se réveiller un thread qui attend à l'intérieur waitUntilEmpty au lieu de waitUntilNotEmpty. Troisièmement, rien de ce qui précède est terminal à la vivacité de votre application, car vous avez utilisé des attentes bornées, mais si vous faites les deux changements ci-dessus, votre application aura de meilleures performances threadées (waitUntilEmpty) et les attentes bornées deviennent inutiles et peuvent devenir de vieilles attentes sans argot.

Answer 2

Cela semble sûr pour moi. Voici mon raisonnement.

Chaque fois que vous accédez à list, vous le synchronisez. C'est bien. Même si vous retirez une partie du list dans item, item n'est pas accessible par plusieurs threads.

Tant que vous ne avez accès list tout synchronisé, vous devriez être bon (dans votre conception actuelle.)

Answer 3

Votre code tel quel est en fil de fait sûr. Le raisonnement derrière ceci est en deux parties.

La première est l'exclusion mutuelle. Votre synchronisation garantit correctement qu'un seul thread à la fois va modifier les collections.

La seconde concerne votre préoccupation concernant le réordonnancement du compilateur. Vous vous êtes inquiété de ce que la compilation puisse en fait réorganiser l'assignation dans laquelle elle ne serait pas sécurisée. Vous n'avez pas à vous en soucier dans ce cas. La synchronisation sur la liste crée une relation arrive-avant. Tout supprime de la liste arrive avant l'écriture à Integer item. Cela indique au compilateur qu'il ne peut pas réorganiser l'écriture à l'élément dans cette méthode.

Answer 4

Votre code est thread-safe, mais pas simultané (comme en parallèle). Comme tout est accessible sous un seul verrou d'exclusion mutuelle, vous numérotez tous les accès, en effet l'accès à la structure est monothread.

Si vous avez besoin de la fonctionnalité décrite dans votre code de production, le package java.util.concurrent fournit déjà une implémentation BlockingQueue avec une baie (taille fixe) et des implémentations basées sur une liste chaînée (évolutive). Ceux-ci sont très intéressants à étudier pour les idées de mise en œuvre à tout le moins.