¿Por qué el locking (esto) {…} es malo?

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 la cerradura se mantendrá más tiempo de lo necesario? ¿O hay alguna razón más insidiosa?

Es una mala forma usar this en instrucciones de locking porque generalmente está fuera de tu 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 interlocking, y tener un número desconocido de puntos de entrada de locking 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 comstackdor impondrá restricciones de acceso a él, y encapsulará el mecanismo de locking. Usar this viola la encapsulación al exponer al público parte de su implementación de locking. Tampoco está claro que va a adquirir un locking en this menos que haya sido documentado. Incluso entonces, depender de la documentación para evitar un problema no es óptimo.

Finalmente, existe el error común de que lock(this) realmente 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 un parámetro para lock simplemente sirve como una clave . Si ya se mantiene un locking en esa tecla, no se puede hacer el locking; de lo contrario, el locking está permitido.

Esta es la razón por la que es malo usar cadenas como las claves en las instrucciones de lock , ya que son inmutables y se comparten / se pueden acceder a través de partes de la aplicación. En su lugar, debe usar una variable privada, una instancia de Object lo hará muy 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 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 acceder a su instancia de objeto (es decir, su puntero), entonces también pueden intentar bloquear ese mismo objeto. Ahora es posible que no se den cuenta de que están bloqueando this internamente, 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 , y lo único que realmente necesita bloquear es esa variable miembro. Si bloquea todo el objeto en sus funciones, entonces otras cosas que llaman a esas funciones serán bloqueadas esperando el locking. Si esas funciones no necesitan acceder a la lista de miembros, provocará que otro código espere y ralentice su aplicación sin ningún motivo.

