Почему lock (this) {...} плохой?

Потому что, если люди могут получить указатель на ваш экземпляр объекта (например, ваш this), они также могут попытаться заблокировать этот же объект. Теперь они могут не знать, что вы блокируете this изнутри, поэтому это может вызвать проблемы (возможно, тупик).

В дополнение к этому, это еще и плохая практика, потому что она блокирует "слишком много".

Например, у вас может быть переменная-член List<int>, и единственное, что вам действительно нужно заблокировать, - это эту переменную-член. Если вы заблокируете весь объект в своих функциях, тогда другие вещи, которые вызывают эти функции, будут заблокированы в ожидании блокировки. Если этим функциям не нужен доступ к списку участников, вы заставите другой код ждать и замедлять работу вашего приложения без всякой причины.

... и те же самые аргументы применимы и к этой конструкции:

lock(typeof(SomeObject))

Взгляните на тему MSDN Синхронизация потоков (Руководство по программированию на C#)

Generally, it is best to avoid locking on a public type, or on object instances beyond the control of your application. For example, lock(this) can be problematic if the instance can be accessed publicly, because code beyond your control may lock on the object as well. This could create deadlock situations where two or more threads wait for the release of the same object. Locking on a public data type, as opposed to an object, can cause problems for the same reason. Locking on literal strings is especially risky because literal strings are interned by the common language runtime (CLR). This means that there is one instance of any given string literal for the entire program, the exact same object represents the literal in all running application domains, on all threads. As a result, a lock placed on a string with the same contents anywhere in the application process locks all instances of that string in the application. As a result, it is best to lock a private or protected member that is not interned. Some classes provide members specifically for locking. The Array type, for example, provides SyncRoot. Many collection types provide a SyncRoot member as well.

Потому что любой фрагмент кода, который может видеть экземпляр вашего класса, также может заблокировать эту ссылку. Вы хотите скрыть (инкапсулировать) объект блокировки, чтобы на него мог ссылаться только код, который должен ссылаться на него. Ключевое слово this относится к текущему экземпляру класса, поэтому любое количество вещей может иметь ссылку на него и может использовать его для синхронизации потоков.

Чтобы было ясно, это плохо, потому что какой-то другой фрагмент кода может использовать экземпляр класса для блокировки и может помешать вашему коду получить своевременную блокировку или может создать другие проблемы синхронизации потоков. В лучшем случае: ничто другое не использует ссылку на ваш класс для блокировки. Средний случай: что-то использует ссылку на ваш класс для блокировки и вызывает проблемы с производительностью. Худший случай: что-то использует ссылку на ваш класс для блокировок, и это вызывает действительно серьезные, очень тонкие, действительно сложные для отладки проблемы.

Здесь также есть хорошее обсуждение этого здесь: Это правильное использование мьютекса?

Использование this в операторах блокировки - плохой тон, потому что, как правило, вы не можете контролировать, кто еще может блокировать этот объект.

Чтобы правильно спланировать параллельные операции, следует уделять особое внимание возможным ситуациям взаимоблокировки, а наличие неизвестного количества точек входа в блокировку препятствует этому. Например, любой, у кого есть ссылка на объект, может заблокировать его без ведома дизайнера / создателя объекта. Это увеличивает сложность многопоточных решений и может повлиять на их корректность.

Частное поле обычно является лучшим вариантом, поскольку компилятор применяет к нему ограничения доступа и инкапсулирует механизм блокировки. Использование this нарушает инкапсуляцию, поскольку часть вашей реализации блокировки становится общедоступной. Также не ясно, будет ли у вас блокировка this, если это не было задокументировано. Даже в этом случае полагаться на документацию для предотвращения проблемы неоптимально.

Наконец, существует распространенное заблуждение, что lock(this) фактически изменяет объект, переданный в качестве параметра, и каким-то образом делает его доступным только для чтения или недоступным. Это ложный. Объект, переданный в качестве параметра lock, просто служит ключ. Если замок уже удерживается на этом ключе, замок не может быть выполнен; в противном случае блокировка разрешена.

Вот почему плохо использовать строки в качестве ключей в операторах lock, поскольку они неизменяемы и доступны для разных частей приложения. Вместо этого вы должны использовать частную переменную, экземпляр Object подойдет.

В качестве примера запустите следующий код C#.

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

Консольный вывод

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

Извините, ребята, но я не могу согласиться с аргументом, что блокировка может вызвать тупик. Вы путаете две вещи: тупик и голодание.

• Вы не можете отменить тупик, не прерывая один из потоков, поэтому после того, как вы попали в тупик, вы не сможете выйти
• Голодание прекратится автоматически после того, как один из потоков закончит свою работу.

Здесь - изображение, которое иллюстрирует разницу.

Заключение
Вы все равно можете безопасно использовать lock(this), если нехватка потоков для вас не проблема. Вы все равно должны помнить, что когда поток, который голодает поток, использующий lock(this), заканчивается блокировкой, в которой ваш объект заблокирован, он, наконец, закончится вечным голоданием;)

