2013-10-24 14 views
8

Betrachten Sie den folgenden Code ein:C# Ist diese Methode sicher?

Dictionary<string, string> list = new Dictionary<string, string>(); 
object lockObj = new object(); 

public void MyMethod(string a) { 

    if (list.Contains(a)) 
     return; 

    lock (lockObj) { 
     list.Add(a,"someothervalue"); 
    } 
} 

Ich rufe MyMethod("mystring") von verschiedenen Threads gleichzeitig Unter der Annahme.

Wäre es möglich für mehr als einen Thread (wir nehmen es nur als zwei) geben Sie die if (!list.Contains(a)) Anweisung in der gleichen Zeit (mit ein paar CPU Zyklen Unterschiede), beide Threads werden als false und ein Thread ausgewertet tritt in die kritische Region ein, während eine andere außerhalb gesperrt wird, so dass der zweite Thread eintritt und "mystring" erneut zur Liste hinzufügt, nachdem der erste Thread beendet wurde, was dazu führt, dass das Wörterbuch versucht, einen doppelten Schlüssel hinzuzufügen?

+13

Das Aufrufen eines Wörterbuchs "Liste" ist wirklich gemein. Es entsteht der Eindruck, dass es sich eher um eine Liste als um ein Wörterbuch handelt. – Servy

+0

@Servy Ich nannte meinen Hund "Katze" obwohl. War das auch gemein? – Cruncher

+0

@Cruncher Nein, weil Katzen großartig sind. – Servy

Antwort

18

Nein, es ist nicht threadsicher. Sie benötigen das Schloss auch um die list.Contains, da es möglich ist, dass zwischen dem if-Test und dem Hinzufügen der Daten ein Thread aus- und wieder eingeschaltet wird. Ein anderer Thread kann zwischenzeitlich Daten hinzugefügt haben.

1

Sie müssen die gesamte Aussage sperren. Es ist möglich, dass Sie Probleme auf dem .Contains Teil (die Art, wie Ihr Code ist jetzt) ​​

1

laufen Sie sollten die Liste nach dem Sperren überprüfen. z.B.

if (list.Contains(a)) 
return; 

    lock (lockObj) { 
     if (list.Contains(a)) 
     return; 
     list.Add(a); 
    } 
} 
+3

das Problem damit ist, dass Microsoft nicht garantieren, dass die Methode list.Contains funktionieren wird, wenn ein anderer Thread die Sammlung ändert – user1937198

+0

Alles klar, Punkt: http://StackOverflow.com/Questions/738444/is-listt-contains -a-threadsafe-call-c-shar – BartoszKP

+0

Das scheint nicht 100% sicher.Ein anderer Thread könnte in die Liste schreiben, wenn das Contains() in der gesperrten Region ausgewertet wird. – user1928346

1
private Dictionary<string, string> list = new Dictionary<string, string>(); 

public void MyMethod(string a) { 
    lock (list) { 
     if (list.Contains(a)) 
     return; 
     list.Add(a,"someothervalue"); 
    } 
} 

prüfen diese Anleitung für locking, ist es gut

Einige Richtlinien beachten

  1. sperren um allgemein ein eigenes statisches Objekt zu tragen, wenn auf mehreren beschreibbaren Verriegelungs Werte
  2. Sperren Sie nicht Dinge mit Bereich außerhalb der Klasse oder l ocal Methode wie lock(this), die zu Deadlocks führen könnte!
  3. Sie können das geänderte Objekt sperren, wenn es das einzige Objekt ist, auf das gleichzeitig zugegriffen wird
  4. Stellen Sie sicher, dass das Objekt, das Sie sperren, nicht null ist!
  5. können Sie sperren nur auf Referenztypen
-2

Sie können zunächst eine nicht-Sperrprüfung tun, aber wenn Sie Thread-sicher sein wollen, müssen Sie im Schloss, um die Prüfung erneut wiederholen. Auf diese Weise sperren Sie nicht, es sei denn, Sie müssen die Sicherheit des Fadens gewährleisten.

+1

Dies ist nicht sicher. 'Contains' und' Add' können immer noch gleichzeitig ausgeführt werden. –

+0

Ja, aber verursacht das Inkonsistenzen? –

11

Sie müssen die gesamte Operation sperren (überprüfen und hinzufügen) oder mehrere Threads versuchen, den gleichen Wert hinzuzufügen.

Thread Timeline

ich die ConcurrentDictionary(TKey, TValue) Verwendung würde empfehlen, da es konzipiert ist sicher fädeln.

private readonly ConcurrentDictionary<string, string> _items 
    = new ConcurrentDictionary<string, string>(); 

public void MyMethod(string item, string value) 
{ 
    _items.AddOrUpdate(item, value, (i, v) => value); 
} 
+0

