lock (new object ()) - Cargo culte ou un «cas particulier de langage» fou?

87

J'examine du code écrit par un consultant, et bien que des dizaines de signaux d'alarme soient déjà apparus, je ne peux pas comprendre l'extrait de code suivant:

private void foo()
{
    if (InvokeRequired)
    {
        lock (new object())
        {
            if (m_bar!= null)
                Invoke(new fooDelegate(foo), new object[] { });
        }
    }
    else
    {
        if(OnBazChanged != null)
            OnBazChanged();
    }
}

Que fait lock (new object ()) ici? Ne devrait avoir aucun effet car il se verrouille toujours sur un autre objet, mais ce type de verrouillage est persistant dans tout le code, même dans les parties non copiées-collées. Est-ce un cas particulier dans le langage C # qui est compilé vers quelque chose que je ne connais pas, ou le programmeur a-t-il simplement adopté un culte du cargo qui fonctionnait il y a quelque temps?

Charles
la source
19
Je pense qu'ils sont très confus. Ils l'ont probablement vu là où le new object()était stocké dans un champ, et ce champ était utilisé dans les lock()déclarations, et ils ne savaient pas qu'il valait mieux ne pas le mettre en ligne.
Damien_The_Unbeliever
21
Ce "consultant" a des explications à faire ... vous n'avez pas tort: ​​ce lockcode est totalement inutile
Marc Gravell
12
@Baboon: Seulement si vous n'êtes pas celui qui doit faire le refactoring ...
2
De plus, s'il s'agit de WinForms, je ne vois pas pourquoi il devrait y avoir un verrou là-bas.
Drew Noakes
7
Supprimez-le, puis réexécutez votre suite de tests de couverture de code à 100%. Qu'est-ce que c'est? Le consultant précédent n'en a pas fait?
Spacedman

Réponses:

82

Je ne serais pas surpris si c'était quelqu'un qui avait vu ceci:

private readonly object lockObj = new object();

private void MyMethod()
{
    lock(lockObj)
    {
        // do amazing stuff, so amazing it can only run once at a time
        // e.g. comands on the Mars Rover, or programs on iOS pre 4 / 5 ??
    }
}

et a pensé qu'il pourrait réduire le nombre de lignes.

Je serais très inquiet si c'était le cas cependant ...

cjk
la source
4
Il a peut-être vu une méthode appelée "newObject ()" et cette méthode a renvoyé une instance de singleton, mais il a dit "hé, c # n'a-t-il pas de mots-clés pour cela"?
Amiram Korach
9
Cela ressemble en effet à un travail de refactorisation sans comprendre efficacement ce qui se passait.
Aphelion
1
@OrangeDog: Malheureusement, c'est impossible, car le code concerné a été écrit avant que je rejoigne l'entreprise. Maintenant qu'il y a des changements à faire, je peux peut-être convaincre la direction de me laisser corriger le code. Sinon, je ne prendrai pas la responsabilité des instabilités (le dernier à toucher quoi que ce soit est celui à blâmer) ...
2
@Ibruder Une priorité plus élevée serait de convaincre la direction qu'elle a besoin de systèmes de contrôle de version, de tests automatisés et de révision. Si le dernier à toucher à quoi que ce soit est celui à blâmer, cela ne semble pas être une très bonne entreprise pour laquelle travailler.
OrangeDog
1
Je ne me soucie pas beaucoup du verrouillage manifestement cassé - tout le monde l'a déjà signalé, mais +1 juste pour `` ou pour des programmes sur iOS pré 4/5 '' <g>
Martin James
15

Voici une question similaire et une réponse:

Les verrous garantissent une exclusion mutuelle - pas plus d'un fil ne peut retenir le verrou en même temps. Le verrou est identifié avec une instance d'objet spécifique. Vous créez un nouvel objet à verrouiller à chaque fois et vous n'avez aucun moyen d'informer un autre thread de se verrouiller sur cette même instance d'objet. Par conséquent, votre verrouillage est inutile.

JleruOHeP
la source
2

C'est probablement inutile. Mais il y a une chance qu'il soit là pour créer une barrière mémoire. Je ne sais pas si c # verrouille l'élision ou s'il le fait s'il préserve la sémantique de classement du verrou.

Benmmurphy
la source
C'était il y a quelques années, mais mec, vous méritez totalement +1 pour avoir déclaré ce fait évident que certaines personnes ont complètement ignoré. Par exemple, sur la plate-forme Windows universelle, il n'y a pas de méthode MemoryBarrier () et la magie avec Interlocked.CompareExchange et lock (new Object ()) deviennent les seuls moyens de résoudre certains problèmes.
Sergey.quixoticaxis.Ivanov
Je ne savais même pas que C # permettait cela. Génial de vous le signaler.
Tout le monde