Почему блокировка (это) {...} плохая? - PullRequest
447 голосов
/ 30 октября 2008

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

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

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

Ответы [ 16 ]

479 голосов
/ 30 октября 2008

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

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

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

Наконец, существует распространенное заблуждение, что lock(this) фактически изменяет объект, переданный в качестве параметра, и каким-то образом делает его доступным только для чтения или недоступным. Это false . Объект, переданный в качестве параметра 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'.
62 голосов
/ 30 октября 2008

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

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

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

42 голосов
/ 30 октября 2008

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

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

32 голосов
/ 09 мая 2012

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

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

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

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

За что это стоит.

25 голосов
/ 30 октября 2008

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

lock(typeof(SomeObject))
6 голосов
/ 12 июня 2013

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

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

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

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

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

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

4 голосов
/ 23 марта 2014

Блокировка для указателя this может быть bad , если вы блокируете общий ресурс . Общим ресурсом может быть статическая переменная или файл на вашем компьютере, т. Е. Что-то общее для всех пользователей класса. Причина в том, что указатель this будет содержать разные ссылки на места в памяти каждый раз, когда создается экземпляр вашего класса. Таким образом, блокировка 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);
    }
}

Вот прогон блокировки программы на this .

   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
3 голосов
/ 15 августа 2014

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

Выдержка:

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

2 голосов
/ 28 июля 2018

Вот гораздо более простая иллюстрация (взято из Вопрос 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

2 голосов
/ 30 октября 2008

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

Добро пожаловать на сайт PullRequest, где вы можете задавать вопросы и получать ответы от других членов сообщества.
...