¿Por qué es malo el bloqueo (esto) {...}?

La documentación de MSDN dice que

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

es "un problema si se puede acceder a la instancia públicamente". Me pregunto por qué ¿Es porque el bloqueo se mantendrá más tiempo del necesario? ¿O hay alguna razón más insidiosa?

Es una mala forma de usar thisen las declaraciones de bloqueo porque generalmente está fuera de su control quién más podría estar bloqueando ese objeto.

Para planificar adecuadamente las operaciones paralelas, se debe tener especial cuidado para considerar posibles situaciones de punto muerto, y tener un número desconocido de puntos de entrada de bloqueo dificulta esto. Por ejemplo, cualquiera con una referencia al objeto puede bloquearlo sin que el diseñador / creador del objeto lo sepa. Esto aumenta la complejidad de las soluciones de subprocesos múltiples y puede afectar su corrección.

Un campo privado suele ser una mejor opción, ya que el compilador impondrá restricciones de acceso y encapsulará el mecanismo de bloqueo. El uso thisviola la encapsulación al exponer parte de su implementación de bloqueo al público. Tampoco está claro que va a adquirir un bloqueo a thismenos que se haya documentado. Incluso entonces, confiar en la documentación para evitar un problema es subóptimo.

Finalmente, existe la idea errónea común que en lock(this)realidad modifica el objeto pasado como parámetro, y de alguna manera lo hace de solo lectura o inaccesible. Esto es falso . El objeto pasado como parámetro locksimplemente sirve como clave . Si ya se está reteniendo un candado en esa llave, no se puede hacer el candado; de lo contrario, se permite el bloqueo.

Esta es la razón por la cual es malo usar cadenas como claves en las lockdeclaraciones, ya que son inmutables y son compartidas / accesibles a través de partes de la aplicación. En su lugar, debe usar una variable privada, una Objectinstancia funcionará bien.

Ejecute el siguiente código C # como ejemplo.

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

Salida de la consola

'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 si las personas pueden llegar al thispuntero de su instancia de objeto (es decir, su ), entonces también pueden intentar bloquear ese mismo objeto. Ahora es posible que no sepan que está bloqueando thisinternamente, por lo que esto puede causar problemas (posiblemente un punto muerto)

Además de esto, también es una mala práctica, porque está bloqueando "demasiado"

Por ejemplo, puede tener una variable miembro de List<int>, y lo único que realmente necesita bloquear es esa variable miembro. Si bloquea todo el objeto en sus funciones, otras cosas que llaman a esas funciones se bloquearán esperando el bloqueo. Si esas funciones no necesitan acceder a la lista de miembros, hará que otro código espere y ralentice su aplicación sin ningún motivo.

