Discussion :

[Chatanga]

J'ai récemment appris que les constructeurs n'étaient pas thread-safe en Java, alors que j'avais toujours cru le contraire, notamment parce qu'on ne peut pas les « synchronized ». Pour ceux qui ne verraient pas de quoi je parle, voici un extrait du livre Java Concurrency in Practice (très bon livre au passage) :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
class Holder {
 
	private int n;
 
	public Holder(int n) {
		this.n = n;
	}
 
	public void assertSanity() {
		if (n != n) {
			throw new AssertionError("This statement is false.");
		}
	}
}

Si la méthode assertSanity est appelée dans un autre thread que celui ayant créé l'instance, l'exception AssertionError peut très bien être levée. Par exemple :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
private static class HolderThread implements Runnable {
 
	// Le final importe !
	private final Holder holder;
 
	public HolderThread(Holder holder) {
		this.holder = holder;
	}
 
	@Override
	public void run() {
		holder.assertSanity();
	}
}
 
// Code susceptible de lever une AssertionError.
new Thread(new HolderThread(new Holder(10))).start();

Ça ne se produira pas forcément sur toutes les JVM, mais ça pourra au moins se produire sur certaines. Je précise au passage que la JSR 133 relative à la révision du modèle mémoire de Java ne change pas ce comportement. Si j'ai bien compris, pour résoudre le problème il faudrait adopter l'une des approches suivantes :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
public class TestThreadSafety {
 
	public interface Holder {
 
		public void assertSanity();
	}
 
	@NotThreadSafe
	public static class MutableHolder implements Holder {
 
		private int n;
 
		public MutableHolder(int n) {
			this.n = n;
		}
 
		@Override
		public void assertSanity() {
			if (n != n) {
				throw new AssertionError("This statement is false.");
			}
		}
	}
 
	@ThreadSafe
	public static class FinalHolder implements Holder {
 
		private final int n;
 
		public FinalHolder(int n) {
			this.n = n;
		}
 
		@Override
		public void assertSanity() {
			if (n != n) {
				throw new AssertionError("This statement is false.");
			}
		}
	}
 
	@ThreadSafe
	public static class SynchronizedSafeHolder implements Holder {
 
		private int n;
 
		public SynchronizedSafeHolder(int n) {
			synchronized (this) {
				this.n = n;
			}
		}
 
		@Override
		public synchronized void assertSanity() {
			if (n != n) {
				throw new AssertionError("This statement is false.");
			}
		}
	}
 
	@ThreadSafe
	public static class VolatileSafeHolder implements Holder {
 
		private volatile int n; // atomique
 
		public VolatileSafeHolder(int n) {
			synchronized (this) {
				this.n = n;
			}
		}
 
		@Override
		public synchronized void assertSanity() {
			if (n != n) {
				throw new AssertionError("This statement is false.");
			}
		}
	}
 
	@ThreadSafe
	public static class AtomicSafeHolder implements Holder {
 
		// Le final importe !
		private final AtomicInteger n = new AtomicInteger(10);
 
		public AtomicSafeHolder(int n) {
			this.n.set(n);
		}
 
		@Override
		public synchronized void assertSanity() {
			if (n.get() != n.get()) {
				throw new AssertionError("This statement is false.");
			}
		}
	}
 
	private static class HolderThread implements Runnable {
 
		// Le final importe !
		private final Holder holder;
 
		// Holder doit être thread-safe, même s'il est "donné".
		public HolderThread(Holder holder) {
			this.holder = holder;
		}
 
		@Override
		public void run() {
			holder.assertSanity();
		}
	}
 
	private static class HolderThread2 implements Runnable {
 
		// Le final importe !
		private final SynchronousQueue<Holder> rdv = new SynchronousQueue<Holder>();
 
		// Holder n'a pas besoin d'être thread-safe s'il est "donné".
		public HolderThread2(Holder holder) {
			rdv.offer(holder);
		}
 
