Pourquoi le verrou (ce) {…} est-il mauvais?

484

La documentation MSDN indique que

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

est "un problème si l'instance est accessible publiquement". Je me demande pourquoi? Est-ce parce que le verrou sera maintenu plus longtemps que nécessaire? Ou y a-t-il une raison plus insidieuse?

Anton
la source

Réponses:

508

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

Afin de planifier correctement les opérations parallèles, une attention particulière doit être portée à la prise en compte d'éventuelles situations de blocage, et le fait d'avoir un nombre inconnu de points d'entrée de verrouillage empêche cela. Par exemple, toute personne ayant une référence à l'objet peut le verrouiller sans que le concepteur / créateur d'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 imposera des restrictions d'accès et encapsulera le mécanisme de verrouillage. L'utilisation thisviole l'encapsulation en exposant une partie de votre implémentation de verrouillage au public. Il n'est pas clair non plus que vous obtiendrez un verrou à thismoins qu'il n'ait été documenté. Même alors, se fier à la documentation pour éviter un problème n'est pas optimal.

Enfin, il y a l'idée fausse courante qui 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 passé en paramètre locksert simplement de clé . Si un verrou est déjà maintenu sur cette clé, le verrouillage ne peut pas être effectué; sinon, le verrouillage est autorisé.

C'est pourquoi il est mauvais d'utiliser des chaînes comme clés dans les lockinstructions, car elles sont immuables et sont partagées / accessibles dans toutes les parties de l'application. Vous devriez utiliser une variable privée à la place, une Objectinstance fera l'affaire.

Exécutez le code C # suivant comme 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 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'.
Esteban Brenes
la source
2
Comme je le dis: (1) Nancy est dans le thread1 avec verrou (ceci). (2) MÊME Nancy est dans le thread2 vieillissant alors qu'elle est toujours verrouillée dans le thread1 - prouver qu'un objet verrouillé n'est pas en lecture seule. AUSSI (2a) dans le thread 2, cet objet Nancy est également verrouillé sur Nom. (3) Créez un objet DIFFÉRENT avec le même nom . (4) Passez dans thread3 et essayez de verrouiller avec Name. (grande finition) MAIS "les chaînes sont immuables" signifiant que tout objet référençant la chaîne "Nancy Drew" regarde littéralement la même instance de chaîne en mémoire. Ainsi, object2 ne peut pas verrouiller une chaîne lorsque object1 est verrouillé sur la même valeur
radarbob
L'utilisation d'une variable standard au lieu de lock(this)constitue un conseil standard; il est important de noter que cela rend généralement impossible pour le code extérieur de maintenir le verrou associé à l'objet entre les appels de méthode. Cela peut ou non être une bonne chose . Il y a un certain danger à permettre au code extérieur de maintenir un verrou pendant une durée arbitraire, et les classes devraient généralement être conçues de manière à rendre une telle utilisation inutile, mais il n'y a pas toujours d'alternatives pratiques. À titre d'exemple simple, à moins qu'une collection ToArrayToList
n'implémente
4
(par opposition aux méthodes d'extension `IEnumerable <T>), la seule façon pour un thread qui souhaite obtenir un instantané de la collection d'en obtenir une peut être de l'énumérer tout en verrouillant toutes les modifications . Pour ce faire, il doit avoir accès à un verrou acquis par n'importe quel code qui changerait la collection. Le fait de ne pas exposer le verrou peut rendre impossible, par exemple, que le programme effectue périodiquement un instantané asynchrone de la collection (par exemple, pour mettre à jour une interface utilisateur de navigation dans la collection).
supercat
there is the common misconception that lock(this) actually modifies the object passed as a parameter, and in some way makes it read-only or inaccessible. This is false- Je crois que ces discussions portent sur le bit SyncBlock dans un objet CLR, donc formellement c'est vrai - verrouiller l'objet modifié lui
sll
@Esteban, j'adore votre exemple, c'est génial. J'ai une question pour vous. Votre code de la méthode NameChange (..) se termine par: <code> if (Monitor.TryEnter (person.Name, 10000)) {. . . } else Monitor.Exit (person.Name); </code> Si cela ne se termine pas par: <code> if (Monitor.TryEnter (person.Name, 10000)) {. . . Monitor.Exit (person.Name); } </code>
AviFarah
64

Parce que si les gens peuvent accéder à votre instance d'objet (c'est-à-dire votre thispointeur), ils peuvent également essayer de verrouiller ce même objet. Maintenant, ils ne savent peut-être pas que vous vous verrouillez en thisinterne, donc cela peut causer des problèmes (peut-être un blocage)

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

