Por que o lock (this) {…} é ruim?

A documentação do MSDN diz que

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

é “um problema se a instância puder ser acessada publicamente”. Eu estou querendo saber porque? Será porque a fechadura será mantida por mais tempo do que o necessário? Ou há algum motivo mais insidioso?

É uma má forma de usar this em instruções de bloqueio porque geralmente está fora de seu controle quem mais pode estar bloqueando esse object.

Para planejar adequadamente as operações paralelas, um cuidado especial deve ser tomado para considerar possíveis situações de deadlock, e ter um número desconhecido de pontos de input de bloqueio dificulta isso. Por exemplo, qualquer um com uma referência ao object pode bloqueá-lo sem que o designer / criador do object saiba disso. Isso aumenta a complexidade das soluções multiencadeadas e pode afetar sua exatidão.

Um campo privado geralmente é uma opção melhor, pois o compilador aplicará restrições de access a ele e encapsulará o mecanismo de bloqueio. Usar this viola o encapsulamento expondo parte da sua implementação de bloqueio ao público. Também não está claro que você estará adquirindo um bloqueio a menos que tenha sido documentado. Mesmo assim, confiar na documentação para evitar um problema é insatisfatório.

Por fim, há o equívoco comum de que o lock(this) realmente modifica o object passado como parâmetro e, de alguma forma, torna-o somente leitura ou inacessível. Isso é falso . O object passado como um parâmetro para lock serve apenas como uma chave . Se um bloqueio já estiver sendo mantido nessa chave, a trava não poderá ser feita; caso contrário, o bloqueio é permitido.

É por isso que é ruim usar strings como chaves em instruções de lock , já que elas são imutáveis ​​e são compartilhadas / acessíveis em partes do aplicativo. Você deve usar uma variável privada em vez disso, uma instância de Object será bem.

Execute o seguinte código C # como um exemplo.

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

Saída do console

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

Porque se as pessoas puderem obter o ponteiro da instância do object (isto é: seu this ), elas também poderão tentar bloquear o mesmo object. Agora eles podem não estar cientes de que você está bloqueando this internamente, então isso pode causar problemas (possivelmente um deadlock)

Além disso, também é uma prática ruim, porque está bloqueando “demais”

Por exemplo, você pode ter uma variável de membro de List , e a única coisa que você realmente precisa bloquear é aquela variável de membro. Se você bloquear o object inteiro em suas funções, outras coisas que chamam essas funções serão bloqueadas aguardando o bloqueio. Se essas funções não precisarem acessar a lista de membros, você estará fazendo com que outro código aguarde e retarde seu aplicativo sem nenhum motivo.