		@Override
		public void run() {
			try {
				Holder holder = rdv.take();
				holder.assertSanity();
			} catch (InterruptedException e) {
				Thread.currentThread().interrupt();
			}
		}
	}
 
	public void testIt() {
		// Code susceptible de lever une AssertionError.
		new Thread(new HolderThread(new MutableHolder(10))).start();
 
		// Code correct.
		new Thread(new HolderThread(new FinalHolder(10))).start();
		new Thread(new HolderThread(new SynchronizedSafeHolder(10))).start();
		new Thread(new HolderThread(new VolatileSafeHolder(10))).start();
		new Thread(new HolderThread(new AtomicSafeHolder(10))).start();
 
		// Code correct.
		new Thread(new HolderThread2(new MutableHolder(10))).start();
	}
}

N'ayant jamais structuré mon code de cette manière, j'imagine que la quasi-totalité du code multi-threads que j’écris depuis 10 ans est buggé ! En fait, j'ai bien l'impression que le code thread-safe est plus l'exception que la règle, notamment dans le Javadoc de Sun. Si on prend l'exemple suivant :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
class PrimeRun implements Runnable {
 
	long minPrime;
 
	PrimeRun(long minPrime) {
		this.minPrime = minPrime;
	}
 
	public void run() {
		// compute primes larger than minPrime
	}
}

C’est incorrect, non ? Il faudrait que minPrime soit final. En fait, même le code de la classe Thread me semble suspect, puisque le constructeur Thread(Runnable target) stocke target dans une variable non final et l'utilise dans run() sans synchronisation particulière ! Du coup, je ne sais plus quoi penser...

[thelvin]

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
// Code susceptible de lever une AssertionError.
new Thread(new HolderThread(new Holder(10))).start();

Absolument pas. L'AssertionError est impossible.

En effet, la méthode start() du Thread est appelée sur le même thread que le constructeur du Thread.
Par conséquent, l'exécution complète du constructeur du Thread happens-before l'appel de la méthode start(), et donc happens-before le début de l'exécution du thread.
Or, l'objet Thread n'est 100% construit qu'après que l'objet HolderThread soit 100% construit, ce qui n'arrivera qu'après que l'objet Holder soit 100% construit, ce qui n'arrivera qu'après que le membre n ait pris sa valeur correcte.

Par contre, il pourrait y avoir une erreur dans ce cas-là :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
public class NoThreadSafety {
 
  private static Holder holder = null;
 
  public static void buildAndCheck() {
    if(holder == null) {
      holder = new Holder(10);
    }
    holder.assertSanity();
  }
 
  public static void main(String... args) {
    Runnable runnable = new Runnable() {
      public void run() {
        buildAndCheck();
      }
    }
 
    // AssertionError possible
    new Thread(runnable).start();
    new Thread(runnable).start();
  }
 
}

Le fameux lazy-loading : ne créer l'objet Holder que quand on en a besoin, s'il est null.
Sauf que si deux threads appellent buildAndCheck() en même temps, le premier thread peut avoir modifié l'objet Holder de sorte qu'il ne soit pas null, avant d'avoir modifié la valeur de n. C'est du moins possible du point de vue de l'autre thread : aucune relation happens-before ne l'empêche. Et dans ce cas-là, bam ! AssertionError.

Ça peut donc arriver, mais pas dans le cas trivial que tu cites. Si un Thread est construit par le même Thread qui le démarre (ce qui est tout de même préférable !) alors sa construction happens-before son exécution.

[Chatanga]

Or, l'objet Thread n'est 100% construit qu'après que l'objet HolderThread soit 100% construit, ce qui n'arrivera qu'après que l'objet Holder soit 100% construit

Il existe vraiment une telle garantie (en dehors des champs final) ? Même si la méthode start utilisait explicitement le contenu de Holder, je ne vois pas ce qui forcerait le compilateur à vider son cache en l’absence de synchronisation.

[Jacobian]

je ne vois pas l'utilité qu'un constructeur soit thread safe ?

[thelvin]