Par exemple, vous pouvez avoir une variable membre de List<int>, et la seule chose que vous devez réellement verrouiller est cette variable membre. Si vous verrouillez l'intégralité de l'objet dans vos fonctions, les autres éléments qui appellent ces fonctions seront bloqués en attendant le verrouillage. Si ces fonctions n'ont pas besoin d'accéder à la liste des membres, vous allez faire attendre un autre code et ralentir votre application sans aucune raison.

Orion Edwards
la source
44
Le dernier paragraphe de cette réponse n'est pas correct. Le verrouillage ne rend en aucun cas l'objet inaccessible ou en lecture seule. Lock (this) n'empêche pas un autre thread d'appeler ou de modifier l'objet référencé par celui-ci.
Esteban Brenes
3
Il le fait si les autres méthodes appelées font également un verrou (this). Je crois que c'est le point qu'il faisait valoir. Remarquez le "Si vous verrouillez tout l'objet dans vos fonctions" ...
Herms
@Orion: C'est plus clair. @Herms: Oui, mais vous n'avez pas besoin d'utiliser `` ceci '' pour obtenir cette fonctionnalité, la propriété SyncRoot dans les listes sert à cet effet, par exemple, tout en précisant que la synchronisation doit être effectuée sur cette clé.
Esteban Brenes
Re: verrouiller «trop»: c'est un acte d'équilibrage fin de décider quoi verrouiller. Sachez que la prise d'un verrou implique des opérations de processeur de vidage du cache et est quelque peu coûteuse. En d'autres termes: ne verrouillez pas et ne mettez pas à jour chaque entier individuel. :)
Zan Lynx
Le dernier paragraphe n'a toujours pas de sens. Si vous avez seulement besoin de restreindre l'accès à la liste, pourquoi les autres fonctions auraient-elles des verrous si elles n'accèdent pas à la liste?
Joakim MH
44