Dê uma olhada na Sincronização de Tópico do Tópico do MSDN (Guia de Programação C #)

Geralmente, é melhor evitar o bloqueio em um tipo público ou em instâncias de objects além do controle de seu aplicativo. Por exemplo, o bloqueio (este) pode ser problemático se a instância puder ser acessada publicamente, porque o código além do seu controle também pode ser bloqueado no object. Isso pode criar situações de deadlock em que dois ou mais threads aguardam o lançamento do mesmo object . Bloquear um tipo de dados públicos, ao contrário de um object, pode causar problemas pelo mesmo motivo. Bloquear em cadeias literais é especialmente arriscado porque strings literais são internadas pelo common language runtime (CLR). Isso significa que há uma instância de qualquer string fornecida para o programa inteiro, exatamente o mesmo object representa o literal em todos os domínios de aplicativo em execução, em todos os threads. Como resultado, um bloqueio colocado em uma seqüência de caracteres com o mesmo conteúdo em qualquer lugar no processo de aplicativo bloqueia todas as instâncias dessa seqüência no aplicativo. Como resultado, é melhor bloquear um membro particular ou protegido que não esteja internado. Algumas classs fornecem membros especificamente para bloqueio. O tipo Array, por exemplo, fornece SyncRoot. Muitos tipos de coleção também fornecem um membro SyncRoot.

Eu sei que este é um thread antigo, mas como as pessoas ainda podem procurar e confiar nele, parece importante ressaltar que lock(typeof(SomeObject)) é significativamente pior que lock(this) . Tendo dito isto; Parabéns sinceros para Alan por apontar que lock(typeof(SomeObject)) é uma prática ruim.

Uma instância de System.Type é um dos objects mais genéricos e com granulação grossa existente. No mínimo, uma instância de System.Type é global para um AppDomain e o .NET pode executar vários programas em um AppDomain. Isso significa que dois programas totalmente diferentes podem causar interferência um no outro, até mesmo ao ponto de criar um deadlock se ambos tentarem obter um bloqueio de synchronization na mesma instância de tipo.

Então, lock(this) não é uma forma particularmente robusta, pode causar problemas e deve sempre levantar as sobrancelhas por todas as razões citadas. No entanto, há um código amplamente utilizado, relativamente respeitado e aparentemente estável como o log4net, que usa o padrão lock (this) extensivamente, embora eu pessoalmente prefira ver essa mudança de padrão.

Mas o lock(typeof(SomeObject)) abre uma lata de worms totalmente nova e aprimorada.

Por que vale a pena.

… e exatamente os mesmos argumentos se aplicam a esta construção também:

 lock(typeof(SomeObject)) 

Imagine que você tenha uma secretária qualificada em seu escritório que seja um recurso compartilhado no departamento. De vez em quando, você corre em direção a eles porque tem uma tarefa, apenas para esperar que outro de seus colegas de trabalho não os tenha reivindicado. Normalmente você só tem que esperar por um breve período de tempo.

Como o cuidado é compartilhar, seu gerente decide que os clientes também podem usar a secretária diretamente. Mas isso tem um efeito colateral: um cliente pode reivindicá-lo enquanto você trabalha para esse cliente e também é necessário que ele execute parte das tarefas. Um deadlock ocorre porque a reivindicação não é mais uma hierarquia. Isso poderia ter sido evitado todos juntos, não permitindo que os clientes os reivindicassem em primeiro lugar.

lock(this) é ruim como vimos. Um object externo pode bloquear o object e, como você não controla quem está usando a class, qualquer pessoa pode bloqueá-lo … Qual é o exemplo exato, conforme descrito acima. Mais uma vez, a solução é limitar a exposição do object. No entanto, se você tiver uma aula private , protected ou internal você já poderá controlar quem está bloqueando seu object , porque você tem certeza de que você mesmo escreveu seu código. Então a mensagem aqui é: não exponha isso como public . Além disso, garantir que um bloqueio seja usado em cenários semelhantes evita conflitos.

O oposto disso é bloquear resources que são compartilhados em todo o domínio do aplicativo – o pior cenário possível. É como colocar sua secretária do lado de fora e permitir que todo mundo lá fora as reivindique. O resultado é um caos total – ou em termos de código fonte: foi uma má ideia; jogue fora e comece de novo. Então, como fazemos isso?

Os tipos são compartilhados no domínio do aplicativo, como a maioria das pessoas aponta aqui. Mas há coisas ainda melhores que podemos usar: strings. A razão é que as strings são agrupadas . Em outras palavras: se você tiver duas sequências que tenham o mesmo conteúdo em um domínio de aplicativo, há uma chance de que elas tenham exatamente o mesmo ponteiro. Como o ponteiro é usado como a chave de bloqueio, o que você basicamente obtém é um sinônimo para “preparar para comportamento indefinido”.

Da mesma forma, você não deve bloquear objects do WCF, HttpContext.Current, Thread.Current, Singletons (em geral), etc. A maneira mais fácil de evitar tudo isso? private [static] object myLock = new object();

Bloquear no ponteiro pode ser ruim se você estiver bloqueando um recurso compartilhado . Um recurso compartilhado pode ser uma variável estática ou um arquivo em seu computador – isto é, algo que é compartilhado entre todos os usuários da class. A razão é que o ponteiro this conterá uma referência diferente a um local na memory toda vez que sua class for instanciada. Portanto, bloquear essa instância em uma vez de uma class é diferente de travar isso em outra instância de uma class.

Confira este código para ver o que quero dizer. Adicione o seguinte código ao seu programa principal em um aplicativo do console:

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

Crie uma nova turma como a abaixo.

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

Aqui está uma execução do programa bloqueando isso .

  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 

Aqui está uma execução do bloqueio do programa no 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 

Há um artigo muito bom sobre isso http://bytes.com/topic/c-sharp/answers/249277-dont-lock-type-objects por Rico Mariani, arquiteto de desempenho do tempo de execução do Microsoft® .NET

Excerto:

O problema básico aqui é que você não possui o object type e não sabe quem mais poderia acessá-lo. Em geral, é uma péssima ideia confiar no bloqueio de um object que você não criou e não sabe quem mais poderia estar acessando. Fazê-lo convida o impasse. A maneira mais segura é apenas bloquear objects privados.

Há também uma boa discussão sobre isso aqui: este é o uso adequado de um mutex?

Porque qualquer parte do código que pode ver a instância da sua class também pode bloquear essa referência. Você deseja ocultar (encapsular) seu object de bloqueio para que apenas o código que precisa referenciá-lo possa referenciá-lo. A palavra-chave this refere-se à instância de class atual, portanto, qualquer número de coisas poderia ter referência a ela e poderia usá-la para fazer a synchronization de threads.

Para ser claro, isso é ruim porque algum outro pedaço de código poderia usar a instância de class para bloquear e impedir que seu código obtivesse um bloqueio oportuno ou poderia criar outros problemas de synchronization de thread. Melhor caso: nada mais usa uma referência à sua class para bloquear. Caso intermediário: algo usa uma referência à sua class para fazer bloqueios e causa problemas de desempenho. Pior caso: algo usa uma referência de sua class para fazer bloqueios e causa problemas realmente ruins, realmente sutis, realmente difíceis de depurar.

Aqui está um exemplo de código que é mais simples de seguir (IMO): ( Funcionará no LinqPad , referência a seguir a namespaces: System.Net e 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); } } 