Il existe vraiment une telle garantie (en dehors des champs final) ? Même si la méthode start utilisait explicitement le contenu de Holder, je ne vois pas ce qui forcerait le compilateur à vider son cache en l’absence de synchronisation.

Quand un thread crée un autre thread, il lui fournit sa mémoire dans le même état qu'il l'a lui-même, que ce soit à coup de cache ou pas : dans ce cas les caches sont dupliqués et fournis avec. Le modèle de données, que ce soit ancien ou nouveau, de Java, le décrit ainsi.
C'est après le démarrage de l'autre thread, que la vue de la mémoire suit son propre chemin pour chacun.

je ne vois pas l'utilité qu'un constructeur soit thread safe ?

C'est plutôt que ça ne veut pas dire grand-chose, "un constructeur thread-safe". Une méthode est thread-safe quand elle peut être appelée par plusieurs threads sans synchronisation, un objet est thread-safe quand toutes ses méthodes sont thread-safe. Or avec la syntaxe Java, il n'existe aucun moyen d'envoyer plus d'un thread sur un constructeur.

Mais l'intérêt de "synchronized" autour d'un constructeur, on l'a montré dans nos exemples. On pourrait éviter l'AssertionError de mon exemple en modifiant Holder comme ça :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
public Holder(int n) {
  synchronized(this) {
    this.n = n;
  }
}
 
public synchronized void assertSanity() {
  if (n != n) {
    throw new AssertionError("This statement is false.");
  }
}

Synchroniser l'accès à n avec le mot-clé synchronized. On ne peut pas mettre synchronized à un constructeur, du coup si on veut le faire quand même il faut mettre un bloc synchronized(this) dans ce constructeur.

Bien sûr, il vaut mieux synchroniser le code extérieur de sorte qu'on ne cherche pas à utiliser un objet avant qu'il soit entièrement construit.

Edit : En me relisant, je constate qu'un test

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

if(n != n)