Eche un vistazo a la Sincronización de subprocesos de tema de MSDN (Guía de programación de C #)

En general, es mejor evitar el bloqueo en un tipo público, o en instancias de objetos fuera del control de su aplicación. Por ejemplo, el bloqueo (esto) puede ser problemático si se puede acceder a la instancia públicamente, porque el código más allá de su control también puede bloquear el objeto. Esto podría crear situaciones de punto muerto donde dos o más subprocesos esperan la liberación del mismo objeto.. Bloquear un tipo de datos público, en oposición a un objeto, puede causar problemas por la misma razón. El bloqueo en cadenas literales es especialmente arriesgado porque las cadenas literales son internados por Common Language Runtime (CLR). Esto significa que hay una instancia de cualquier literal de cadena dado para todo el programa, exactamente el mismo objeto representa el literal en todos los dominios de aplicación en ejecución, en todos los hilos. Como resultado, un bloqueo colocado en una cadena con el mismo contenido en cualquier parte del proceso de la aplicación bloquea todas las instancias de esa cadena en la aplicación. Como resultado, es mejor bloquear a un miembro privado o protegido que no esté internado. Algunas clases proporcionan miembros específicamente para el bloqueo. El tipo de matriz, por ejemplo, proporciona SyncRoot. Muchos tipos de colección también proporcionan un miembro SyncRoot.

Sé que este es un hilo viejo, pero debido a que la gente todavía puede buscarlo y confiar en él, parece importante señalar que lock(typeof(SomeObject))es significativamente peor que eso lock(this). Una vez dicho esto; sinceras felicitaciones a Alan por señalar que lock(typeof(SomeObject))es una mala práctica.

Una instancia de System.Typees uno de los objetos más genéricos y de grano grueso que hay. Como mínimo, una instancia de System.Type es global para un AppDomain, y .NET puede ejecutar múltiples programas en un AppDomain. Esto significa que dos programas completamente diferentes podrían causar interferencia entre sí, incluso hasta el punto de crear un punto muerto si ambos intentan obtener un bloqueo de sincronización en la misma instancia de tipo.

Por lock(this)lo tanto, no es una forma particularmente robusta, puede causar problemas y siempre debe levantar las cejas por todas las razones mencionadas. Sin embargo, existe un código ampliamente utilizado, relativamente respetado y aparentemente estable, como log4net, que usa ampliamente el patrón de bloqueo (este), a pesar de que personalmente preferiría ver ese cambio de patrón.

Pero lock(typeof(SomeObject))abre una lata de gusanos completamente nueva y mejorada.

Por lo que vale.

... y los mismos argumentos exactos se aplican a esta construcción también:

lock(typeof(SomeObject))

Imagine que tiene una secretaria experta en su oficina que es un recurso compartido en el departamento. De vez en cuando, te apresuras hacia ellos porque tienes una tarea, solo para esperar que otro de tus compañeros de trabajo no los haya reclamado. Por lo general, solo tiene que esperar un breve período de tiempo.

Como cuidar es compartir, su gerente decide que los clientes también pueden usar la secretaria directamente. Pero esto tiene un efecto secundario: un cliente puede incluso reclamarlos mientras trabaja para este cliente y también los necesita para ejecutar parte de las tareas. Se produce un punto muerto, porque reclamar ya no es una jerarquía. Esto podría haberse evitado por completo al no permitir a los clientes reclamarlos en primer lugar.

lock(this)Es malo como hemos visto. Un objeto externo puede bloquearse en el objeto y, dado que usted no controla quién está usando la clase, cualquiera puede bloquearlo ... Que es el ejemplo exacto como se describe anteriormente. Nuevamente, la solución es limitar la exposición del objeto. Sin embargo, si tiene una private, protectedo internalclase en la que ya se podía controlar quién está bloqueando en su objeto , porque está seguro de que usted ha escrito su código usted mismo. Entonces el mensaje aquí es: no lo exponga como public. Además, garantizar que se use un bloqueo en situaciones similares evita los puntos muertos.

Todo lo contrario de esto es bloquear los recursos que se comparten en todo el dominio de la aplicación, el peor de los casos. Es como poner a tu secretaria afuera y permitir que todos los reclamen. El resultado es un caos total, o en términos de código fuente: fue una mala idea; tíralo y comienza de nuevo. ¿Entonces cómo hacemos eso?

Los tipos se comparten en el dominio de la aplicación como la mayoría de las personas aquí señalan. Pero hay cosas aún mejores que podemos usar: cadenas. La razón es que las cadenas se agrupan . En otras palabras: si tiene dos cadenas que tienen el mismo contenido en un dominio de aplicación, existe la posibilidad de que tengan exactamente el mismo puntero. Dado que el puntero se usa como la tecla de bloqueo, lo que básicamente obtienes es un sinónimo de "prepararse para un comportamiento indefinido".

Del mismo modo, no debe bloquear objetos WCF, HttpContext.Current, Thread.Current, Singletons (en general), etc. ¿La forma más fácil de evitar todo esto? private [static] object myLock = new object();

El bloqueo en este puntero puede ser malo si está bloqueando un recurso compartido . Un recurso compartido puede ser una variable estática o un archivo en su computadora, es decir, algo que se comparte entre todos los usuarios de la clase. La razón es que este puntero contendrá una referencia diferente a una ubicación en la memoria cada vez que se crea una instancia de su clase. Por lo tanto, bloquear esto en una instancia de una clase es diferente de bloquear esto en otra instancia de una clase.

Mira este código para ver a qué me refiero. Agregue el siguiente código a su programa principal en una aplicación de consola:

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

Crea una nueva clase como la siguiente.

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

Aquí hay una ejecución del programa bloqueando esto .

   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

Aquí hay una ejecución del programa que se bloquea en 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

Hay un muy buen artículo al respecto http://bytes.com/topic/c-sharp/answers/249277-dont-lock-type-objects por Rico Mariani, arquitecto de rendimiento para el tiempo de ejecución de Microsoft® .NET

Extracto:

El problema básico aquí es que no posee el objeto type, y no sabe quién más podría acceder a él. En general, es una muy mala idea confiar en bloquear un objeto que no creó y no sabe a quién más podría estar accediendo. Hacerlo invita a un punto muerto. La forma más segura es bloquear solo objetos privados.

También hay una buena discusión sobre esto aquí: ¿Es este el uso adecuado de un mutex?

Aquí hay una ilustración mucho más simple (tomada de la Pregunta 34 aquí ) por qué el bloqueo (esto) es malo y puede provocar puntos muertos cuando el consumidor de su clase también intenta bloquear el objeto. A continuación, solo uno de los tres subprocesos puede continuar, los otros dos están bloqueados.

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 evitarlo, este tipo usó Thread. TryMonitor (con tiempo de espera) en lugar 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

Porque cualquier fragmento de código que pueda ver la instancia de su clase también puede bloquear esa referencia. Desea ocultar (encapsular) su objeto de bloqueo para que solo el código que necesita referenciarlo pueda hacer referencia. La palabra clave this se refiere a la instancia de clase actual, por lo que cualquier cantidad de cosas podría tener referencia a ella y podría usarla para realizar la sincronización de subprocesos.

Para ser claros, esto es malo porque algún otro fragmento de código podría usar la instancia de clase para bloquear y podría impedir que su código obtenga un bloqueo oportuno o podría crear otros problemas de sincronización de subprocesos. Mejor caso: nada más utiliza una referencia a su clase para bloquear. Caso medio: algo usa una referencia a su clase para hacer bloqueos y causa problemas de rendimiento. El peor de los casos: algo usa una referencia de su clase para hacer bloqueos y causa problemas realmente malos, muy sutiles y difíciles de depurar.

Lo siento chicos, pero no puedo estar de acuerdo con el argumento de que bloquear esto podría causar un punto muerto. Estás confundiendo dos cosas: estancamiento y hambre.

• No puede cancelar el punto muerto sin interrumpir uno de los hilos, por lo que después de entrar en un punto muerto no puede salir
• El hambre desaparecerá automáticamente después de que uno de los hilos termine su trabajo

Aquí hay una imagen que ilustra la diferencia.

Conclusión
Todavía puede usarlo con seguridad lock(this)si el hambre de hilo no es un problema para usted. Aún debe tener en cuenta que cuando el hilo, que está usando un hilo muerto de hambre, lock(this)termina en una cerradura que tiene su objeto bloqueado, finalmente terminará en un hambre eterna;)

