Warum ist lock(this) {...} schlecht?

Question

Die MSDN-Dokumentation besagt, dass

public class SomeObject
{
  public void SomeOperation()
  {
    lock(this)
    {
      //Zugriff auf Instanzvariablen
    }
  }
}

ein "Problem darstellt, wenn die Instanz öffentlich zugänglich ist". Ich frage mich warum? Liegt es daran, dass das Schloss länger als nötig gehalten wird? Oder gibt es einen noch hinterhältigeren Grund?

Answer 1

Es ist schlechte Form, dies in Sperranweisungen zu verwenden, da im Allgemeinen nicht unter Ihrer Kontrolle liegt, wer sonst noch auf dieses Objekt sperrt.

Um parallele Operationen ordentlich zu planen, sollte besondere Sorgfalt darauf verwendet werden, mögliche Deadlock-Situationen in Betracht zu ziehen, und eine unbekannte Anzahl von Sperr-Einstiegspunkten hindert dieses Vorhaben. Zum Beispiel kann jeder mit einer Referenz auf das Objekt darauf sperren, ohne dass der Objektdesigner/-ersteller darüber Bescheid weiß. Dies erhöht die Komplexität von Mehr-Thread-Lösungen und könnte ihre Richtigkeit beeinträchtigen.

Ein privates Feld ist normalerweise eine bessere Option, da der Compiler den Zugriff darauf einschränken wird und er den Sperrmechanismus umschließen wird. Das Verwenden von dies verletzt die Umschließung, indem ein Teil Ihrer Sperrimplementierung der Öffentlichkeit enthüllt wird. Es ist auch nicht klar, dass Sie eine Sperre auf dies erhalten werden, es sei denn, es wurde dokumentiert. Selbst dann ist es suboptimal, sich auf Dokumentation zu verlassen, um ein Problem zu verhindern.

Zuletzt gibt es die falsche Annahme, dass lock(this) tatsächlich das übergebene Objekt verändert und es auf irgendeine Weise schreibgeschützt oder unzugänglich macht. Dies ist falsch. Das übergebene Objekt dient lediglich als Schlüssel. Wenn bereits eine Sperre auf diesem Schlüssel liegt, kann die Sperre nicht erfolgen; andernfalls ist die Sperre zulässig.

Deshalb ist es schlecht, Strings als Schlüssel in lock-Anweisungen zu verwenden, da sie unveränderlich sind und über Teile der Anwendung hinweg geteilt/zugänglich sind. Verwenden Sie stattdessen eine private Variable, eine Object-Instanz wird hierfür gut geeignet sein.

Führen Sie den folgenden C#-Code als Beispiel aus.

public class Person
{
    public int Age { get; set;  }
    public string Name { get; set; }

    public void LockThis()
    {
        lock (this)
        {
            System.Threading.Thread.Sleep(10000);
        }
    }
}

class Program
{
    static void Main(string[] args)
    {
        var nancy = new Person {Name = "Nancy Drew", Age = 15};
        var a = new Thread(nancy.LockThis);
        a.Start();
        var b = new Thread(Timewarp);
        b.Start(nancy);
        Thread.Sleep(10);
        var anotherNancy = new Person { Name = "Nancy Drew", Age = 50 };
        var c = new Thread(NameChange);
        c.Start(anotherNancy);
        a.Join();
        Console.ReadLine();
    }

    static void Timewarp(object subject)
    {
        var person = subject as Person;
        if (person == null) throw new ArgumentNullException("subject");
        // A lock does not make the object read-only.
        lock (person.Name)
        {
            while (person.Age <= 23)
            {
                // There will be a lock on 'person' due to the LockThis method running in another thread
                if (Monitor.TryEnter(person, 10) == false)
                {
                    Console.WriteLine("'this' person is locked!");
                }
                else Monitor.Exit(person);
                person.Age++;
                if(person.Age == 18)
                {
                    // Changing the 'person.Name' value doesn't change the lock...
                    person.Name = "Nancy Smith";
                }
                Console.WriteLine("{0} is {1} years old.", person.Name, person.Age);
            }
        }
    }

    static void NameChange(object subject)
    {
        var person = subject as Person;
        if (person == null) throw new ArgumentNullException("subject");
        // You should avoid locking on strings, since they are immutable.
        if (Monitor.TryEnter(person.Name, 30) == false)
        {
            Console.WriteLine("Failed to obtain lock on 50 year old Nancy, because Timewarp(object) locked on string \"Nancy Drew\".");
        }
        else Monitor.Exit(person.Name);

        if (Monitor.TryEnter("Nancy Drew", 30) == false)
        {
            Console.WriteLine("Failed to obtain lock using 'Nancy Drew' literal, locked by 'person.Name' since both are the same object thanks to inlining!");
        }
        else Monitor.Exit("Nancy Drew");
        if (Monitor.TryEnter(person.Name, 10000))
        {
            string oldName = person.Name;
            person.Name = "Nancy Callahan";
            Console.WriteLine("Name changed from '{0}' to '{1}'.", oldName, person.Name);
        }
        else Monitor.Exit(person.Name);
    }
}

Konsolenausgabe