Ich glaube, dein Diagramm ist falsch. Sie müssen die Acquire-Sperre von Thread 2 verschieben, nachdem Thread 1 das Element hinzugefügt hat. –

+1

Ich sollte es formuliert haben, "Versuch, Lock zu erwerben." T1 wird das Schloss erhalten und T2 wird gesperrt, bis T1 das Schloss verlässt. – Romoku

0

Ich gehe davon aus, dass Sie ContainsKey statt Contains schreiben soll. Contains auf einem Dictionary ist explizit implementiert, so dass es nicht über den Typ zugänglich ist, den Sie deklariert haben.

Ihr Code ist nicht sicher. Der Grund dafür ist, dass nichts verhindert ContainsKey und Add zur gleichen Zeit ausgeführt werden kann. Es gibt tatsächlich einige ziemlich bemerkenswerte Fehlerszenarien, die dies einführen würde. Da ich gesehen habe, wie die Dictionary implementiert ist, kann ich sehen, dass Ihr Code eine Situation verursachen könnte, in der Datenstruktur Duplikate enthält. Und ich meine es wörtlich enthält Duplikate. Die Ausnahme wird nicht unbedingt ausgelöst. Die anderen Misserfolgszenarien werden immer seltsamer und seltsamer, aber ich werde hier nicht näher darauf eingehen.

Eine geringfügige Änderung an Ihrem Code könnte eine Variation des doppelt überprüften Sperrmusters beinhalten.

public void MyMethod(string a) 
{ 
    if (!dictionary.ContainsKey(a)) 
    { 
    lock (dictionary) 
    { 
     if (!dictionary.ContainsKey(a)) 
     { 
     dictionary.Add(a, "someothervalue"); 
     } 
    } 
    } 
} 

Dies ist natürlich nicht sicherer aus dem Grund, den ich bereits angegeben habe. Tatsächlich ist es schwierig, das doppelt überprüfte Sperrmuster in allen, außer den einfachsten Fällen (wie die kanonische Implementierung eines Singleton), zu korrigieren. Zu diesem Thema gibt es viele Variationen. Sie können es mit TryGetValue oder dem Standard-Indexer versuchen, aber letztendlich sind alle diese Variationen einfach falsch.

Also wie könnte das richtig gemacht werden, ohne ein Schloss zu nehmen? Sie könnten versuchen ConcurrentDictionary. Es hat die Methode GetOrAdd, die in diesen Szenarien wirklich nützlich ist. Ihr Code würde so aussehen.

public void MyMethod(string a) 
{ 
    // The variable 'dictionary' is a ConcurrentDictionary. 
    dictionary.GetOrAdd(a, "someothervalue"); 
} 

Das ist alles, was es ist. Die GetOrAdd Funktion überprüft, ob das Element vorhanden ist. Wenn nicht, wird es hinzugefügt. Andernfalls wird die Datenstruktur in Ruhe gelassen. Dies alles geschieht auf thread-sichere Weise. In den meisten Fällen macht das ConcurrentDictionary dies, ohne auf ein Schloss zu warten. 2


By the way, Ihr Variablenname ist zu widerlich. Wenn es nicht für Servys Kommentar gewesen wäre, hätte ich vielleicht die Tatsache übersehen, dass wir über eine Dictionary im Gegensatz zu einer List sprachen. In der Tat, basierend auf dem Contains Anruf dachte ich zuerst, wir sprachen über eine List.

Auf den ConcurrentDictionary Leser sind völlig frei sperren. Writer immer nehmen Sie eine Sperre (fügt hinzu und aktualisiert das ist; die Entfernung ist noch frei von Sperren). Dies beinhaltet die GetOrAdd Funktion. Der Unterschied besteht darin, dass die Datenstruktur mehrere mögliche Sperroptionen enthält, sodass in den meisten Fällen keine oder nur geringe Sperrkonflikte auftreten. Aus diesem Grund wird diese Datenstruktur als "Low Lock" oder "Concurrent" im Gegensatz zu "Lock Free" bezeichnet.

+1

Seltsamerweise versucht die Überladung von GetOrAdd, die einen Factory-Delegaten akzeptiert, einen sperrlosen TryGetValue, bevor die Schreibsperre aktiviert wird. Die nicht-delegierte Überladung ist jedoch nicht - also ersteres für High-Concurrency-Szenarien bevorzugen http://blog.basildoncoder.com/2014/01/concurrentdictionary-getoradd-vs.html – Russ

+0

@Russ: Guter Punkt. Ich habe auch andere Merkwürdigkeiten mit dem 'ConcurrentDictionary' bemerkt. Zum Beispiel kann 'AddOrUpdate' [den Factory-Delegierten mehr als einmal ausführen] (http://stackoverflow.com/a/10487983/158779), aber' GetOrAdd' wird nicht. –