Почему блокировка (это) {...} плохая?
Документация MSDN говорит, что
public class SomeObject
{
public void SomeOperation()
{
lock(this)
{
//Access instance variables
}
}
}
это "проблема, если экземпляр доступен публично". Мне интересно почему? Это потому, что замок будет держаться дольше, чем необходимо? Или есть еще какая-то коварная причина?
18 ответов
Это плохая форма для использования 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
Выдержка:
Основная проблема здесь в том, что вы не являетесь владельцем объекта типа и не знаете, кто еще может получить к нему доступ. В общем, очень плохая идея полагаться на блокировку объекта, который вы не создали, и не знаете, кто еще может получить к нему доступ. Это вызывает тупик. Самый безопасный способ - блокировать только частные объекты.
Здесь также есть хорошая дискуссия: это правильное использование мьютекса?
Пожалуйста, обратитесь к следующей ссылке, которая объясняет, почему блокировка (это) не является хорошей идеей.
http://blogs.msdn.com/b/bclteam/archive/2004/01/20/60719.aspx
Таким образом, решение состоит в том, чтобы добавить частный объект, например, lockObject, в класс и поместить область кода в оператор блокировки, как показано ниже:
lock (lockObject)
{
...
}
Вкратце: Общение с людьми, которые используют ваш класс.
thisявляется (метафорически) общедоступным, и у вас нет возможности сообщить потребителям, что иногда указанная внешне доступная вещь используется в качестве мьютекса. Пользователь может ожидать, что безопасно использовать ссылку на ваш объект в качестве мьютекса, но это может быть не так, если вы таковым являетесь. Если вместо этого вы используете приватное поле в качестве мьютекса, то вы можете с уверенностью предположить, что люди вне вашего класса не могут (хотя это может сделать ваш класс больше, если вам нужно будет добавить приватное поле специально для использования его в качестве блокировки). .. если это имеет значение).
Я имею в виду, что они могут заблокировать его, если используют отражение, но тогда вы можете посмеиваться, закрывая отчет об ошибке. Они сделали это для себя в тот момент.
Вот гораздо более простая иллюстрация (взятое здесь из Вопроса 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
Вот пример кода, которому проще следовать (IMO): (Будет работать в LinqPad, ссылаться на следующие пространства имен: System.Net и System.Threading.Tasks)
void Main()
{
ClassTest test = new ClassTest();
lock(test)
{
Parallel.Invoke (
() => test.DoWorkUsingThisLock(1),
() => test.DoWorkUsingThisLock(2)
);
}
}
public class ClassTest
{
public void DoWorkUsingThisLock(int i)
{
Console.WriteLine("Before ClassTest.DoWorkUsingThisLock " + i);
lock(this)
{
Console.WriteLine("ClassTest.DoWorkUsingThisLock " + i);
Thread.Sleep(1000);
}
Console.WriteLine("ClassTest.DoWorkUsingThisLock Done " + i);
}
}
Извините, ребята, но я не могу согласиться с аргументом, что блокировка может привести к тупику. Вы путаете две вещи: тупик и голод.
- Вы не можете отменить взаимоблокировку, не прерывая один из потоков, поэтому после попадания в тупик вы не сможете выйти
- Голодание закончится автоматически после того, как один из потоков завершит свою работу
Вот картина, которая иллюстрирует разницу.
Заключение
Вы все еще можете безопасно использовать lock(this) если нить голода не проблема для вас. Вы все еще должны иметь в виду, что, когда поток, который голодает поток, используя lock(this) заканчивается замком, когда ваш объект заблокирован, это в конце концов закончится вечным голодом;)
Вы можете установить правило, которое гласит, что класс может иметь код, который блокирует "this" или любой объект, который создается кодом в классе. Так что это только проблема, если шаблон не соблюдается.
Если вы хотите защитить себя от кода, который не будет следовать этому шаблону, то принятый ответ правильный. Но если следовать шаблону, это не проблема.
Преимущество блокировки (это) - эффективность. Что делать, если у вас есть простой "объект значения", который содержит одно значение. Это просто оболочка, и она создается миллионами раз. Требуя создания частного объекта синхронизации только для блокировки, вы в основном удвоили размер объекта и удвоили количество выделений. Когда производительность имеет значение, это преимущество.
Если вас не волнует количество выделений или объем памяти, избегание блокировки (это) предпочтительнее по причинам, указанным в других ответах.
Потому что любой фрагмент кода, который может видеть экземпляр вашего класса, также может заблокировать эту ссылку. Вы хотите скрыть (инкапсулировать) свой блокирующий объект, чтобы ссылаться на него мог только тот код, который должен ссылаться на него. Ключевое слово this ссылается на текущий экземпляр класса, поэтому любое количество вещей может иметь ссылку на него и может использовать его для синхронизации потоков.
Чтобы было ясно, это плохо, потому что некоторый другой кусок кода может использовать экземпляр класса для блокировки и может помешать вашему коду получить своевременную блокировку или может создать другие проблемы синхронизации потока. Лучший вариант: ничто иное не использует ссылку на ваш класс для блокировки. Средний случай: что-то использует ссылку на ваш класс для блокировки, и это вызывает проблемы с производительностью. В худшем случае: что-то использует ссылку вашего класса для блокировки, и это вызывает очень плохие, очень тонкие, действительно трудные для отладки проблемы.
Будет проблема, если к экземпляру можно получить открытый доступ, потому что могут быть другие запросы, которые могут использовать тот же экземпляр объекта. Лучше использовать приватную / статическую переменную.
Вот почему это не рекомендуется.
Чтобы объяснить это более подробно в соответствии со следующим фрагментом кода, предположим, что вы написали класс (в этом примере), и потребитель вашего класса (кодер по имени «Джон») хочет получить блокировку экземпляра вашего класса (в этом примере) . Он сталкивается с тупиком, потому что он получает блокировку над экземпляром
someObjectи внутри этой блокировки он вызывает метод этого экземпляра (
SomeMethod()), который внутренне получает блокировку точно такого же экземпляра.
Я мог бы написать следующий пример с Task/Thread или без него, и суть взаимоблокировки осталась бы прежней.
Чтобы предотвратить странную ситуацию, когда основной поток завершается, пока его дочерние элементы все еще работают, я использовал
.Wait(). Однако в длительных задачах или в ситуации, когда фрагмент кода выполняется чаще, вы определенно увидите такое же поведение.Хотя Джон применил плохую практику использования экземпляра класса в качестве объекта блокировки, но мы (как разработчик библиотеки классов
SomeClass) должен предотвратить такую ситуацию, просто не используяthisкак блокирующий объект в нашем классе.Вместо этого мы должны объявить простое приватное поле и использовать его как объект блокировки.
using System; using System.Threading; using System.Threading.Tasks; class SomeClass { public void SomeMethod() { //NOTE: Locks over an object that is already locked by the caller. // Hence, the following code-block never executes. lock (this) { Console.WriteLine("Hi"); } } } public class Program { public static void Main() { SomeClass o = new SomeClass(); lock (o) { Task.Run(() => o.SomeMethod()).Wait(); } Console.WriteLine("Finish"); } }