Aide à la révision du code suivant, est-il sûr pour les threads?

Question

private static Callback callback;

public Foo()
{
    super(getCallback());
}

private static Callback getCallback()
{
    callback = new Callback();
    return callback;
}

Le constructeur Foo() peut potentiellement être appelé à partir de plusieurs threads. Mon inquiétude concerne le champ statique privé 'callback' et la méthode statique 'getCallback()'.

Comme on peut le voir, à chaque fois que 'getCallback()' est appelé, il assigne une nouvelle valeur au champ statique 'callback'.

Je pense que ce n'est pas thread-safe car le mot-clé static est toujours attaché à la classe et non à l'instance, ce qui signifie que le champ statique 'callback' d'un Foo peut potentiellement être écrasé par un autre thread qui construit un autre Foo(). Est-ce correct?

Veuillez me corriger si je me trompe. Merci!

EDIT: Mon intention est de conserver 'callback' quelque part dans la classe, afin de pouvoir le réutiliser plus tard. Mais ce n'est pas facile car Foo étend une classe dont le constructeur exige que 'callback' soit passé.

Answer 1

Oui, vous avez raison. Il est possible que deux instances de Foo se retrouvent avec la même instance de CallBack lorsque deux threads entrent simultanément dans la méthode getCallback() et que l'un assigne un nouveau CallBack au champ statique tandis que l'autre a déjà fait cela mais n'a pas encore retourné. Dans ce cas, la meilleure solution est de ne pas avoir le champ statique, car il ne sert à rien. En alternative, rendre getCallback() synchronisé.

Mais notez qu'il n'est pas vrai que seul le mot-clé static entraîne un code non threadsafe.

Answer 2

Ce n'est pas thread-safe. Essayez ces alternatives :

Option 1 : ici, toutes les instances partagent le même rappel (callback)

private static final Callback callback = new Callback();

public Foo() {
    super(callback);
}

Option 2 : ici, chaque instance a son propre rappel (callback)

public Foo() {
    super(new Callback());
}

Notez que, dans les deux cas, bien que le constructeur soit thread-safe, la sécurité des threads de l'ensemble de la classe dépend de l'implémentation de Callback. S'il a un état mutable, alors vous aurez des problèmes potentiels. Si Callback est immuable, alors vous avez la thread-safety.

Answer 3

Le rappel obtiendra une nouvelle valeur chaque fois que Foo() est appelé (même depuis le même thread). Je ne suis pas tout à fait sûr de ce que devrait faire votre code (si vous souhaitez initialiser la variable statique une seule fois (singleton), vous devriez vérifier si elle est toujours nulle dans getCallback() - et qu'est-ce que actionCallback?). Pour le rendre thread-safe, utilisez synchronized.

Answer 4

Je pense que tu l'as parfaitement résumé toi-même, mais sans plus de détails sur ce que tu essaies d'accomplir, il sera difficile de formuler des suggestions pour résoudre ton problème.

Une question évidente est, est-ce que callback doit être statique? Ou pourrais-tu sûrement le transformer en un champ d'instance sans compromettre la fonctionnalité de ta classe?

Answer 5

Je sais que la question a déjà été résolue mais le pourquoi n'a pas vraiment été détaillé.

Si deux threads appellent la méthode getCallback(), ils pourraient exécuter les lignes comme suit:

1. Thread 1 - callback = new Callback();
2. Thread 2 - callback = new Callback();
3. Thread 1 - return actionCallback;
4. Thread 2 - return actionCallback;

Dans ce cas, le callback généré à (2.) serait retourné à la fois dans (3.) et (4.)

La solution semblerait être de se demander pourquoi le callback est défini de manière statique s'il est spécifique à l'instance et non à la classe.

J'espère que cela aide.