Я знаю, что это старая ветка, но поскольку люди все еще могут найти ее и положиться на нее, кажется важным отметить, что lock(typeof(SomeObject)) значительно хуже, чем lock(this). Было сказано, что; искренние похвалы Алану за указание на то, что lock(typeof(SomeObject)) - плохая практика.

Экземпляр System.Type - один из самых общих, крупнозернистых объектов. По крайней мере, экземпляр System.Type является глобальным для AppDomain, а .NET может запускать несколько программ в AppDomain. Это означает, что две совершенно разные программы могут потенциально вызвать взаимное влияние друг друга даже до степени взаимоблокировки, если обе они попытаются получить синхронизирующую блокировку для экземпляра одного и того же типа.

Таким образом, lock(this) не является особенно надежной формой, может вызывать проблемы и всегда должен вызывать недоумение по всем указанным причинам. Тем не менее, существует широко используемый, относительно уважаемый и, по-видимому, стабильный код, такой как log4net, который широко использует шаблон блокировки (this), хотя я лично предпочел бы увидеть изменение этого шаблона.

Но lock(typeof(SomeObject)) открывает совершенно новую и усовершенствованную банку червей.

Для чего это стоит.

Возникнет проблема, если к экземпляру можно будет получить доступ публично, потому что могут быть другие запросы, которые могут использовать тот же экземпляр объекта. Лучше использовать частную / статическую переменную.

Вот пример кода, которому проще следовать (IMO): (Будет работать в LinqPad, ссылаться на следующие пространства имен: System.Net и System.Threading.Tasks)

Следует помнить, что lock (x) в основном является синтаксическим сахаром, и он использует Monitor.Enter, а затем использует блок try, catch, finally для вызова Monitor.Exit. См .: https://docs.microsoft.com/en-us/dotnet/api/system.threading.monitor.enter (раздел примечаний)

or use the C# lock statement (SyncLock statement in Visual Basic), which wraps the Enter and Exit methods in a try…finally block.

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

Выход

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

Обратите внимание, что Thread # 12 никогда не заканчивается, так как он заблокирован.

Представьте, что у вас в офисе есть опытный секретарь, который является общим ресурсом в отделе. Время от времени вы бросаетесь к ним, потому что у вас есть задача, только в надежде, что кто-то из ваших коллег еще не взял их на себя. Обычно вам нужно подождать совсем немного времени.

Поскольку забота - это совместное использование, ваш менеджер решает, что клиенты также могут использовать секретаря напрямую. Но у этого есть побочный эффект: клиент может даже потребовать их, пока вы работаете на этого клиента, и они также нужны вам для выполнения части задач. Возникает тупик, потому что утверждение больше не является иерархией. Этого можно было бы избежать, не позволяя покупателям в первую очередь требовать их.

