Por que 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 me perguntando por que? É porque a trava será mantida por mais tempo que o necessário? Ou há algum motivo mais insidioso?

É péssimo usar thisnas instruções de bloqueio, porque geralmente está fora de seu controle quem mais pode estar bloqueando esse objeto.

Para planejar adequadamente as operações paralelas, deve-se tomar cuidado especial para considerar possíveis situações de conflito, e ter um número desconhecido de pontos de entrada de bloqueio dificulta isso. Por exemplo, qualquer pessoa com uma referência ao objeto pode travá-lo sem que o criador / criador do objeto saiba disso. Isso aumenta a complexidade das soluções multithread e pode afetar sua correção.

Um campo privado geralmente é uma opção melhor, pois o compilador aplicará restrições de acesso a ele e encapsulará o mecanismo de bloqueio. O uso thisviola 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 thismenos que tenha sido documentado. Mesmo assim, depender da documentação para evitar um problema é sub-ideal.

Finalmente, existe o equívoco comum que lock(this)realmente modifica o objeto passado como parâmetro e, de alguma forma, o torna somente leitura ou inacessível. Isso é falso . O objeto passado como parâmetro lockapenas serve como chave . Se uma fechadura já estiver sendo mantida nessa chave, a fechadura não poderá ser feita; caso contrário, o bloqueio é permitido.

É por isso que é ruim usar cadeias de caracteres como as chaves nas lockinstruções, pois 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 Objectinstância 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 acessar o thisponteiro da instância do seu objeto (ou seja: seu ), também poderão tentar bloquear o mesmo objeto. Agora eles podem não estar cientes de que você está travando thisinternamente, portanto isso pode causar problemas (possivelmente um impasse)

Além disso, também é uma prática ruim, porque bloqueia "demais"

Por exemplo, você pode ter uma variável de membro de List<int>e a única coisa que realmente precisa bloquear é essa variável de membro. Se você bloquear o objeto 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 desacelere seu aplicativo sem motivo algum.

