Question Pourquoi est-ce que lock (this) {...} est mauvais?


le Documentation MSDN dit ça

public class SomeObject
{
  public void SomeOperation()
  {
    lock(this)
    {
      //Access instance variables
    }
  }
}

est "un problème si l'instance peut être consultée publiquement". Je me demande pourquoi? Est-ce parce que le verrou sera tenu plus longtemps que nécessaire? Ou existe-t-il une raison plus insidieuse?


415
2017-10-30 19:30


origine


Réponses:


C'est une mauvaise forme à utiliser this dans les instructions de verrouillage, car il est généralement hors de votre contrôle qui d'autre pourrait se verrouiller sur cet objet.

Afin de planifier correctement les opérations en parallèle, il faut veiller tout particulièrement à prendre en compte les situations de blocage possibles, et le fait d'avoir un nombre inconnu de points d'entrée de verrouillage le gêne. Par exemple, n'importe qui avec une référence à l'objet peut le verrouiller sans que le concepteur / créateur de l'objet le sache. Cela augmente la complexité des solutions multithread et peut affecter leur exactitude.

Un champ privé est généralement une meilleure option car le compilateur lui appliquera des restrictions d'accès et encapsulera le mécanisme de verrouillage. En utilisant this viole l'encapsulation en exposant une partie de votre implémentation de verrouillage au public. Il n'est pas clair non plus que vous allez acquérir un verrou sur this sauf si cela a été documenté. Même dans ce cas, le recours à la documentation pour éviter un problème est sous-optimal.

Enfin, il y a l'idée fausse commune que lock(this) modifie réellement l'objet passé en paramètre et le rend en quelque sorte en lecture seule ou inaccessible. C'est faux. L'objet transmis en paramètre à lock sert simplement de clé. Si un verrou est déjà maintenu sur cette touche, le verrouillage ne peut pas être effectué; sinon, le verrou est autorisé.

C'est pourquoi il est mauvais d'utiliser les cordes comme clés lock déclarations, car elles sont immuables et sont partagées / accessibles entre les différentes parties de l'application. Vous devriez utiliser une variable privée à la place, Object instance va bien faire.

Exécutez le code C # suivant à titre d'exemple.

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);
    }
}

Sortie de la console

'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'.

449



Parce que si les gens peuvent accéder à votre instance d'objet (ex: votre this), ils peuvent aussi essayer de verrouiller le même objet. Maintenant, ils ne sont peut-être pas au courant que vous vous verrouillez this en interne, cela peut causer des problèmes (éventuellement un blocage)

En plus de cela, c'est aussi une mauvaise pratique, car il verrouille "trop"

Par exemple, vous pouvez avoir une variable membre de List<int>, et la seule chose que vous avez réellement besoin de verrouiller est cette variable membre. Si vous verrouillez l'objet entier dans vos fonctions, alors tout ce qui appelle ces fonctions sera bloqué dans l'attente du verrouillage. Si ces fonctions n'ont pas besoin d'accéder à la liste des membres, vous allez provoquer l'attente de l'autre code et ralentir votre application sans aucune raison.


59



Jetez un coup d'œil sur le sujet MSDN Thread Synchronization (Guide de programmation C #)

Généralement, il vaut mieux éviter le verrouillage   sur un type public, ou sur un objet   instances au-delà du contrôle de votre   application. Par exemple, verrouiller (ceci)   peut être problématique si l'instance peut   être accessible publiquement, car le code   au-delà de votre contrôle peut verrouiller le   objet aussi bien. Cela pourrait créer   situations d'impasse où deux ou plusieurs   threads attendent la publication de la   même objet. Verrouiller un public   type de données, par opposition à un objet,   peut causer des problèmes pour le même   raison. Le verrouillage des chaînes littérales est   particulièrement risqué parce que littéral   les chaînes sont internées par le commun   langue d'exécution (CLR). Ça signifie   qu'il y a une instance de   chaîne littérale donnée pour l'ensemble   programme, exactement le même objet   représente le littéral dans tous les courir   domaines d'application, sur tous les threads.   En conséquence, un verrou placé sur une chaîne   avec le même contenu partout dans le   processus de demande verrouille tous   instances de cette chaîne dans le   application. En conséquence, il vaut mieux   verrouiller un membre privé ou protégé   ce n'est pas interné. Quelques classes   fournir des membres spécifiquement pour   verrouillage. Le type de tableau, par exemple,   fournit SyncRoot. Beaucoup collection   types fournissent un membre SyncRoot comme   bien.


39



Je sais que c'est un vieux fil de discussion, mais parce que les gens peuvent toujours regarder ça et s'en remettre, il semble important de souligner que lock(typeof(SomeObject)) est significativement pire que lock(this). Ayant dit cela; bravo sincères à Alan pour souligner que lock(typeof(SomeObject)) est une mauvaise pratique.

Une instance de System.Type est l'un des objets les plus génériques et à gros grain qu'il existe. À tout le moins, une instance de System.Type est globale à un AppDomain et .NET peut exécuter plusieurs programmes dans un AppDomain. Cela signifie que deux programmes entièrement différents peuvent potentiellement causer des interférences mutuelles, au point de créer un blocage s'ils essayent tous deux d'obtenir un verrouillage de synchronisation sur la même instance de type.