lock(this), как мы видели, плохой. Внешний объект может заблокировать объект, и, поскольку вы не контролируете, кто использует класс, любой может заблокировать его ... Это точный пример, описанный выше. Опять же, решение состоит в том, чтобы ограничить экспозицию объекта. Однако, если у вас есть класс private, protected или internal, вы уже может контролировать, кто блокирует ваш объект, потому что вы уверены, что написали свой код самостоятельно. Итак, сообщение здесь: не выставляйте его как public. Кроме того, использование блокировки в аналогичных сценариях позволяет избежать взаимоблокировок.

Полная противоположность этому - блокировка ресурсов, которые являются общими для всего домена приложения - наихудший сценарий. Это все равно, что выставить вашего секретаря на улицу и позволить всем требовать их. Результат - полный хаос - или с точки зрения исходного кода: это была плохая идея; выбросьте его и начните заново. Так как же нам это сделать?

Как отмечает большинство людей, типы являются общими в домене приложения. Но есть еще лучшие вещи, которые мы можем использовать: строки. Причина в том, что строки объединены. Другими словами: если у вас есть две строки с одинаковым содержимым в домене приложения, есть вероятность, что они имеют один и тот же указатель. Поскольку указатель используется в качестве ключа блокировки, то, что вы в основном получаете, является синонимом «подготовиться к неопределенному поведению».

Точно так же вы не должны блокировать объекты WCF, HttpContext.Current, Thread.Current, Singletons (в общем) и т. д. Самый простой способ избежать всего этого? private [static] object myLock = new object();

Блокировка указателя это может быть плохой, если вы блокируете общий ресурс. Общий ресурс может быть статической переменной или файлом на вашем компьютере, то есть чем-то, что совместно используется всеми пользователями класса. Причина в том, что указатель this будет содержать разные ссылки на место в памяти каждый раз, когда создается экземпляр вашего класса. Таким образом, блокировка это в одном экземпляре класса отличается от блокировки это в другом экземпляре класса.

Посмотрите этот код, чтобы понять, что я имею в виду. Добавьте следующий код в свою основную программу в консольном приложении:

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

Создайте новый класс, как показано ниже.

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

Вот запуск блокировки программы на это.

   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

Вот запуск блокировки программы на 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

Об этом есть очень хорошая статья http://bytes.com/topic/c-sharp/answers/249277-dont-lock-type-objects Рико Мариани, архитектора производительности среды выполнения Microsoft® .NET.

Отрывок:

The basic problem here is that you don't own the type object, and you don't know who else could access it. In general, it's a very bad idea to rely on locking an object you didn't create and don't know who else might be accessing. Doing so invites deadlock. The safest way is to only lock private objects.

Пожалуйста, обратитесь к следующей ссылке, в которой объясняется, почему блокировка (это) не является хорошей идеей.

https://docs.microsoft.com/en-us/dotnet/standard/threading/managed-threading-best-practices

Таким образом, решение состоит в том, чтобы добавить в класс частный объект, например lockObject, и поместить область кода внутри оператора блокировки, как показано ниже:

lock (lockObject)
{
...
}

Вот гораздо более простая иллюстрация (взятая из Вопрос 34 здесь), почему lock (this) плохая и может привести к тупикам, когда потребитель вашего класса также пытается заблокировать объект. Ниже может продолжаться только один из трех потоков, два других зашли в тупик.

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

Чтобы обойти эту проблему, этот парень использовал Thread.TryMonitor (с таймаутом) вместо блокировки:

            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

Вы можете установить правило, которое гласит, что у класса может быть код, который блокирует this или любой объект, экземпляры которого создает код в классе. Так что проблема возникает только в том случае, если шаблон не соблюдается.

Если вы хотите защитить себя от кода, который не следует этому шаблону, то принятый ответ правильный. Но если следовать шаблону, это не проблема.

Преимущество блокировки (это) - эффективность. Что делать, если у вас есть простой «объект значения», содержащий одно значение. Это просто оболочка, экземпляры которой создаются миллионы раз. Требуя создания частного объекта синхронизации только для блокировки, вы, по сути, удвоили размер объекта и удвоили количество выделений. Когда производительность имеет значение, это преимущество.

Если вас не волнует количество распределений или объем памяти, избегание блокировки (это) предпочтительнее по причинам, указанным в других ответах.