Pourquoi ce programme Java se termine-t-il malgré cela, apparemment il ne devrait pas (et ne l'a pas fait)?

205

Aujourd'hui, une opération délicate dans mon laboratoire s'est complètement mal passée. Un actionneur sur un microscope électronique a dépassé ses limites et, après une chaîne d'événements, j'ai perdu 12 millions de dollars d'équipement. J'ai réduit plus de 40 000 lignes dans le module défectueux à ceci:

import java.util.*;

class A {
    static Point currentPos = new Point(1,2);
    static class Point {
        int x;
        int y;
        Point(int x, int y) {
            this.x = x;
            this.y = y;
        }
    }
    public static void main(String[] args) {
        new Thread() {
            void f(Point p) {
                synchronized(this) {}
                if (p.x+1 != p.y) {
                    System.out.println(p.x+" "+p.y);
                    System.exit(1);
                }
            }
            @Override
            public void run() {
                while (currentPos == null);
                while (true)
                    f(currentPos);
            }
        }.start();
        while (true)
            currentPos = new Point(currentPos.x+1, currentPos.y+1);
    }
}

Quelques échantillons de la sortie que je reçois:

$ java A
145281 145282
$ java A
141373 141374
$ java A
49251 49252
$ java A
47007 47008
$ java A
47427 47428
$ java A
154800 154801
$ java A
34822 34823
$ java A
127271 127272
$ java A
63650 63651

Puisqu'il n'y a pas d'arithmétique à virgule flottante ici, et nous savons tous que les entiers signés se comportent bien en cas de débordement en Java, je pense qu'il n'y a rien de mal à ce code. Cependant, malgré la sortie indiquant que le programme n'a pas atteint la condition de sortie, il a atteint la condition de sortie (il était à la fois atteint et non atteint?). Pourquoi?


J'ai remarqué que cela ne se produit pas dans certains environnements. Je suis sur OpenJDK 6 sur Linux 64 bits.

Chien
la source
41
12 millions d'équipements? je suis vraiment curieux de savoir comment cela pourrait arriver ... pourquoi vous utilisez un bloc de synchronisation vide: synchronized (this) {}?
Martin V.
84
Ce n'est même pas à distance thread-safe.
Matt Ball
8
Intéressant à noter: l'ajout du finalqualificatif (qui n'a aucun effet sur le bytecode produit) dans les champs xet y"résout" le bug. Bien que cela n'affecte pas le bytecode, les champs sont marqués avec lui, ce qui m'amène à penser que c'est un effet secondaire d'une optimisation JVM.
Niv Steingarten
9
@Eugene: Cela ne devrait pas se terminer. La question est "pourquoi ça finit?". A Point pest construit qui satisfait p.x+1 == p.y, puis une référence est transmise au thread d'interrogation. Finalement, le thread d'interrogation décide de quitter, car il pense que la condition n'est pas satisfaite pour l'un des Points qu'il reçoit, mais la sortie de la console montre alors qu'il aurait dû être satisfait. L'absence d' volatileici signifie simplement que le fil d'interrogation peut rester bloqué, mais ce n'est clairement pas le problème ici.
Erma K. Pizarro
21
@JohnNicholas: Le vrai code (ce qui n'est manifestement pas le cas) avait une couverture de test à 100% et des milliers de tests, dont beaucoup testaient des choses dans des milliers de commandes et de permutations diverses ... JIT / cache / ordonnanceur. Le vrai problème est que le développeur qui a écrit ce code ne savait pas que la construction ne se produit pas avant d'utiliser l'objet. Remarquez comment la suppression du vide synchronizedempêche le bug de se produire? C'est parce que j'ai dû écrire au hasard du code jusqu'à ce que j'en trouve un qui reproduise ce comportement de manière déterministe.
Chien

Réponses:

140

De toute évidence, l'écriture dans currentPos ne se produit pas avant sa lecture, mais je ne vois pas comment cela peut être le problème.

currentPos = new Point(currentPos.x+1, currentPos.y+1);fait quelques choses, y compris l'écriture de valeurs par défaut dans xet y(0), puis l'écriture de leurs valeurs initiales dans le constructeur. Étant donné que votre objet n'est pas publié en toute sécurité, ces 4 opérations d'écriture peuvent être librement réorganisées par le compilateur / JVM.

Du point de vue du thread de lecture, c'est donc une exécution légale à lire xavec sa nouvelle valeur mais yavec sa valeur par défaut de 0 par exemple. Au moment où vous atteignez l' printlninstruction (qui est d'ailleurs synchronisée et influence donc les opérations de lecture), les variables ont leurs valeurs initiales et le programme imprime les valeurs attendues.

Marquer currentPoscomme volatilegarantira une publication sûre puisque votre objet est effectivement immuable - si dans votre cas d'utilisation réel l'objet est muté après la construction, les volatilegaranties ne seront pas suffisantes et vous pourriez voir à nouveau un objet incohérent.

Alternativement, vous pouvez rendre l' Pointimmuable qui assurera également une publication sûre, même sans utilisation volatile. Pour atteindre l'immuabilité, il vous suffit de marquer xet de yterminer.

En remarque et comme déjà mentionné, synchronized(this) {}peut être traité comme un no-op par la JVM (je comprends que vous l'avez inclus pour reproduire le comportement).

assylias
la source
4
Je ne suis pas sûr, mais faire final x et y n'aurait pas le même effet, en évitant la barrière de la mémoire?
Michael Böckling
3
Une conception plus simple est un objet ponctuel immuable qui teste les invariants sur la construction. Vous ne risquez donc jamais de publier une configuration dangereuse.
Ron
@BuddyCasino Oui en effet - je l'ai ajouté. Pour être honnête, je ne me souviens pas de toute la discussion d'il y a 3 mois (l'utilisation de final a été proposée dans les commentaires, donc je ne sais pas pourquoi je ne l'ai pas incluse en option).
assylias
2
L'immutabilité en soi ne garantit pas une publication sûre (si x an y était privé mais exposé uniquement avec des getters, le même problème de publication existerait toujours). final ou volatile le garantit. Je préfère le final au volatile.
Steve Kuo
@SteveKuo Immutability nécessite final - sans final, le meilleur que vous pouvez obtenir est une immuabilité efficace qui n'a pas la même sémantique.
assylias
29

Comme il currentPosest modifié en dehors du fil, il doit être marqué comme volatile:

static volatile Point currentPos = new Point(1,2);

Sans volatilité, le thread n'est pas garanti de lire les mises à jour de currentPos qui sont effectuées dans le thread principal. Par conséquent, de nouvelles valeurs continuent d'être écrites pour currentPos, mais le thread continue d'utiliser les versions mises en cache précédentes pour des raisons de performances. Étant donné qu'un seul thread modifie currentPos, vous pouvez vous en tirer sans verrous, ce qui améliorera les performances.

Les résultats sont très différents si vous ne lisez les valeurs qu'une seule fois dans le thread pour les utiliser dans la comparaison et leur affichage ultérieur. Lorsque je fais ce qui suit xs'affiche toujours en tant que 1et yvarie entre 0et un grand entier. Je pense que le comportement de celui-ci à ce stade est quelque peu indéfini sans le volatilemot - clé et il est possible que la compilation JIT du code y contribue. De plus, si je commente le synchronized(this) {}bloc vide , le code fonctionne également et je soupçonne que c'est parce que le verrouillage provoque un retard suffisant currentPoset que ses champs sont relus plutôt qu'utilisés à partir du cache.

int x = p.x + 1;
int y = p.y;

if (x != y) {
    System.out.println(x+" "+y);
    System.exit(1);
}
Ed Plese
la source
2
Oui, et je pourrais aussi mettre un verrou sur tout. À quoi veux-tu en venir?
Chien
J'ai ajouté quelques explications supplémentaires pour l'utilisation de volatile.
Ed Plese
19

Vous avez une mémoire ordinaire, la référence 'currentpos' et l'objet Point et ses champs derrière elle, partagés entre 2 threads, sans synchronisation. Ainsi, il n'y a pas d'ordre défini entre les écritures qui arrivent à cette mémoire dans le thread principal et les lectures dans le thread créé (appelez-le T).

Le thread principal effectue les écritures suivantes (en ignorant la configuration initiale de point, px et py auront des valeurs par défaut):

  • à px
  • à py
  • à currentpos

Parce qu'il n'y a rien de spécial à propos de ces écritures en termes de synchronisation / barrières, le runtime est libre pour permettre au thread T de les voir se produire dans n'importe quel ordre (le thread principal voit bien sûr toujours les écritures et les lectures ordonnées selon l'ordre du programme), et se produit à tout moment entre les lectures de T.

Alors T fait:

  1. lit currentpos en p
  2. lire px et py (dans l'un ou l'autre ordre)
  3. comparer et prendre la branche
  4. lire px et py (dans les deux cas) et appeler System.out.println

Étant donné qu'il n'y a pas de relations de classement entre les écritures dans main et les lectures dans T, il existe clairement plusieurs façons de produire votre résultat, car T peut voir l'écriture de main dans currentpos avant les écritures dans currentpos.y ou currentpos.x:

  1. Il lit currentpos.x en premier, avant que l'écriture x ne se produise - obtient 0, puis lit currentpos.y avant que l'écriture y se produise - obtient 0. Compare evals à true. Les écritures deviennent visibles pour T. System.out.println est appelé.
  2. Il lit currentpos.x en premier, après que l'écriture x s'est produite, puis lit currentpos.y avant que l'écriture y se soit produite - obtient 0. Compare evals à true. Les écritures deviennent visibles pour T ... etc.
  3. Il lit currentpos.y en premier, avant que l'écriture y n'ait eu lieu (0), puis lit currentpos.x après l'écriture x, ce qui est vrai. etc.

et ainsi de suite ... Il y a un certain nombre de courses de données ici.

Je soupçonne l'hypothèse erronée ici de penser que les écritures qui résultent de cette ligne sont rendues visibles sur tous les threads dans l'ordre du programme du thread qui l'exécute:

currentPos = new Point(currentPos.x+1, currentPos.y+1);

Java n'offre aucune garantie (ce serait terrible pour les performances). Quelque chose de plus doit être ajouté si votre programme a besoin d'un ordre garanti des écritures par rapport aux lectures dans d'autres threads. D'autres ont suggéré de rendre les champs x, y définitifs ou de rendre volatils les courants.

  • Si vous définissez les champs x, y comme définitifs, Java garantit que l'écriture de leurs valeurs se produira avant le retour du constructeur, dans tous les threads. Ainsi, comme l'affectation à currentpos se fait après le constructeur, le thread T est garanti de voir les écritures dans le bon ordre.
  • Si vous rendez la position actuelle volatile, Java garantit qu'il s'agit d'un point de synchronisation qui sera ordonné au total par rapport aux autres points de synchronisation. Comme en général, les écritures sur x et y doivent se produire avant l'écriture dans currentpos, alors toute lecture de currentpos dans un autre thread doit également voir les écritures de x, y qui se sont produites auparavant.

L'utilisation de final présente l'avantage de rendre les champs immuables et permet ainsi la mise en cache des valeurs. L'utilisation de données volatiles entraîne une synchronisation à chaque écriture et lecture des positions actuelles, ce qui peut nuire aux performances.

Voir le chapitre 17 de la spécification du langage Java pour les détails sanglants: http://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html

(La réponse initiale supposait un modèle de mémoire plus faible, car je n'étais pas sûr que la volatilité garantie par JLS était suffisante. ).

paulj
la source
2
C'est la meilleure explication à mon avis. Merci beaucoup!
skyde
1
@skyde mais faux sur la sémantique de volatile. volatile garantit que les lectures d'une variable volatile verront la dernière écriture disponible d'une variable volatile ainsi que toute écriture précédente . Dans ce cas, s'il currentPosest rendu volatil, l'affectation garantit la publication sûre de l' currentPosobjet ainsi que de ses membres, même s'ils ne sont pas volatils eux-mêmes.
assylias
Eh bien, je disais que je ne pouvais pas, pour moi, voir exactement comment le JLS garantissait que le volatile formait une barrière avec les autres lectures et écritures normales. Techniquement, je ne peux pas me tromper là-dessus;). En ce qui concerne les modèles de mémoire, il est prudent de supposer qu'une commande n'est pas garantie et d'avoir tort (vous êtes toujours en sécurité) que l'inverse et d'être faux et dangereux. C'est génial si volatile offre cette garantie. Pouvez-vous expliquer comment le ch 17 du JLS le fournit?
paulj
2
En bref, dans Point currentPos = new Point(x, y), vous avez 3 écritures: (w1) this.x = x, (w2)this.y = y et (w3) currentPos = the new point. L'ordre du programme garantit que hb (w1, w3) et hb (w2, w3). Plus tard dans le programme, vous lisez (r1) currentPos. S'il currentPosn'est pas volatil, il n'y a pas de hb entre r1 et w1, w2, w3, donc r1 pourrait en observer une (ou aucune). Avec volatile, vous introduisez hb (w3, r1). Et la relation hb étant transitive, vous introduisez également hb (w1, r1) et hb (w2, r1). Ceci est résumé dans Java Concurrency in Practice (3.5.3. Safe Publication Idioms).
assylias
2
Ah, si hb est transitif de cette façon, alors c'est une «barrière» assez forte, oui. Je dois dire qu'il n'est pas facile de déterminer que 17.4.5 du JLS définit hb pour avoir cette propriété. Ce n'est certainement pas dans la liste des propriétés donnée vers le début du 17.4.5. La fermeture transitive n'est mentionnée plus loin qu'après quelques notes explicatives! Quoi qu'il en soit, bon à savoir, merci pour la réponse! :). Remarque: je mettrai à jour ma réponse pour refléter le commentaire d'assylias.
paulj
-2

Vous pouvez utiliser un objet pour synchroniser les écritures et les lectures. Sinon, comme d'autres l'ont déjà dit, une écriture dans currentPos se produira au milieu des deux lectures p.x + 1 et py

new Thread() {
    void f(Point p) {
        if (p.x+1 != p.y) {
            System.out.println(p.x+" "+p.y);
            System.exit(1);
        }
    }
    @Override
    public void run() {
        while (currentPos == null);
        while (true)
            f(currentPos);
    }
}.start();
Object sem = new Object();
while (true) {
    synchronized(sem) {
        currentPos = new Point(currentPos.x+1, currentPos.y+1);
    }
}
Germano Fronza
la source
En fait, cela fait le travail. Dans ma première tentative, j'ai mis la lecture à l'intérieur du bloc synchronisé, mais plus tard, j'ai réalisé que ce n'était pas vraiment nécessaire.
Germano Fronza
1
-1 La JVM peut prouver qu'elle semn'est pas partagée et traiter l'instruction synchronisée comme un no-op ... Le fait qu'elle résout le problème est une pure chance.
assylias
4
Je déteste la programmation multi-thread, beaucoup de choses fonctionnent à cause de la chance.
Jonathan Allen
-3

Vous accédez à currentPos deux fois et ne garantissez pas qu'il n'est pas mis à jour entre ces deux accès.

Par exemple:

  1. x = 10, y = 11
  2. le thread de travail évalue px comme 10
  3. le thread principal exécute la mise à jour, maintenant x = 11 et y = 12
  4. thread de travail évalue py comme 12
  5. le thread de travail remarque que 10 + 1! = 12, imprime et quitte donc.

Vous comparez essentiellement deux différents points .

Notez que même rendre currentPos volatile ne vous protégera pas de cela, car il s'agit de deux lectures distinctes par le thread de travail.

Ajouter un

boolean IsValid() { return x+1 == y; }

à votre classe de points. Cela garantira qu'une seule valeur de currentPos est utilisée lors de la vérification de x + 1 == y.

user2686913
la source
currentPos n'est lu qu'une seule fois, sa valeur est copiée dans p. p est lu deux fois, mais il va toujours pointer au même endroit.
Jonathan Allen