Est-ce que j’ai un problème de réapprovisionnement et est-ce dû à l’évacuation de la référence?

J’ai cette classe où je cache des instances et les clone (Data est mutable) lorsque je les utilise.

Je me demande si je peux faire face à un problème de réorganisation avec ceci.

J’ai jeté un œil à cette réponse et à JLS mais je ne suis toujours pas confiant.

public class DataWrapper { private static final ConcurrentMap map = new ConcurrentHashMap(); private Data data; private String name; public static DataWrapper getInstance(String name) { DataWrapper instance = map.get(name); if (instance == null) { instance = new DataWrapper(name); } return instance.cloneInstance(); } private DataWrapper(String name) { this.name = name; this.data = loadData(name); // A heavy method map.put(name, this); // I know } private DataWrapper cloneInstance() { return new DataWrapper(this); } private DataWrapper(DataWrapper that) { this.name = that.name; this.data = that.data.cloneInstance(); } } 

Ma pensée: le moteur d’exécution peut réorganiser l’instruction dans le constructeur et publier l’instance actuelle de DataWrapper ( DataWrapper dans la carte) avant d’initialiser l’object de data . Un deuxième thread lit l’instance DataWrapper partir de la carte et voit data champ de data null (ou partiellement construit).

Est-ce possible? Si oui, est-ce uniquement à cause de l’évasion de référence?

Si non, pourriez-vous s’il vous plaît m’expliquer comment raisonner à propos de la cohérence se produit-avant en termes plus simples?

Et si je faisais ça:

 public class DataWrapper { ... public static DataWrapper getInstance(Ssortingng name) { DataWrapper instance = map.get(name); if (instance == null) { instance = new DataWrapper(name); map.put(name, instance); } return instance.cloneInstance(); } private DataWrapper(Ssortingng name) { this.name = name; this.data = loadData(name); // A heavy method } ... } 

Est-il toujours sujet au même problème?

Veuillez noter que cela ne me dérange pas si une ou deux instances supplémentaires sont créées si plusieurs threads tentent de créer et de placer l’instance pour la même valeur au même moment.

MODIFIER:

Et si le nom et les champs de données étaient finaux ou volatiles?

 public class DataWrapper { private static final ConcurrentMap map = new ConcurrentHashMap(); private final Data data; private final String name; ... private DataWrapper(String name) { this.name = name; this.data = loadData(name); // A heavy method map.put(name, this); // I know } ... } 

Est-ce toujours dangereux? Autant que je sache, les garanties de sécurité relatives à l’initialisation du constructeur ne s’appliquent que si la référence ne s’est pas échappée lors de l’initialisation. Je cherche des sources officielles qui le confirment.

Si vous voulez être conforme aux spécifications, vous ne pouvez pas appliquer ce constructeur:

 private DataWrapper(Ssortingng name) { this.name = name; this.data = loadData(name); map.put(name, this); } 

Comme vous le signalez, la machine virtuelle Java est autorisée à réorganiser cette opération de la manière suivante:

 private DataWrapper(Ssortingng name) { map.put(name, this); this.name = name; this.data = loadData(name); } 

Lorsque vous affectez une valeur au champ final , cela implique une action dite de gel à la fin du constructeur. Le modèle de mémoire garantit une relation antérieure entre cette action gel et toute déréglementation de l’instance sur laquelle cette action gel a été appliquée. Cette relation n’existe toutefois qu’à la fin du constructeur, vous la rompez donc. En faisant glisser la publication hors de votre constructeur, vous pouvez corriger ce lien.

Si vous souhaitez un aperçu plus formel de cette relation, je vous recommande de parcourir ce jeu de diapositives . J’ai également expliqué la relation dans cette présentation à partir de la minute 34 environ .

La mise en œuvre présente des mises en garde très subtiles.

Il semble que vous en soyez conscient, mais pour que tout soit bien clair, dans cette partie de code, plusieurs threads peuvent obtenir une instance null et entrer le bloc if , créant inutilement de nouvelles instances de DataWrapper :

 public static DataWrapper getInstance(Ssortingng name) { DataWrapper instance = map.get(name); if (instance == null) { instance = new DataWrapper(name); } return instance.cloneInstance(); } 

Il semble que cela vous loadData(name) , mais cela nécessite l’hypothèse que loadData(name) (utilisé par DataWrapper(Ssortingng) ) renvoie toujours la même valeur. S’il peut renvoyer des valeurs différentes en fonction de la synchronisation, rien ne garantit que le dernier thread chargé de charger les données les stockera dans la map , de sorte que la valeur peut être obsolète. Si vous dites que cela n’arrivera pas ou que ce n’est pas important, c’est bien, mais cette hypothèse devrait au moins être documentée.

Pour illustrer un autre problème subtil, laissez-moi incorporer la méthode instance.cloneInstance() :

 public static DataWrapper getInstance(Ssortingng name) { DataWrapper instance = map.get(name); if (instance == null) { instance = new DataWrapper(name); } return new DataWrapper(instance); } 

La question subtile ici est que cette déclaration de retour n’est pas une publication sûre. La nouvelle instance de DataWrapper peut être partiellement construite et un thread peut l’observer dans un état incohérent, par exemple, les champs de l’object peuvent ne pas être encore définis.

Il existe une solution simple à cela: si vous définissez le name et data champs de data final , la classe devient immuable. Les classes immuables bénéficient de garanties d’initialisation spéciales, et du return new DataWrapper(this); devient publication sécurisée.

Avec ce simple changement, et en supposant que vous soyez d’accord avec le premier point ( loadData n’est pas sensible au temps), je pense que la mise en œuvre devrait fonctionner correctement.


Je recommanderais une amélioration supplémentaire qui n’est pas liée à la correction, mais à d’autres bonnes pratiques. L’implémentation actuelle a trop de responsabilités: c’est un wrapper autour de Data et en même temps un cache. La responsabilité ajoutée rend la lecture un peu déroutante. De plus, la carte de hachage simultanée n’est pas vraiment utilisée à son potentiel.

Si vous séparez les responsabilités, le résultat peut être plus simple, meilleur et plus lisible:

 class DataWrapperCache { private static final ConcurrentMap map = new ConcurrentHashMap<>(); public static DataWrapper get(String name) { return map.computeIfAbsent(name, DataWrapper::new).defensiveCopy(); } } class DataWrapper { private final String name; private final Data data; DataWrapper(String name) { this.name = name; this.data = loadData(name); // A heavy method } private DataWrapper(DataWrapper that) { this.name = that.name; this.data = that.data.cloneInstance(); } public DataWrapper defensiveCopy() { return new DataWrapper(this); } }