GOTCHA #57 Locking on globally visible objects is too sweepingIn Gotcha #56, "Calling Type.GetType() may not return what you expect," I discussed options for synchronizing threads. While the last solution presented there seems to work for that specific example, there is still a problem. Locking the type metadata to define a critical section is much too sweeping. If all static/Shared methods synchronize on the type metadata, then all access to the class methods is one-at-a-time, even if it doesn't need to be. This certainly provides thread safety, but sacrifices concurrency in ways that are neither necessary nor desirable in most cases. Consider Example 7-12. Example 7-12. Consequence of locking the Type metadataC# (SynchWithIntent) //Bacteria.cs using System; using System.Threading; namespace SynchOnType { public class Bacteria { private static int bacteriaCount; private static void IncreaseCount() { Console.WriteLine( "IncreaseCount called by {0} at {1}", AppDomain.GetCurrentThreadId(), DateTime.Now.ToLongTimeString()); bacteriaCount++; Thread.Sleep(2000); // Used for illustration purpose } public Bacteria() { lock(typeof(Bacteria)) { IncreaseCount(); } } } } //Test.cs using System; using System.Threading; namespace SynchOnType { class Test { private static void Worker() { Console.WriteLine("In thread {0}", AppDomain.GetCurrentThreadId()); Bacteria aBacteria = new Bacteria(); } [STAThread] static void Main(string[] args) { Thread thread1 = new Thread(new ThreadStart(Worker)); Thread thread2 = new Thread(new ThreadStart(Worker)); lock(typeof(Bacteria)) { Console.WriteLine("Starting threads at {0}", DateTime.Now.ToLongTimeString()); thread1.Start(); thread2.Start(); Thread.Sleep(3000); } } } } VB.NET (SynchWithIntent) Imports System.Threading 'Bacteria.vb Public Class Bacteria Private Shared bacteriaCount As Integer Private Shared Sub IncreaseCount() Console.WriteLine( _ "IncreaseCount called by {0} at {1}", _ AppDomain.GetCurrentThreadId(), _ DateTime.Now.ToLongTimeString()) bacteriaCount += 1 Thread.Sleep(2000) ' Used for illustration purpose End Sub Public Sub New() SyncLock GetType(Bacteria) IncreaseCount() End SyncLock End Sub End Class 'Test.vb Imports System.Threading Module Test Private Sub Worker() Console.WriteLine("In thread {0}", _ AppDomain.GetCurrentThreadId()) Dim aBacteria As New Bacteria End Sub Public Sub Main() Dim thread1 As New Thread(AddressOf Worker) Dim thread2 As New Thread(AddressOf Worker) SyncLock (GetType(Bacteria)) Console.WriteLine("Starting threads at {0}", _ DateTime.Now.ToLongTimeString()) thread1.Start() thread2.Start() Thread.Sleep(3000) End SyncLock End Sub End Module When you execute the above program, since the Main() method holds a lock on the Bacteria type for 3 seconds, the creation of the Bacteria objects is delayed until Main() lets go of that lock. One reason why a client of the Bacteria class might hold a lock on its metadata is so it can call multiple static/Shared methods in a thread-safe way. For whatever reason, if Main() or any other method in the application grabs a lock on the Bacteria metadata, the execution of the constructors (and all other methods) is delayed. This is shown in the output in Figure 7-13. Figure 7-13. Output from Example 7-12The problem here is that Bacteria's constructor claims a lock on the Bacteria type in order to synchronize the execution of its static/Shared method. The static/Shared method and the implementation of the constructor are purely private to the class. However, you rely on a publicly visible object (the metadata of Bacteria) to synchronize. You can lock much more locally and precisely. Let's look at a modified implementation in Example 7-13. Example 7-13. Synchronizing locally and preciselyC# (SynchWithIntent) //Bacteria.cs using System; namespace SynchOnType { public class Bacteria { private static int bacteriaCount; private static object theIncrementCountLock = new Object(); private static void IncreaseCount() { Console.WriteLine( "IncreaseCount called by {0} at {1}", AppDomain.GetCurrentThreadId(), DateTime.Now.ToLongTimeString()); bacteriaCount++; System.Threading.Thread.Sleep(2000); // Used for illustration purpose } public Bacteria() { lock(theIncrementCountLock) { IncreaseCount(); } } } } VB.NET (SynchWithIntent) Imports System.Threading 'Bacteria.vb Public Class Bacteria Private Shared bacteriaCount As Integer Private Shared theIncrementCountLock As New Object Private Shared Sub IncreaseCount() Console.WriteLine( _ "IncreaseCount called by {0} at {1}", _ AppDomain.GetCurrentThreadId(), _ DateTime.Now.ToLongTimeString()) bacteriaCount += 1 Thread.Sleep(2000) ' Used for illustration purpose End Sub Public Sub New() SyncLock theIncrementCountLock IncreaseCount() End SyncLock End Sub End Class In this case, you have created a dummy object (theIncrementCountLock refers to it) within the Bacteria class. In the constructor, you lock this object. Note that the dummy lock-facilitator object is private, so no method outside the class can claim a lock with it. There are two advantages to this approach. One, code in other classes can't affect the concurrency of the constructor, because they can't see theIncrementCountLock. Second, suppose you have two different tasks, call them Task A and Task B. Each of these tasks must be executed one-at-a-time. But if Task A doesn't access any resources that Task B needs, and Task B touches none of Task A's resources, there is no problem with their running concurrently. You can realize this most effectively using two private dummy-lock objects. The output after this change is shown in Figure 7-14. Figure 7-14. Output after change in Example 7-13For a great discussion on possible deadlock consequences, refer to the article "Don't Lock Type Objects!" in the "on the web" section of the Appendix. IN A NUTSHELLDo not synchronize within your class on publicly visible objects. Synchronize precisely by synchronizing locally. SEE ALSOGotcha #20, "Singleton isn't guaranteed process-wide," Gotcha #56, "Calling Type.GetType() may not return what you expect," Gotcha #62, "Accessing WinForm controls from arbitrary threads is dangerous," and Gotcha #64, "Raising events lacks thread-safety." |