Jetez un oeil à la synchronisation des threads de rubrique MSDN (Guide de programmation C #)

En règle générale, il est préférable d'éviter de verrouiller un type public ou des instances d'objet indépendantes de la volonté de votre application. Par exemple, lock (this) peut être problématique si l'instance est accessible au public, car le code indépendant de votre volonté peut également verrouiller l'objet. Cela pourrait créer des situations de blocage où deux threads ou plus attendent la libération du même objet. Le verrouillage d'un type de données public, par opposition à un objet, peut provoquer des problèmes pour la même raison. Le verrouillage des chaînes littérales est particulièrement risqué car les chaînes littérales sont internées par le Common Language Runtime (CLR). Cela signifie qu'il existe une instance d'un littéral de chaîne donné pour l'ensemble du programme, le même objet exact représente le littéral dans tous les domaines d'application en cours d'exécution, sur tous les threads. Par conséquent, un verrou placé sur une chaîne avec le même contenu n'importe où dans le processus d'application verrouille toutes les instances de cette chaîne dans l'application. Par conséquent, il est préférable de verrouiller un membre privé ou protégé qui n'est pas interné. Certaines classes fournissent des membres spécifiquement pour le verrouillage. Le type Array, par exemple, fournit SyncRoot. De nombreux types de collections fournissent également un membre SyncRoot.

crashmstr
la source
34

Je sais que c'est un vieux fil conducteur, mais parce que les gens peuvent toujours le chercher et s'y fier, il semble important de souligner que lock(typeof(SomeObject))c'est bien pire que lock(this). Ayant dit cela; bravo sincère à Alan pour avoir souligné que lock(typeof(SomeObject))c'est une mauvaise pratique.

Une instance de System.Typeest l'un des objets les plus génériques à grain grossier qui existe. À tout le moins, une instance de System.Type est globale pour un AppDomain et .NET peut exécuter plusieurs programmes dans un AppDomain. Cela signifie que deux programmes entièrement différents pourraient potentiellement provoquer des interférences l'un dans l'autre, même au point de créer un blocage s'ils tentent tous les deux d'obtenir un verrou de synchronisation sur la même instance de type.

Ce lock(this)n'est donc pas une forme particulièrement robuste, peut causer des problèmes et devrait toujours lever les sourcils pour toutes les raisons citées. Pourtant, il existe un code largement utilisé, relativement bien respecté et apparemment stable comme log4net qui utilise largement le modèle de verrouillage (ce), 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.

Craig
la source
26

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

lock(typeof(SomeObject))
Alan
la source
17
lock (typeof (SomeObject)) est en réalité bien pire que lock (this) ( stackoverflow.com/a/10510647/618649 ).
Craig
1
eh bien, verrouiller (Application.Current) est encore pire alors, mais qui essaierait l'une ou l'autre de ces choses stupides de toute façon? lock (this) semble logique et succint, mais pas ces autres exemples.
Zar Shardan
Je ne suis pas d'accord pour dire que cela lock(this)semble particulièrement logique et succinct. C'est un verrou terriblement grossier, et tout autre code pourrait verrouiller votre objet, ce qui pourrait provoquer des interférences dans votre code interne. Prenez des verrous plus granuleux et prenez un contrôle plus strict. Ce lock(this)qui est vrai, c'est que c'est beaucoup mieux que lock(typeof(SomeObject)).
Craig
8

Imaginez que vous ayez une secrétaire qualifiée à votre bureau qui soit une ressource partagée au sein du 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 un court instant.

Parce que se soucier c'est partager, votre manager décide que les clients peuvent également utiliser directement la secrétaire. 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. Un blocage 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 puisque vous ne contrôlez pas qui utilise la classe, tout le monde 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. Toutefois, si vous avez private, protectedou internalclasse que vous pouvez déjà contrôler qui est verrouillage sur votre objet , parce que vous êtes sûr que vous avez écrit votre code vous - même. Donc le message ici est: ne l'exposez pas en tant que public. De plus, garantir qu'un verrou est utilisé dans des scénarios similaires évite les blocages.

L'opposé complet de cela est de verrouiller les ressources partagées dans tout le domaine d'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 on fait ça?

Les types sont partagés dans le domaine de l'application, comme le soulignent la plupart des gens ici. Mais il y a encore de meilleures choses que nous pouvons utiliser: les chaînes. La raison en est que les chaînes sont regroupées . 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. Puisque le pointeur est utilisé comme clé de verrouillage, ce que vous obtenez essentiellement est un synonyme de "se préparer à un comportement indéfini".

De même, vous ne devriez 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();

atlaste
la source
3
En fait, avoir une classe privée n'empêche pas le problème. Le code externe peut obtenir une référence à une instance d'une classe privée ...
Rashack
1
@Rashack alors que vous êtes techniquement correct (+1 pour le signaler), mon point était que vous devriez contrôler qui verrouille l'instance. Le retour d'instances comme celle-ci rompt cela.
atlaste
4

Le verrouillage du pointeur this peut être mauvais si vous verrouillez une 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. Ainsi, le verrouillage sur cette instance unique d'une classe est différent de celui sur 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 de verrouillage sur ce point .

   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 de verrouillage 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
ItsAllABadJoke
la source
1
quelle est la chose à noter dans votre exemple, comme ce que vous montrez qui est incorrect. il est difficile de repérer ce qui ne va pas lorsque vous utilisez Random rand = new Random();nvm, je pense que je vois son équilibre répété
Seabizkit
3

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 la performance pour le runtime Microsoft® .NET

Extrait:

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

vikrant
la source
2

Voici une illustration beaucoup plus simple (tirée de la question 34 ici ) pourquoi le verrouillage (ceci) est mauvais et peut entraîner des blocages lorsque le consommateur de votre classe essaie également de verrouiller l'objet. Ci-dessous, un seul des trois threads peut continuer, les deux autres sont bloqués.

class SomeClass
{
    public void SomeMethod(int id)
    {
        **lock(this)**
        {
            while(true)
            {
                Console.WriteLine("SomeClass.SomeMethod #" + id);
            }
        }
    }
}

class Program
{
    static void Main(string[] args)
    {
        SomeClass o = new SomeClass();

        lock(o)
        {
            for (int threadId = 0; threadId < 3; threadId++)
            {
                Thread t = new Thread(() => {
                    o.SomeMethod(threadId);
                        });
                t.Start();
            }

            Console.WriteLine();
        }

Pour contourner ce problème, ce type a utilisé Thread.TryMonitor (avec timeout) au lieu de verrouiller:

            Monitor.TryEnter(temp, millisecondsTimeout, ref lockWasTaken);
            if (lockWasTaken)
            {
                doAction();
            }
            else
            {
                throw new Exception("Could not get lock");
            }

https://blogs.appbeat.io/post/c-how-to-lock-without-deadlocks

utilisateur3761555
la source
Pour autant que je vois, lorsque je remplace le verrou (ceci) par un verrou sur un membre d'instance privée de SomeClass, j'obtiens toujours le même blocage. En outre, si le verrouillage de la classe principale est effectué sur un autre membre d'instance privée de Program, le même verrouillage se produit. Donc, je ne sais pas si cette réponse n'est pas trompeuse et incorrecte. Voir ce comportement ici: dotnetfiddle.net/DMrU5h
Bartosz
1

Parce que tout morceau de code qui peut voir l'instance de votre classe peut également verrouiller cette référence. Vous souhaitez masquer (encapsuler) votre objet de verrouillage afin 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 actuelle, donc un certain nombre de choses pourraient y faire référence et pourraient 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 verrouiller et pourrait empêcher votre code d'obtenir un verrouillage en temps opportun ou pourrait créer d'autres problèmes de synchronisation de thread. Meilleur cas: rien d'autre n'utilise une référence à votre classe pour se verrouiller. Cas du milieu: quelque chose utilise une référence à votre classe pour faire des verrous et cela cause des problèmes de performances. Pire cas: quelque chose utilise une référence de votre classe pour faire des verrous et cela cause des problèmes vraiment mauvais, vraiment subtils, vraiment difficiles à déboguer.

Jason Jackson
la source
1

Désolé les gars, mais je ne suis pas d'accord avec l'argument selon lequel le verrouillage pourrait entraîner un blocage. Vous confondez deux choses: l'impasse et la faim.

  • Vous ne pouvez pas annuler l'impasse sans interrompre l'un des threads, donc après être entré dans une impasse, vous ne pouvez pas en sortir
  • La famine se terminera automatiquement une fois que l'un des threads aura terminé son travail

Voici une image qui illustre la différence.

Conclusion
Vous pouvez toujours utiliser en toute sécurité lock(this)si la famine de fil n'est pas un problème pour vous. Vous devez toujours garder à l'esprit que lorsque le fil, qui utilise le fil affamé en utilisant des lock(this)extrémités dans une serrure ayant votre objet verrouillé, il se terminera finalement par une famine éternelle;)

SOReader
la source
9
Il y a une différence, mais cela n'a absolument rien à voir avec cette discussion. Et la première phrase de votre conclusion est complètement fausse.
Ben Voigt
1
Pour être clair: je ne défends pas lock(this)- ce type de code est tout simplement faux. Je pense simplement que le qualifier d'impasse est un peu abusif.
SOReader
2
Le lien vers l'image n'est plus disponible. :( Y a-t-il une chance que vous puissiez le référencer à nouveau? Thx
VG1
1

Veuillez vous référer au lien suivant qui explique pourquoi verrouiller (ceci) n'est pas une bonne idée.

http://blogs.msdn.com/b/bclteam/archive/2004/01/20/60719.aspx

La solution consiste donc à ajouter un objet privé, par exemple, lockObject à la classe et à placer la région de code à l'intérieur de l'instruction lock, comme indiqué ci-dessous:

lock (lockObject)
{
...
}
Dhruv Rangunwala
la source
le lien n'est plus valide.
Rauld
1

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

Quelque chose à retenir est que lock (x) est fondamentalement du sucre syntaxique et ce qu'il fait est d'utiliser Monitor.Enter puis utilise un try, catch, finalement block pour appeler Monitor.Exit. Voir: https://docs.microsoft.com/en-us/dotnet/api/system.threading.monitor.enter (section remarques)

ou utilisez l'instruction de verrouillage C # (instruction SyncLock dans Visual Basic), qui encapsule les méthodes Enter et Exit dans un bloc try… finally.

void Main()
{
    //demonstrates why locking on THIS is BADD! (you should never lock on something that is publicly accessible)
    ClassTest test = new ClassTest();
    lock(test) //locking on the instance of ClassTest
    {
        Console.WriteLine($"CurrentThread {Thread.CurrentThread.ManagedThreadId}");
        Parallel.Invoke(new Action[]
        {
            () => {
                //this is there to just use up the current main thread. 
                Console.WriteLine($"CurrentThread {Thread.CurrentThread.ManagedThreadId}");
                },
            //none of these will enter the lock section.
            () => test.DoWorkUsingThisLock(1),//this will dead lock as lock(x) uses Monitor.Enter
            () => test.DoWorkUsingMonitor(2), //this will not dead lock as it uses Montory.TryEnter
        });
    }
}

public class ClassTest
{
    public void DoWorkUsingThisLock(int i)
    {
        Console.WriteLine($"Start ClassTest.DoWorkUsingThisLock {i} CurrentThread {Thread.CurrentThread.ManagedThreadId}");
        lock(this) //this can be bad if someone has locked on this already, as it will cause it to be deadlocked!
        {
            Console.WriteLine($"Running: ClassTest.DoWorkUsingThisLock {i} CurrentThread {Thread.CurrentThread.ManagedThreadId}");
            Thread.Sleep(1000);
        }
        Console.WriteLine($"End ClassTest.DoWorkUsingThisLock Done {i}  CurrentThread {Thread.CurrentThread.ManagedThreadId}");
    }

    public void DoWorkUsingMonitor(int i)
    {
        Console.WriteLine($"Start ClassTest.DoWorkUsingMonitor {i} CurrentThread {Thread.CurrentThread.ManagedThreadId}");
        if (Monitor.TryEnter(this))
        {
            Console.WriteLine($"Running: ClassTest.DoWorkUsingMonitor {i} CurrentThread {Thread.CurrentThread.ManagedThreadId}");
            Thread.Sleep(1000);
            Monitor.Exit(this);
        }
        else
        {
            Console.WriteLine($"Skipped lock section!  {i} CurrentThread {Thread.CurrentThread.ManagedThreadId}");
        }

        Console.WriteLine($"End ClassTest.DoWorkUsingMonitor Done {i} CurrentThread {Thread.CurrentThread.ManagedThreadId}");
        Console.WriteLine();
    }
}

Production

CurrentThread 15
CurrentThread 15
Start ClassTest.DoWorkUsingMonitor 2 CurrentThread 13
Start ClassTest.DoWorkUsingThisLock 1 CurrentThread 12
Skipped lock section!  2 CurrentThread 13
End ClassTest.DoWorkUsingMonitor Done 2 CurrentThread 13

Notez que le thread # 12 ne se termine jamais car il est verrouillé.

Raj Rao
la source
1
semble que le deuxième DoWorkUsingThisLockfil n'est pas nécessaire pour illustrer le problème?
Jack Lu
ne voulez-vous pas dire que le verrou extérieur est principal, un thread attendrait simplement que l'autre se termine? ce qui invaliderait alors le Parallèle ... je pense que nous avons besoin de meilleurs exemples du monde réel ..
Seabizkit
@Seabizkit, a mis à jour le code pour le rendre un peu plus clair. Le parallèle est là juste pour créer un nouveau thread et exécuter le code de manière asynchrone. En réalité, le 2ème thread aurait pu être invoqué de différentes manières (clic de bouton, demande séparée, etc.).
Raj Rao
0

Vous pouvez établir une règle qui dit qu'une classe peut avoir du code qui se verrouille sur «ceci» ou tout autre objet que le code de la classe instancie. Ce n'est donc un problème que si le modèle n'est pas suivi.

Si vous souhaitez vous protéger d'un code qui ne suivra pas ce modèle, la réponse acceptée est correcte. Mais si le modèle est suivi, ce n'est pas un problème.

L'avantage de la serrure (ceci) est l'efficacité. Que faire si vous avez un simple "objet valeur" qui contient une seule valeur. Ce n'est qu'un emballage, et il est instancié des millions de fois. En exigeant la création d'un objet de synchronisation privé uniquement pour le verrouillage, vous avez essentiellement doublé la taille de l'objet et doublé le nombre d'allocations. Lorsque les performances sont importantes, c'est un avantage.

Lorsque vous ne vous souciez pas du nombre d'allocations ou de l'encombrement de la mémoire, éviter le verrouillage (ceci) est préférable pour les raisons indiquées dans d'autres réponses.

zumalifeguard
la source
-1

Il y aura un problème si l'instance est accessible publiquement car il pourrait y avoir d'autres demandes qui pourraient utiliser la même instance d'objet. Il est préférable d'utiliser une variable privée / statique.

William
la source
5
Je ne sais pas ce que cela ajoute à l'homme, des réponses détaillées déjà existantes qui disent la même chose.
Andrew Barber