Eche un vistazo a la sincronización de subprocesos de temas de MSDN (Guía de progtwigción C #)

En general, es mejor evitar el locking en un tipo público o en instancias de objetos que escapen al control de su aplicación. Por ejemplo, bloquear (esto) puede ser problemático si se puede acceder a la instancia públicamente, porque el código que está más allá de su control también puede bloquear el objeto. Esto podría crear situaciones de interlocking donde dos o más subprocesos esperan la liberación del mismo objeto . Bloquear en un tipo de datos público, a diferencia de un objeto, puede causar problemas por la misma razón. El locking en cadenas literales es especialmente arriesgado porque las cadenas literales están intercaladas por el tiempo de ejecución de lenguaje común (CLR). Esto significa que hay una instancia de cualquier literal de cadena dada para todo el progtwig, el mismo objeto exacto representa el literal en todos los dominios de aplicación en ejecución, en todos los hilos. Como resultado, un locking colocado en una cadena con los mismos contenidos en cualquier parte del proceso de 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 locking. El tipo de matriz, por ejemplo, proporciona SyncRoot. Muchos tipos de colecciones también proporcionan un miembro SyncRoot.

Sé que este es un hilo viejo, pero debido a que las personas aún pueden ver esto y confiar en él, parece importante señalar que el lock(typeof(SomeObject)) es significativamente peor que el lock(this) . Una vez dicho esto; sinceras felicitaciones a Alan por señalar ese lock(typeof(SomeObject)) es una mala práctica.

Una instancia de System.Type es uno de los objetos generics de grano grueso que hay. Por lo menos, una instancia de System.Type es global para un AppDomain, y .NET puede ejecutar múltiples progtwigs en un AppDomain. Esto significa que dos progtwigs completamente diferentes podrían causar interferencia entre sí incluso hasta el punto de crear un interlocking si ambos intentan obtener un locking de sincronización en la misma instancia de tipo.

Así que el lock(this) no es una forma particularmente robusta, puede causar problemas y siempre debe levantar las cejas por todas las razones citadas. Sin embargo, hay un código ampliamente usado, relativamente respetado y aparentemente estable, como log4net, que usa el patrón de locking (esto) extensivamente, aunque yo personalmente preferiría ver ese cambio de patrón.

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

Por lo que vale.

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

 lock(typeof(SomeObject)) 

Imagine que tiene una secretaria competente en su oficina que es un recurso compartido en el departamento. De vez en cuando, corres 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 tienes que esperar un breve período de tiempo.

Como el cuidado es compartir, su gerente decide que los clientes también pueden usar la secretaria directamente. Pero esto tiene un efecto secundario: un cliente incluso podría reclamarlo mientras trabaja para este cliente y también necesita que ejecute 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 que los clientes los reclamen en primer lugar.

lock(this) es malo como hemos visto. Un objeto externo puede bloquear el objeto y, como no se controla quién está usando la clase, cualquiera puede bloquearlo … Este es el ejemplo exacto como se describió anteriormente. De nuevo, la solución es limitar la exposición del objeto. Sin embargo, si tiene una clase private , protected o internal , ya puede controlar quién está bloqueando su objeto , porque está seguro de haber escrito su código usted mismo. Entonces el mensaje aquí es: no lo expongas como public . Además, asegurarse de que se use un locking en situaciones similares evita los interlockings.

Todo lo contrario de esto es bloquear los recursos que se comparten en el dominio de la aplicación, en el peor de los casos. Es como sacar a su secretaria afuera y permitir que todos los reclamen. El resultado es un caos absoluto, o en términos de código fuente: fue una mala idea; tirarlo y comenzar 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 los mismos contenidos en un dominio de aplicación, existe la posibilidad de que tengan exactamente el mismo puntero. Como el puntero se usa como la tecla de locking, lo que básicamente se obtiene 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();

Bloquear 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 a bloquearlo en otra instancia de una clase.

Mira este código para ver a qué me refiero. Agregue el siguiente código a su progtwig 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 progtwig bloqueado en 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 progtwig bloqueado 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 artículo muy bueno 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 tipo, 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 creaste y no saber 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 correcto de un mutex?

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 locking para que solo el código que necesite referencia pueda referenciarlo. La palabra clave this se refiere a la instancia de la clase actual, por lo que cualquier cantidad de elementos podría tener referencia y podría usarse para sincronizar los hilos.

Para que quede claro, esto es malo porque algún otro fragmento de código podría usar la instancia de clase para bloquear, y podría evitar que tu código obtenga un locking oportuno o podría crear otros problemas de sincronización de subprocesos. Mejor caso: nada más usa una referencia a tu clase para bloquear. Caso medio: algo usa una referencia a tu clase para hacer lockings y causa problemas de rendimiento. Peor caso: algo usa una referencia de tu clase para hacer lockings y causa problemas realmente malos, muy sutiles y realmente difíciles de solucionar.

Aquí hay un código de muestra que es más simple de seguir (IMO): ( Funcionará en LinqPad , referencia siguiente espacios de nombres: System.Net y 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); } } 

Consulte el siguiente enlace que explica por qué el locking (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 del código dentro de la statement de locking como se muestra a continuación:

 lock (lockObject) { ... } 

Aquí hay una ilustración mucho más simple (tomada de la pregunta 34 aquí ) por qué el locking (esto) es malo y puede provocar lockings cuando el consumidor de su clase también intenta bloquear el objeto. A continuación, solo uno de tres hilos puede continuar, los otros dos están en punto muerto.

 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

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: locking y hambre.

  • No puede cancelar el interlocking sin interrumpir uno de los subprocesos, por lo que una vez que se encuentra en un punto muerto, no puede salir.
  • El morir de hambre terminará automáticamente después de que uno de los hilos finalice su trabajo

Aquí hay una imagen que ilustra la diferencia.

Conclusión
Todavía puede usar de forma segura el lock(this) si la falta de hilo no es un problema para usted. Aún debe tener en cuenta que cuando el hilo, que es un hilo que muere de hambre utilizando el lock(this) termina en un locking que tiene su objeto bloqueado, finalmente terminará en inanición eterna;)

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