Question multi-thread - ajout d'un élément à une liste statique

Question

Bon, débutant question multi-threading:

J'ai une classe Singleton. La classe a une liste statique et fonctionne essentiellement comme ceci:

class MyClass { 
    private static MyClass _instance; 
    private static List<string> _list; 
    private static bool IsRecording; 

    public static void StartRecording() { 
     _list = new List<string>(); 
     IsRecording = true; 
    } 

    public static IEnumerable<string> StopRecording() { 
     IsRecording = false; 
     return new List<string>(_list).AsReadOnly(); 
    } 

    public MyClass GetInstance(){ 

    } 

    public void DoSomething(){ 
     if(IsRecording) _list.Add("Something"); 
    } 
} 

Fondamentalement, un utilisateur peut appeler StartRecording() pour initialiser une liste, puis tous les appels à une instance méthode peut ajouter des choses à la liste. Toutefois, plusieurs threads peuvent contenir une instance à MyClass, de sorte que plusieurs threads peuvent ajouter des entrées à la liste. Cependant, la création et la lecture d'une liste sont des opérations uniques, de sorte que le problème habituel du lecteur-enregistreur dans les situations de multithreading ne s'applique pas. Le seul problème que j'ai pu voir est que l'ordre d'insertion est bizarre, mais ce n'est pas un problème. Puis-je laisser le code tel quel ou dois-je prendre des précautions pour le multi-threading? Je dois ajouter que dans l'application réelle ce n'est pas une liste de chaînes mais une liste d'objets personnalisés (donc le code est _list.Add (new Object (somedata))), mais ces objets contiennent uniquement des données, pas de code en plus d'un appel à DateTime.Now.

Edit: Clarifications suivant quelques réponses: DoSomething ne peut pas être statique (la classe est abrégé ici, il y a beaucoup de choses en cours qui utilise instance des variables, mais celles-ci créées par le constructeur, puis lecture seule) . Est-il assez bon pour faire

lock(_list){ 
    _list.Add(something); 
} 

and 

lock(_list){ 
    return new List<string>(_list).AsReadOnly(); 
} 

ou ai-je besoin de la magie plus profond?

Answer 1

Vous devez certainement verrouiller la _list. Et puisque vous créez plusieurs instances pour _list vous ne pouvez pas verrouiller _list lui-même, mais vous devez utiliser quelque chose comme:

private static object _listLock = new object(); 

En aparté, de suivre quelques bonnes pratiques:

• DoSomething(), comme indiqué, peut être statique et il devrait donc être.

• pour les classes Bibliothèque du motif recommandé est de faire thread-safe membres statiques, qui s'appliquerait à StartRecording(), StopRecording() et DoSomething().

Je également StopRecording() mis _list = null et vérifier pour null dans DoSomething().

Et avant de demander, tout cela prend si peu de temps qu'il n'y a vraiment pas de raisons de performance pas pour le faire.

Answer 2

Il est possible, bien que difficile, d'écrire une liste chaînée qui permet des insertions simultanées à partir de plusieurs threads sans verrou, mais ce n'est pas le cas. Il n'est pas prudent d'appeler _list.Add en parallèle et d'espérer le meilleur. Selon la façon dont il est écrit, vous pourriez perdre une ou deux valeurs, ou corrompre toute la structure. Juste le verrouiller.

Answer 3

Les collections .NET ne sont pas totalement sécurisées pour les threads. De MSDN: "Plusieurs lecteurs peuvent lire la collection en toute confiance, mais toute modification de la collection produit des résultats indéfinis pour tous les threads qui accèdent à la collection, y compris les threads de lecture." Vous pouvez suivre les suggestions sur cette page MSDN pour sécuriser vos accès.

Un problème que vous rencontrerez probablement avec votre code actuel est si StopRecording est appelé alors qu'un autre thread se trouve dans DoSomething. Étant donné que la création d'une liste à partir d'une liste existante nécessite une énumération, vous risquez de rencontrer l'ancien problème "La collection a été modifiée, l'opération d'énumération peut ne pas s'exécuter".

La ligne du bas: pratiquez un filetage sûr!

Answer 4

Vous devez verrouiller la liste si plusieurs threads y ajoutent.

Quelques observations ...

Peut-être qu'il ya une raison de ne pas, mais je suggère faire la classe statique et donc tous ses membres statiques. Il n'y a aucune raison, au moins d'après ce que vous avez montré, d'obliger les clients de MyClass à appeler la méthode GetInstance() juste pour qu'ils puissent appeler une méthode d'instance, DoSomething() dans ce cas.

Je ne vois pas ce qui empêche quelqu'un d'appeler la méthode StartRecording() plusieurs fois. Vous pourriez envisager de mettre un chèque là-bas afin que si vous enregistrez déjà, vous ne créez pas une nouvelle liste, en retirant le tapis de tous les pieds.

Enfin, lorsque vous verrouillez la liste, ne le faites pas comme ceci:

static object _sync = new object(); 
lock(_sync){ 
    _list.Add(new object(somedata)); 
} 

réduire la quantité de temps passé à l'intérieur de la serrure en déplaçant la nouvelle création d'objets à l'extérieur de la serrure.

static object _sync = new object(); 
object data = new object(somedata); 
lock(_sync){ 
    _list.Add(data); 
} 

EDIT

Vous avez dit que DoSomething() ne peut pas être statique, mais je parie qu'il peut. Vous pouvez toujours utiliser un objet de MyClass dans DoSomething() pour tout ce que vous avez à faire. Mais du point de vue de la facilité d'utilisation, ne demandez pas aux utilisateurs de MyClass d'appeler GetInstance() en premier. Considérez ceci:

class MyClass { 
    private static MyClass _instance; 
    private static List<string> _list; 
    private static bool IsRecording; 
    public static void StartRecording() 
    { 
     _list = new List<string>(); 
     IsRecording = true; 
    } 
    public static IEnumerable<string> StopRecording() 
    { 
     IsRecording = false; 
     return new List<string>(_list).AsReadOnly(); 
    } 
    private static MyClass GetInstance() // make this private, not public 
    { return _instance; } 
    public static void DoSomething() 
    { 
     // use inst internally to the function to get access to instance variables 
     MyClass inst = GetInstance(); 
    } 
} 

Faire cela, les utilisateurs de MyClass peut aller de

MyClass.GetInstance().DoSomething(); 

à

MyClass.DoSomething();