Dê uma olhada no tópico Sincronização de threads do MSDN (Guia de Programação em C #)

Geralmente, é melhor evitar o bloqueio de um tipo público ou de instâncias de objetos fora do controle do seu aplicativo. Por exemplo, o bloqueio (isso) pode ser problemático se a instância puder ser acessada publicamente, porque o código fora do seu controle também pode bloquear o objeto. Isso pode criar situações de conflito em que dois ou mais encadeamentos aguardam o lançamento do mesmo objeto. Bloquear um tipo de dados público, ao contrário de um objeto, pode causar problemas pelo mesmo motivo. O bloqueio de cadeias literais é especialmente arriscado porque as cadeias literais são internadas pelo Common Language Runtime (CLR). Isso significa que existe uma instância de qualquer literal de seqüência de caracteres fornecida para todo o programa; o mesmo objeto representa o literal em todos os domínios de aplicativo em execução, em todos os threads. Como resultado, um bloqueio colocado em uma sequência com o mesmo conteúdo em qualquer lugar do processo do aplicativo bloqueia todas as instâncias dessa sequência no aplicativo. Como resultado, é melhor bloquear um membro privado ou protegido que não esteja internado. Algumas classes fornecem membros especificamente para o bloqueio. O tipo Array, por exemplo, fornece SyncRoot. Muitos tipos de coleção também fornecem um membro SyncRoot.

Eu sei que esse é um tópico antigo, mas como as pessoas ainda podem procurar e confiar nele, parece importante ressaltar que lock(typeof(SomeObject))é significativamente pior do que lock(this). Tendo dito isto; parabéns sinceros a Alan por apontar que lock(typeof(SomeObject))é uma má prática.

Uma instância de System.Typeé um dos objetos mais genéricos e de granulação grossa que existe. No mínimo, uma instância do System.Type é global para um AppDomain e o .NET pode executar vários programas em um AppDomain. Isso significa que dois programas completamente diferentes podem potencialmente causar interferência um no outro, até o ponto de criar um impasse, se ambos tentarem obter um bloqueio de sincronização na mesma instância de tipo.

Portanto, lock(this)não é uma forma particularmente robusta, pode causar problemas e deve sempre levantar as sobrancelhas por todos os motivos citados. No entanto, existe um código amplamente usado, relativamente respeitado e aparentemente estável, como o log4net, que usa extensivamente o padrão lock (este), mesmo que eu pessoalmente prefira ver esse padrão mudar.

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

Pelo que vale a pena.

... e exatamente os mesmos argumentos se aplicam a essa 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 na direção deles porque tem uma tarefa, apenas esperando que outro colega de trabalho ainda não os tenha reivindicado. Normalmente você só precisa esperar um breve período de tempo.

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

lock(this)é ruim como vimos. Um objeto externo pode travar no objeto e, como você não controla quem está usando a classe, qualquer pessoa pode travá-lo ... Qual é o exemplo exato, conforme descrito acima. Mais uma vez, a solução é limitar a exposição do objeto. No entanto, se você tem uma classe private, protectedou já pode controlar quem está bloqueando seu objeto , porque você tem certeza de que escreveu o seu código. Portanto, a mensagem aqui é: não a exponha como . Além disso, garantir que um bloqueio seja usado em cenários semelhantes evita conflitos.internalpublic

O oposto completo disso é bloquear recursos compartilhados em todo o domínio do aplicativo - o pior cenário. É como colocar sua secretária do lado de fora e permitir que todos lá fora os reivindiquem. O resultado é um caos absoluto - ou em termos de código fonte: foi uma má idéia; 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 indica. Mas há coisas ainda melhores que podemos usar: cordas. A razão é que as cadeias são agrupadas . Em outras palavras: se você tiver duas cadeias de caracteres com o mesmo conteúdo em um domínio de aplicativo, é possível que elas tenham exatamente o mesmo ponteiro. Como o ponteiro é usado como chave de bloqueio, o que você basicamente obtém é sinônimo de "preparar-se para um comportamento indefinido".

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

O bloqueio desse ponteiro pode ser ruim se você estiver bloqueando um recurso compartilhado . Um recurso compartilhado pode ser uma variável estática ou um arquivo no seu computador - ou seja, algo que é compartilhado entre todos os usuários da classe. O motivo é que o ponteiro this conterá uma referência diferente para um local na memória cada vez que sua classe for instanciada. Portanto, bloquear isso em uma instância de classe é diferente de bloquear isso em outra instância de uma classe.

Confira este código para ver o que quero dizer. Adicione o seguinte código ao seu programa principal em um aplicativo de 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 classe 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 programa bloqueando 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 para o tempo de execução do Microsoft® .NET

Excerto:

O problema básico aqui é que você não é o proprietário do objeto de tipo e não sabe quem mais poderia acessá-lo. Em geral, é uma péssima idéia confiar no bloqueio de um objeto que você não criou e não sabe quem mais pode estar acessando. Fazer isso convida a um impasse. A maneira mais segura é bloquear apenas objetos particulares.

Também há uma boa discussão sobre isso aqui: esse é o uso adequado de um mutex?

Aqui está uma ilustração muito mais simples (tirada da pergunta 34 aqui ) por que o bloqueio (isso) é ruim e pode resultar em conflitos quando o consumidor da sua classe também tenta bloquear o objeto. Abaixo, apenas um dos três segmentos pode prosseguir, os outros dois estão em um impasse.

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 o Thread.TryMonitor (com tempo limite) em vez do bloqueio:

            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

Porque qualquer parte do código que pode ver a instância da sua classe também pode bloquear essa referência. Você deseja ocultar (encapsular) seu objeto de bloqueio para que apenas o código que precise fazer referência a ele possa fazer referência a ele. A palavra-chave refere-se à instância atual da classe, portanto, várias coisas podem ter referência a ela e usá-la para sincronizar threads.

Para ficar claro, isso é ruim porque outro pedaço de código pode usar a instância da classe para bloquear e pode impedir que seu código obtenha um bloqueio oportuno ou criar outros problemas de sincronização de encadeamento. Melhor caso: nada mais usa uma referência à sua classe para bloquear. Caso intermediário: algo usa uma referência à sua classe para fazer bloqueios e causa problemas de desempenho. Pior: algo usa uma referência da sua classe para fazer bloqueios e causa problemas muito ruins, sutis e difíceis de depurar.

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

• Você não pode cancelar o impasse sem interromper um dos encadeamentos; portanto, depois de entrar em um impasse, não é possível sair
• A fome termina automaticamente depois que um dos segmentos termina seu trabalho

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

Conclusão
Você ainda pode usar com segurança lock(this)se a falta de thread não for um problema para você. Você ainda deve ter em mente que, quando o thread, que está passando fome usando as lock(this)extremidades de uma fechadura e com o objeto bloqueado, finalmente termina em inanição eterna;)

Consulte o link a seguir, que explica por que bloquear (isso) não é uma boa ideia.

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

Portanto, a solução é adicionar um objeto privado, por exemplo, lockObject à classe e colocar a região de código dentro da instrução lock, como mostrado abaixo:

lock (lockObject)
{
...
}

Aqui está um código de exemplo mais simples de seguir (IMO): ( Funcionará no LinqPad , faça referência aos seguintes namespaces: System.Net e System.Threading.Tasks)

Algo a lembrar é que o bloqueio (x) é basicamente um açúcar sintático e o que ele faz é usar o Monitor.Enter e, em seguida, usa um bloco try, catch, finalmente chamado Call.Exit. Consulte: https://docs.microsoft.com/en-us/dotnet/api/system.threading.monitor.enter (seção de comentários)

ou use a instrução de bloqueio do C # (instrução SyncLock no Visual Basic), que envolve os métodos Enter e Exit em um bloco try ... finalmente.

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

Resultado

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

Observe que o segmento nº 12 nunca termina, pois está totalmente bloqueado.

Você pode estabelecer uma regra que diz que uma classe pode ter um código que bloqueie 'this' ou qualquer objeto que o código da classe instancia. Portanto, é apenas um problema se o padrão não for seguido.

Se você deseja se proteger de um código que não segue esse padrão, a resposta aceita está correta. Mas se o padrão for seguido, não será um problema.

A vantagem do bloqueio (isso) é a eficiência. E se você tiver um "objeto de valor" simples que contém um único valor. É apenas um invólucro e é instanciado milhões de vezes. Ao exigir a criação de um objeto de sincronização privado apenas para o bloqueio, você basicamente duplicou o tamanho do objeto e o número de alocações. Quando o desempenho importa, isso é uma vantagem.

Quando você não se importa com o número de alocações ou espaço ocupado na memória, é preferível evitar o bloqueio (isso) pelos motivos indicados em outras respostas.

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