Alors lock(this) La forme n'est pas particulièrement robuste, peut causer des problèmes et doit toujours lever les sourcils pour toutes les raisons citées. Pourtant, il existe un code largement utilisé, relativement respecté et apparemment stable, comme log4net, qui utilise largement le schéma de verrouillage (this), même si je préférerais personnellement que ce modèle change.

Mais lock(typeof(SomeObject)) ouvre une toute nouvelle boîte de vers améliorée.

Pour ce que ça vaut.


32



... et les mêmes arguments s'appliquent à cette construction:

lock(typeof(SomeObject))

24



Imaginez que vous ayez une secrétaire compétente dans votre bureau qui est une ressource partagée dans le ministère. De temps en temps, vous vous précipitez vers eux parce que vous avez une tâche, seulement pour espérer qu'un autre de vos collègues ne les a pas déjà réclamés. Habituellement, vous n'avez qu'à attendre une courte période de temps.

Parce que le soin est le partage, votre responsable décide que les clients peuvent également utiliser la secrétaire directement. Mais cela a un effet secondaire: un client peut même les réclamer pendant que vous travaillez pour ce client et vous en avez également besoin pour exécuter une partie des tâches. Une impasse se produit, car la revendication n'est plus une hiérarchie. Cela aurait pu être évité tous ensemble en ne permettant pas aux clients de les réclamer en premier lieu.

lock(this) est mauvais comme nous l'avons vu. Un objet extérieur peut se verrouiller sur l'objet et comme vous ne contrôlez pas qui utilise la classe, n'importe qui peut le verrouiller ... C'est l'exemple exact décrit ci-dessus. Encore une fois, la solution est de limiter l'exposition de l'objet. Cependant, si vous avez un private, protected ou internal classe toi pourrait déjà contrôler qui se verrouille sur votre objet, parce que vous êtes sûr d'avoir écrit votre code vous-même. Donc, le message ici est: ne pas l'exposer comme public. De plus, s'assurer qu'un verrou est utilisé dans des scénarios similaires évite les blocages.

Le contraire de cela est de verrouiller les ressources qui sont partagées dans le domaine de l'application - le pire des cas. C'est comme mettre votre secrétaire dehors et permettre à tout le monde de les réclamer. Le résultat est un chaos total - ou en termes de code source: c'était une mauvaise idée; jetez-le et recommencez. Alors, comment faisons-nous cela?

Les types sont partagés dans le domaine de l'application, comme le soulignent la plupart des personnes ici. Mais il y a encore mieux que nous pouvons utiliser: les cordes. La raison en est que les chaînes sont mis en commun. En d'autres termes: si vous avez deux chaînes qui ont le même contenu dans un domaine d'application, il est possible qu'elles aient exactement le même pointeur. Comme le pointeur est utilisé comme clé de verrouillage, ce que vous obtenez est un synonyme de "préparer à un comportement indéfini".

De même, vous ne devez pas verrouiller les objets WCF, HttpContext.Current, Thread.Current, Singletons (en général), etc. La manière la plus simple d'éviter tout cela? private [static] object myLock = new object();


6



Verrouillage sur le ce le pointeur peut être mal si vous verrouillez un ressource partagée. Une ressource partagée peut être une variable statique ou un fichier sur votre ordinateur, c'est-à-dire quelque chose qui est partagé entre tous les utilisateurs de la classe. La raison en est que le pointeur this contiendra une référence différente à un emplacement en mémoire chaque fois que votre classe est instanciée. Donc, le verrouillage ce en une fois l'instance d'une classe est différente de se verrouiller ce dans une autre instance d'une classe.

Consultez ce code pour voir ce que je veux dire. Ajoutez le code suivant à votre programme principal dans une application console:

    static void Main(string[] args)
    {
         TestThreading();
         Console.ReadLine();
    }

    public static void TestThreading()
    {
        Random rand = new Random();
        Thread[] threads = new Thread[10];
        TestLock.balance = 100000;
        for (int i = 0; i < 10; i++)
        {
            TestLock tl = new TestLock();
            Thread t = new Thread(new ThreadStart(tl.WithdrawAmount));
            threads[i] = t;
        }
        for (int i = 0; i < 10; i++)
        {
            threads[i].Start();
        }
        Console.Read();
    }

Créez une nouvelle classe comme ci-dessous.

 class TestLock
{
    public static int balance { get; set; }
    public static readonly Object myLock = new Object();

    public void Withdraw(int amount)
    {
      // Try both locks to see what I mean
      //             lock (this)
       lock (myLock)
        {
            Random rand = new Random();
            if (balance >= amount)
            {
                Console.WriteLine("Balance before Withdrawal :  " + balance);
                Console.WriteLine("Withdraw        : -" + amount);
                balance = balance - amount;
                Console.WriteLine("Balance after Withdrawal  :  " + balance);
            }
            else
            {
                Console.WriteLine("Can't process your transaction, current balance is :  " + balance + " and you tried to withdraw " + amount);
            }
        }

    }
    public void WithdrawAmount()
    {
        Random rand = new Random();
        Withdraw(rand.Next(1, 100) * 100);
    }
}