Por favor, consulte o seguinte link que explica porque o bloqueio (isto) não é uma boa idéia.

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

Portanto, a solução é adicionar um object privado, por exemplo, lockObject à class e colocar a região de código dentro da instrução de bloqueio, conforme mostrado abaixo:

 lock (lockObject) { ... } 

Aqui está uma ilustração muito mais simples (tirada da Questão 34 aqui ) porque o lock (this) é ruim e pode resultar em deadlocks quando o consumidor da sua class também tenta bloquear o object. Abaixo, apenas um dos três threads pode prosseguir, os outros dois estão em deadlock.

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

Para contornar, esse cara usou Thread.TryMonitor (com o tempo limite) em vez de bloquear:

  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

Desculpe pessoal, mas não posso concordar com o argumento de que bloquear isso pode causar um impasse. Você está confundindo duas coisas: deadlocking e starving.

  • Você não pode cancelar o deadlock sem interromper um dos threads então depois que você entrar em um deadlock você não pode sair
  • O faminto terminará automaticamente depois que um dos threads terminar seu trabalho

Aqui está uma foto que ilustra a diferença.

Conclusão
Você ainda pode usar com segurança o lock(this) se a privação de threads não for um problema para você. Você ainda tem que ter em mente que quando o thread, que é thread faminto usando lock(this) terminar em um lock com seu object bloqueado, ele finalmente terminará em fome eterna;)

Haverá um problema se a instância puder ser acessada publicamente porque pode haver outras solicitações que possam estar usando a mesma instância de object. É melhor usar a variável privada / estática.