devrait toujours renvoyer false sauf à la rigueur si n est volatile (et encore, il faudrait que je vérifie : c'est une seule et même opération d'évaluation, là, je ne sais pas si le compilateur estime qu'il doit bidouiller les caches pour aller chercher deux fois la même chose pour la même opération).

Je pensais plutôt à un test genre

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

if(n != 10)

ou

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

if(n != m)

avec n et m initialisés dans le constructeur, à la même valeur.

[Chatanga]

En fouillant un peu, je suis tombé là-dessus à propos du JMM :

Thread.start() requires barriers ensuring that the started thread sees all stores visible to the caller at the call point. Conversely, Thread.join() requires barriers ensuring that the caller sees all stores by the terminating thread. These are normally generated by the synchronization entailed in implementations of these constructs.

En clair, les méthodes start/join on une sémantique particulière dans le modèle mémoire Java et correspondent à des points de synchronisation. Inutile de le faire explicitement donc. Ça colle donc avec ce que disait thelvin dans sa seconde réponse. Par contre, le problème de la synchronisation des constructeurs reste entier. En fouillant encore sur le net, je suis tombé sur une discussion intéressante qui conclut à un défaut de Java. Pour n’en citer qu’un message :

Generally speaking the thread-safety of a class pertains to the use of an
instance of that class once it has been shared between threads. The sharing,
in general, requires safe-publication, which is something normally under the
control of the code doing the sharing not the code of the object that was
shared. Sometime a class needs to add some protection against
unsafe-publication that might violate important semantic guarantees of the
object - like sharing Strings in a way that might make the string appear to
have different values hence String uses final fields and we have the JMM's
final field semantics.

It is rare that you need to synchronize a constructor, or that having a
synchronized constructor addresses your needs. But in the case where it does
then you can use a synchronized block within the constructor.

I would not be surprised that when the prohibition against synchronizing a
constructor was created, visibility and safe-publication were not
considerations.

La dernière phrase me semble importante car beaucoup de développeurs ne savent pas qu’un synchronized ne se cantonne pas à garantir qu'un seul thread exécute un bloc synchronized à un moment donné, mais garantit aussi qu'un thread exécutant un bloc synchronized(machin) puisse voir les modifications faite par le passé par d'autres threads dans des blocs synchronized(machin). Les constructeurs ont la particularité de ne pas avoir besoin du premier point. Le point emmerdant, c’est qu’il n’y a pas de moyens de garantir leur initialisation correcte. Utiliser un bloc synchronized interne (qui ne pourra inclure le constructeur du parent au demeurant), ne garantira pas pour autant sa publication après ce bloc (par contre, s’il est vu initialisé, il le sera vu complètement). La seule solution semble de passer par une synchronisation externe.

[thelvin]

La dernière phrase me semble importante car beaucoup de développeurs ne savent pas qu’un synchronized ne se cantonne pas à garantir qu'un seul thread exécute un bloc synchronized à un moment donné, mais garantit aussi qu'un thread exécutant un bloc synchronized(machin) puisse voir les modifications faite par le passé par d'autres threads dans des blocs synchronized(machin).

Euh... Le mot-clé en question c'est synchronized. Il s'appelle comme ça, il s'écrit comme ça. C'est comme le port-salut, c'est marqué dessus. Il sert à synchroniser.
On commence tous débutants bien sûr, mais ça on peut rien y faire.

Les constructeurs ont la particularité de ne pas avoir besoin du premier point.

Chuis pas d'accord, mon exemple prouve le contraire. Au lieu d'être deux threads dans le même constructeur, c'est un thread dans le constructeur et un autre dans une méthode de l'objet.

De toute façon, on ne peut synchroniser l'état des variables, que si on n'accède pas tous en même temps à ces variables. Les deux notions vont ensemble.

Le point emmerdant, c’est qu’il n’y a pas de moyens de garantir leur initialisation correcte. Utiliser un bloc synchronized interne (qui ne pourra inclure le constructeur du parent au demeurant), ne garantira pas pour autant sa publication après ce bloc (par contre, s’il est vu initialisé, il le sera vu complètement). La seule solution semble de passer par une synchronisation externe.

J'ai du mal à croire que ça puisse être embêtant. Je touche assez souvent à du multithreading (mais pas à du calcul parallèle complexe, c'est vrai,) et j'ai mis des années à me rendre compte qu'on ne peut pas mettre synchronized à un constructeur. Et le jour où je m'en suis rendu compte, je n'aurais pas dû essayer de toute façon.

[tchize_]

Je crois que tu confond pas mal par rapport à l'exemple et l'histoire du constructeur.

Est-ce que ton exemple peux "pêter" -> non, jamais. Start ne pourra jamasi être appelé avant que le constructeur aie fini puisque les deux tournent sur le même thread.

Est-ce qu'on peux voir en java le contenu d'un nouvel objet depuis un autre Thread *avant* d'avoir fini la construction? Malheureusement oui, sur certains modèles mémoire, il me semble. C'est d'ailleurs un des problèmes qui est au coeur des difficultés à créer un classe singleton

Mais, dans ce dernier cas, la cause n'est pas le constructeur en lui même mais le fait que java peux faire en réalité, en arrière scène, cette transformation:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

variable = new Variable();

->

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
variable = allocation d'un objet d'un type précis
variable.appel du constructeur

alors qu'on s'attendrais à ce qu'il fasse

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
registre = allocation d'un objet d'un type précis
registre.appel du constructeur
variable = registre

et que le code qui appelle ce constructeur rende "variable" visible sans en synchroniser correctement la gestion. Exemple typique qui pourrais poser problème avec ton holder:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
public Holder getHolder() {return h;}
 
public void newHolder(int i) {h=new holder(i);}

Solution (arrêtez moi si je dit des conneries hein):

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
public Holder getHolder() {return h;}
public void newHolder(int i) {Holder h=new holder(i);
this.h=h;}

[thelvin]

Solution (arrêtez moi si je dit des conneries hein):

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
public Holder getHolder() {return h;}
public void newHolder(int i) {Holder h=new holder(i);
this.h=h;}

