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.