Почему блокировка (это) {…} плохая?

В документации MSDN сказано, что

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

«проблема, если экземпляр доступен публично». Мне интересно почему? Это потому, что замок будет держаться дольше, чем необходимо? Или есть еще какая-то коварная причина?

Это плохая форма для использования 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'.

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

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

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

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

Как правило, лучше избегать блокирования для открытого типа или для экземпляров объектов, находящихся вне контроля вашего приложения. Например, блокировка (это) может быть проблематичной, если к экземпляру можно получить открытый доступ, поскольку код, находящийся вне вашего контроля, также может блокировать объект.Это может создать тупиковые ситуации, когда два или более потоков ожидают освобождения одного и того же объекта., Блокировка открытого типа данных, в отличие от объекта, может вызвать проблемы по той же причине. Блокировка литеральных строк особенно опасна, потому что литеральные строки интернированы общеязыковой средой исполнения (CLR). Это означает, что существует один экземпляр любого данного строкового литерала для всей программы, точно такой же объект представляет литерал во всех запущенных доменах приложения, во всех потоках. В результате блокировка, помещенная в строку с одинаковым содержимым в любом месте процесса приложения, блокирует все экземпляры этой строки в приложении. В результате лучше заблокировать закрытый или защищенный член, который не интернирован. Некоторые классы предоставляют члены специально для блокировки. Например, тип Array предоставляет SyncRoot. Многие типы коллекций также предоставляют член SyncRoot.

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

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

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

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

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

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

lock(typeof(SomeObject))

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

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

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

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

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

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

Блокировка указателя this может быть плохой, если вы блокируете общий ресурс . Общим ресурсом может быть статическая переменная или файл на вашем компьютере, т. Е. Что-то общее для всех пользователей класса. Причина в том, что указатель 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 от Rico Mariani, архитектора производительности для среды выполнения Microsoft® .NET

Выдержка:

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

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

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

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 ссылается на текущий экземпляр класса, поэтому любое количество вещей может иметь ссылку на него и может использовать его для синхронизации потоков.

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

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

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

Вот картина, которая иллюстрирует разницу.

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

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

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

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

lock (lockObject)
{
...
}

Вот пример кода, которому проще следовать (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 (раздел замечаний).

или используйте оператор блокировки C # (оператор SyncLock в Visual Basic), который оборачивает методы Enter и Exit в блок 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();
    }
}

Вывод

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

Обратите внимание, что поток № 12 никогда не заканчивается, поскольку он заблокирован.

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

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

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

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

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