Consulte el siguiente enlace que explica por qué bloquear (esto) no es una buena idea.

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

Entonces, la solución es agregar un objeto privado, por ejemplo, lockObject a la clase y colocar la región de código dentro de la declaración de bloqueo como se muestra a continuación:

lock (lockObject)
{
...
}

Aquí hay un código de muestra que es más fácil de seguir (IMO): ( Funcionará en LinqPad , referencia los siguientes espacios de nombres: System.Net y System.Threading.Tasks)

Algo para recordar es que el bloqueo (x) básicamente es azúcar sintáctico y lo que hace es usar Monitor.Enter y luego usa try, catch, finalmente block para llamar a Monitor.Exit. Ver: https://docs.microsoft.com/en-us/dotnet/api/system.threading.monitor.enter (sección de comentarios)

o use la instrucción de bloqueo C # (instrucción SyncLock en Visual Basic), que envuelve los métodos Enter y Exit en un intento ... finalmente bloquee.

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

Salida

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

Tenga en cuenta que el hilo # 12 nunca termina ya que está completamente bloqueado.

Puede establecer una regla que diga que una clase puede tener código que se bloquea en 'esto' o cualquier objeto que el código en la clase instancia. Por lo tanto, solo es un problema si no se sigue el patrón.

Si desea protegerse del código que no seguirá este patrón, entonces la respuesta aceptada es correcta. Pero si se sigue el patrón, no es un problema.

La ventaja del bloqueo (esto) es la eficiencia. ¿Qué sucede si tiene un simple "objeto de valor" que contiene un solo valor? Es solo una envoltura, y se instancia millones de veces. Al requerir la creación de un objeto de sincronización privado solo para el bloqueo, básicamente ha duplicado el tamaño del objeto y ha duplicado el número de asignaciones. Cuando el rendimiento importa, esta es una ventaja.

Cuando no le importa la cantidad de asignaciones o la huella de memoria, es preferible evitar el bloqueo (esto) por las razones indicadas en otras respuestas.

Habrá un problema si se puede acceder a la instancia públicamente porque podría haber otras solicitudes que podrían estar utilizando la misma instancia de objeto. Es mejor usar variable privada / estática.