Bon ben tu es en état d'arrestation.
Passer par une variable locale intermédiaire et superflue Holder h ne change absolument rien à rien. En fait, du point de vue du compilateur, ce bout de code est exactement le même que le bout de code précédent.

Avec les nouveaux modèles de données, une solution serait plutôt de rendre la variable membre h volatile : cela oblige à ce que tous les threads voient les accès en lecture/écriture de la variable, de la même manière. Mais seulement dans le nouveau modèle.
Dans les anciens modèles il y a d'autres techniques, mais je préfère juste un synchronized, c'est plus simple et pas spécialement moins rapide.

[tchize_]

Il me semblait que dans le cas des données du constructeur, justement, le seul problème c'est qu'on "stocke" l'objet avant d'appeler son constructeur. Hors passer par une variable intermédiaire empecherais cela (mais il est vrais que JIT pourrais "simplifier" ce bout de code :/). Il ne me semble cependant que le volatile n'y changerais rien. Au contraire, l'objet non initialisé serait visible d'autant plus vite par les autres thread qu'il n'y a plus de cache local. A moins que volatile interdise le stockage avant l'appel au constructeur?

Mais bon, c'est vrai que quand on fais du multithread, on utilise synchronized pour éviter les ennuis

[thelvin]

Il me semblait que dans le cas des données du constructeur, justement, le seul problème c'est qu'on "stocke" l'objet avant d'appeler son constructeur. Hors passer par une variable intermédiaire empecherais cela (mais il est vrais que JIT pourrais "simplifier" ce bout de code :/).

Je n'aurai pas souvent l'occasion de te dire cela : ça me semble un peu naïf.

On "réserve la mémoire" d'un objet avant d'appeler son constructeur, certes, pas trop le choix (et ça fait d'ailleurs des trucs très funs avec les valeurs que les variables membres ont à ce moment-là.) Mais la question est de savoir quand une référence vers cet objet est poussée, et où, pour donner l'accès à cet objet.
En principe la référence est censée être assignée après que l'objet ait été construit bien sûr. En réalité s'il n'y a pas de synchronisation,
- ça peut être fait dans l'ordre que ça lui chante, du moment que pour le thread en cours ça produise les mêmes résultats
- ça sera poussé dans n'importe quel ordre dans la mémoire partagée (ça c'est pas que Java, c'est aussi l'OS, les processeurs, les caches de mémoire centrale...)

Et insérer une variable locale ne change rien à aucun de ces deux aspects : elle ne modifie rien des résultats du thread en cours, et n'a pas le pouvoir de changer quoi que ce soit à l'organisation des caches.
Logiquement donc, le compilateur ne devrait pas la garder, et même s'il le fait, ça ne fera qu'ajouter quelques instructions touchant à la pile, qui ne changent rien à l'affaire.

Après, je ne sais absolument pas s'il existe un quelconque problème de ce genre sur les machines x64 sous Windows ou linux, par exemple. Je serais surpris qu'il n'y en ait pas, mais voilà.

Il ne me semble cependant que le volatile n'y changerais rien. Au contraire, l'objet non initialisé serait visible d'autant plus vite par les autres thread qu'il n'y a plus de cache local. A moins que volatile interdise le stockage avant l'appel au constructeur?

Entre autres.

Dans le nouveau modèle, un accès volatile est défini comme une forme de synchronisation. Ce qui est écrit comme arrivant avant ne peut donc pas être déplacé pour arriver après, ni l'inverse, et les caches sont synchronisés à chaque fois.

Ça ne résout pas tout, notamment le lazy-loading de singleton reste assez compliqué comparé à un simple synchronized. Mais c'est déjà ça.

Mais bon, c'est vrai que quand on fais du multithread, on utilise synchronized pour éviter les ennuis

Après, le problème c'est les performances. Le calcul parallèle est censé éviter de synchroniser à tout va, mais ce n'est pas dans ce domaine que je fais du multithread.