Voici une exécution du programme se verrouillant sur ce.

   Balance before Withdrawal :  100000
    Withdraw        : -5600
    Balance after Withdrawal  :  94400
    Balance before Withdrawal :  100000
    Balance before Withdrawal :  100000
    Withdraw        : -5600
    Balance after Withdrawal  :  88800
    Withdraw        : -5600
    Balance after Withdrawal  :  83200
    Balance before Withdrawal :  83200
    Withdraw        : -9100
    Balance after Withdrawal  :  74100
    Balance before Withdrawal :  74100
    Withdraw        : -9100
    Balance before Withdrawal :  74100
    Withdraw        : -9100
    Balance after Withdrawal  :  55900
    Balance after Withdrawal  :  65000
    Balance before Withdrawal :  55900
    Withdraw        : -9100
    Balance after Withdrawal  :  46800
    Balance before Withdrawal :  46800
    Withdraw        : -2800
    Balance after Withdrawal  :  44000
    Balance before Withdrawal :  44000
    Withdraw        : -2800
    Balance after Withdrawal  :  41200
    Balance before Withdrawal :  44000
    Withdraw        : -2800
    Balance after Withdrawal  :  38400

Voici une exécution du programme se verrouillant sur myLock.

Balance before Withdrawal :  100000
Withdraw        : -6600
Balance after Withdrawal  :  93400
Balance before Withdrawal :  93400
Withdraw        : -6600
Balance after Withdrawal  :  86800
Balance before Withdrawal :  86800
Withdraw        : -200
Balance after Withdrawal  :  86600
Balance before Withdrawal :  86600
Withdraw        : -8500
Balance after Withdrawal  :  78100
Balance before Withdrawal :  78100
Withdraw        : -8500
Balance after Withdrawal  :  69600
Balance before Withdrawal :  69600
Withdraw        : -8500
Balance after Withdrawal  :  61100
Balance before Withdrawal :  61100
Withdraw        : -2200
Balance after Withdrawal  :  58900
Balance before Withdrawal :  58900
Withdraw        : -2200
Balance after Withdrawal  :  56700
Balance before Withdrawal :  56700
Withdraw        : -2200
Balance after Withdrawal  :  54500
Balance before Withdrawal :  54500
Withdraw        : -500
Balance after Withdrawal  :  54000

4



Il y a un très bon article à ce sujet http://bytes.com/topic/c-sharp/answers/249277-dont-lock-type-objects par Rico Mariani, architecte de performance pour l'environnement d'exécution Microsoft .NET

Extrait:

Le problème de base ici est que vous ne possédez pas l'objet type, et vous   Je ne sais pas qui d'autre pourrait y avoir accès. En général, c'est une très mauvaise idée   compter sur le verrouillage d'un objet que vous n'avez pas créé et ne savez pas qui d'autre   pourrait avoir accès. Cela entraîne une impasse. Le moyen le plus sûr est de   verrouille uniquement les objets privés.


3



Il y a aussi une bonne discussion à ce sujet ici: Est-ce la bonne utilisation d'un mutex? 


2



Parce que n'importe quel morceau de code qui peut voir l'instance de votre classe peut également verrouiller sur cette référence. Vous voulez masquer (encapsuler) votre objet de verrouillage de sorte que seul le code qui doit le référencer puisse le référencer. Le mot-clé this fait référence à l'instance de classe en cours, de sorte que plusieurs éléments peuvent s'y référer et l'utiliser pour effectuer la synchronisation des threads.

Pour être clair, cela est mauvais car un autre morceau de code pourrait utiliser l'instance de classe pour se verrouiller et empêcher votre code d'obtenir un verrou opportun ou créer d'autres problèmes de synchronisation de thread. Meilleur cas: rien d'autre n'utilise une référence à votre classe pour verrouiller. Cas moyen: quelque chose utilise une référence à votre classe pour faire des verrous et cela cause des problèmes de performance. Le pire des cas: quelque chose utilise une référence de votre classe pour faire des verrous et cela cause des problèmes vraiment graves, très subtils, vraiment difficiles à déboguer.


1



Voici un exemple de code plus simple à suivre (IMO): LinqPad, référence les espaces de noms suivants: System.Net et System.Threading.Tasks)

void Main()
{
    ClassTest test = new ClassTest();
    lock(test)
    {
        Parallel.Invoke (
            () => test.DoWorkUsingThisLock(1),
            () => test.DoWorkUsingThisLock(2)
        );
    }
}

public class ClassTest
{
    public void DoWorkUsingThisLock(int i)
    {
        Console.WriteLine("Before ClassTest.DoWorkUsingThisLock " + i);
        lock(this)
        {
            Console.WriteLine("ClassTest.DoWorkUsingThisLock " + i);
            Thread.Sleep(1000);
        }
        Console.WriteLine("ClassTest.DoWorkUsingThisLock Done " + i);
    }
}

1