'this' person is locked!
Nancy Drew is 16 years old.
'this' person is locked!
Nancy Drew is 17 years old.
Failed to obtain lock on 50 year old Nancy, because Timewarp(object) locked on string "Nancy Drew".
'this' person is locked!
Nancy Smith is 18 years old.
'this' person is locked!
Nancy Smith is 19 years old.
'this' person is locked!
Nancy Smith is 20 years old.
Failed to obtain lock using 'Nancy Drew' literal, locked by 'person.Name' since both are the same object thanks to inlining!
'this' person is locked!
Nancy Smith is 21 years old.
'this' person is locked!
Nancy Smith is 22 years old.
'this' person is locked!
Nancy Smith is 23 years old.
'this' person is locked!
Nancy Smith is 24 years old.
Name changed from 'Nancy Drew' to 'Nancy Callahan'.

Answer 2

Denn wenn Leute auf Ihre Objektinstanz zugreifen können (d. h. auf Ihr this-Zeiger), können sie auch versuchen, dasselbe Objekt zu sperren. Möglicherweise sind sie sich nicht bewusst, dass Sie intern auf this sperren, was zu Problemen führen kann (möglicherweise zu einem Deadlock).

Zusätzlich dazu ist es auch eine schlechte Praxis, weil es "zu viel" sperrt.

Zum Beispiel könnten Sie eine Member-Variable von List haben, und das Einzige, was Sie tatsächlich sperren müssen, ist diese Member-Variable. Wenn Sie das gesamte Objekt in Ihren Funktionen sperren, werden andere Funktionen, die diese Funktionen aufrufen, blockiert, während sie auf die Sperre warten. Wenn diese Funktionen nicht auf die Member-Liste zugreifen müssen, werden Sie anderen Code zum Warten zwingen und Ihre Anwendung ohne ersichtlichen Grund verlangsamen.

Answer 3

Werfen Sie einen Blick auf das MSDN-Thema Thread-Synchronisierung (C#-Programmierhandbuch)

Im Allgemeinen ist es am besten, das Sperren auf einen öffentlichen Typ oder auf Objektinstanzen zu vermeiden, die außerhalb der Kontrolle Ihrer Anwendung liegen. Beispielsweise kann lock(this) problematisch sein, wenn die Instanz öffentlich zugänglich ist, da Code außerhalb Ihrer Kontrolle ebenfalls auf das Objekt gesperrt werden kann. Dies könnte Deadlock-Situationen erzeugen, bei denen zwei oder mehr Threads auf die Freigabe desselben Objekts warten. Das Sperren auf einen öffentlichen Datentyp anstelle eines Objekts kann aus dem gleichen Grund Probleme verursachen. Das Sperren auf Literalzeichenfolgen birgt besonders Risiken, da Literalzeichenfolgen vom Common Language Runtime (CLR) interniert sind. Dies bedeutet, dass es eine Instanz jeder gegebenen Literalzeichenfolge für das gesamte Programm gibt, dass das exakt gleiche Objekt die Zeichenfolge in allen laufenden Anwendungsdomänen auf allen Threads darstellt. Als Ergebnis sperrt ein Sperren auf eine Zeichenfolge mit dem gleichen Inhalt an beliebiger Stelle im Anwendungsprozess alle Instanzen dieser Zeichenfolge in der Anwendung. Daher ist es am besten, ein privates oder geschütztes Element zu sperren, das nicht interniert ist. Einige Klassen stellen speziell für das Sperren Mitglieder zur Verfügung. Der Array-Typ bietet beispielsweise SyncRoot. Viele Sammlungstypen bieten ebenfalls ein SyncRoot-Mitglied.

Answer 4

Ich weiß, dass dies ein alter Thread ist, aber da Menschen dies immer noch nachschauen und darauf verlassen können, scheint es wichtig zu erwähnen, dass lock(typeof(SomeObject)) wesentlich schlechter ist als lock(this). Nichtsdestotrotz ein großes Lob an Alan, der darauf hingewiesen hat, dass lock(typeof(SomeObject)) eine schlechte Praxis ist.

Ein System.Type-Exemplar ist eines der generischsten, grobkörnigsten Objekte überhaupt. Mindestens ist ein System.Type-Exemplar global für eine AppDomain und .NET kann mehrere Programme in einer AppDomain ausführen. Das bedeutet, dass zwei vollständig unterschiedliche Anwendungen potenziell gegenseitige Störungen verursachen könnten, selbst bis hin zu einem Deadlock, wenn sie beide versuchen, eine Synchronisierungssperre auf demselben globalen System.Type-Exemplar zu erhalten.

Deshalb ist lock(this) keine besonders robuste Form, kann Probleme verursachen und sollte aus all den genannten Gründen immer Skepsis hervorrufen. Trotzdem gibt es weit verbreiteten, relativ gut respektierten und anscheinend stabilen Code wie log4net, der das lock(this)-Muster umfangreich nutzt, auch wenn ich persönlich lieber eine Änderung dieses Musters sehen würde.

Aber lock(typeof(SomeObject)) öffnet eine ganz neue und erweiterte Büchse der Pandora.

Für was es wert ist.

Answer 5

...und die genauen gleichen Argumente gelten auch für dieses Konstrukt:

lock(typeof(SomeObject))