[asp.net] Get rid of a possible (although unlikely) race condition when acquiring locks

The patch removes a small race condition where a boolean flag is set after acquiring a lock to
indicate to code executing in the finally {} block that it should release the lock. The boolean
variable is now removed and the lock is released unconditionally. It carries a potential to
throw an exception when the lock is not held, but it's better than to fail to release it and
lead the application to a deadlock.

mcs/class/System.Web/System.Web.Caching/Cache.cs

@@ -42,6 +42,21 @@ namespace System.Web.Caching
 	{
 		public static readonly DateTime NoAbsoluteExpiration = DateTime.MaxValue;
 		public static readonly TimeSpan NoSlidingExpiration = TimeSpan.Zero;
+
+		// cacheLock will be released in the code below without checking whether it was
+		// actually acquired. The API doesn't offer a reliable way to check whether the lock
+		// is being held by the current thread and since Mono does't implement CER
+		// (Constrained Execution Regions -
+		// http://msdn.microsoft.com/en-us/library/ms228973.aspx) currently, we have no
+		// reliable way of recording the information that the lock has been successfully
+		// acquired.
+		// It can happen that a Thread.Abort occurs while acquiring the lock and the lock
+		// isn't  actually held. In this case the attempt to release a lock will throw an
+		// exception. It's better than a race of setting a boolean flag  after acquiring the
+		// lock and then relying upon it here to release it - that may cause a deadlock
+		// should we fail to release the lock  which was successfully acquired but
+		// Thread.Abort happened right after that during the stloc instruction to set the
+		// boolean flag. Once CERs are supported we can use the boolean flag reliably.
 		ReaderWriterLockSlim cacheLock;
 		Dictionary <string, CacheItem> cache;
 		CacheItemPriorityQueue timedItems;
@@ -153,19 +168,17 @@ namespace System.Web.Caching
 		{
 			if (key == null)
 				throw new ArgumentNullException ("key");
-
-			bool locked = false;
+			
 			try {
 				cacheLock.EnterWriteLock ();
-				locked = true;
 				CacheItem it = GetCacheItem (key);
 
 				if (it != null)
 					return it.Value;
 				Insert (key, value, dependencies, absoluteExpiration, slidingExpiration, priority, onRemoveCallback, null, false);
 			} finally {
-				if (locked)
-					cacheLock.ExitWriteLock ();
+				// See comment at the top of the file, above cacheLock declaration
+				cacheLock.ExitWriteLock ();
 			}
 				
 			return null;
@@ -173,10 +186,8 @@ namespace System.Web.Caching
 		
 		public object Get (string key)
 		{
-			bool locked = false;
 			try {
 				cacheLock.EnterUpgradeableReadLock ();
-				locked = true;
 				CacheItem it = GetCacheItem (key);
 				if (it == null)
 					return null;
@@ -187,6 +198,7 @@ namespace System.Web.Caching
 						if (!NeedsUpdate (it, CacheItemUpdateReason.DependencyChanged, false))
 							Remove (it.Key, CacheItemRemovedReason.DependencyChanged, false, true);
 					} finally {
+						// See comment at the top of the file, above cacheLock declaration
 						cacheLock.ExitWriteLock ();
 					}
 					
@@ -212,6 +224,7 @@ namespace System.Web.Caching
 							if (!NeedsUpdate (it, CacheItemUpdateReason.Expired, false))
 								Remove (key, CacheItemRemovedReason.Expired, false, true);
 						} finally {
+							// See comment at the top of the file, above cacheLock declaration
 							cacheLock.ExitWriteLock ();
 						}
 
@@ -221,9 +234,8 @@ namespace System.Web.Caching
 				
 				return it.Value;
 			} finally {
-				if (locked) {
-					cacheLock.ExitUpgradeableReadLock ();
-				}
+				// See comment at the top of the file, above cacheLock declaration
+				cacheLock.ExitUpgradeableReadLock ();
 			}
 		}
 		
@@ -282,20 +294,17 @@ namespace System.Web.Caching
 		
 		internal void SetItemTimeout (string key, DateTime absoluteExpiration, TimeSpan slidingExpiration, bool doLock)
 		{
-			CacheItem ci = null;
-			bool locked = false;
-			
+			CacheItem ci = null;			
 			try {
-				if (doLock) {
+				if (doLock)
 					cacheLock.EnterWriteLock ();
-					locked = true;
-				}
 				
 				ci = GetCacheItem (key);
 				if (ci != null)
 					SetItemTimeout (ci, absoluteExpiration, slidingExpiration, ci.OnRemoveCallback, null, key, false);
 			} finally {
-				if (locked) {
+				if (doLock) {
+					// See comment at the top of the file, above cacheLock declaration
 					cacheLock.ExitWriteLock ();
 				}
 			}
@@ -317,12 +326,9 @@ namespace System.Web.Caching
 			ci.OnRemoveCallback = onRemoveCallback;
 			ci.OnUpdateCallback = onUpdateCallback;
 			
-			bool locked = false;
 			try {
-				if (doLock) {
+				if (doLock)
 					cacheLock.EnterWriteLock ();
-					locked = true;
-				}
 				
 				if (ci.Timer != null) {
 					ci.Timer.Dispose ();
@@ -336,7 +342,8 @@ namespace System.Web.Caching
 				if (!disableExpiration && ci.AbsoluteExpiration != NoAbsoluteExpiration)
 					EnqueueTimedItem (ci);
 			} finally {
-				if (locked) {
+				if (doLock) {
+					// See comment at the top of the file, above cacheLock declaration
 					cacheLock.ExitWriteLock ();
 				}
 			}
@@ -376,16 +383,14 @@ namespace System.Web.Caching
 		object Remove (string key, CacheItemRemovedReason reason, bool doLock, bool invokeCallback)
 		{
 			CacheItem it = null;
-			bool locked = false;
 			try {
-				if (doLock) {
+				if (doLock)
 					cacheLock.EnterWriteLock ();
-					locked = true;
-				}
 				
 				it = RemoveCacheItem (key);
 			} finally {
-				if (locked) {
+				if (doLock) {
+					// See comment at the top of the file, above cacheLock declaration
 					cacheLock.ExitWriteLock ();
 				}
 			}
@@ -424,10 +429,8 @@ namespace System.Web.Caching
 		internal void InvokePrivateCallbacks ()
 		{
 			CacheItemRemovedReason reason = CacheItemRemovedReason.Removed;
-			bool locked = false;
 			try {
 				cacheLock.EnterReadLock ();
-				locked = true;
 				foreach (string key in cache.Keys) {
 					CacheItem item = GetCacheItem (key);
 					if (item.Disabled)
@@ -442,25 +445,21 @@ namespace System.Web.Caching
 					}
 				}
 			}  finally {
-				if (locked) {
-					cacheLock.ExitReadLock ();
-				}
+				// See comment at the top of the file, above cacheLock declaration
+				cacheLock.ExitReadLock ();
 			}
 		}
 
 		public IDictionaryEnumerator GetEnumerator ()
 		{
 			ArrayList list = new ArrayList ();
-			bool locked = false;
 			try {
 				cacheLock.EnterReadLock ();
-				locked = true;
 				foreach (CacheItem it in cache.Values)
 					list.Add (it);
 			} finally {
-				if (locked) {
-					cacheLock.ExitReadLock ();
-				}
+				// See comment at the top of the file, above cacheLock declaration
+				cacheLock.ExitReadLock ();
 			}
 			
 			return new CacheItemEnumerator (list);
@@ -478,13 +477,9 @@ namespace System.Web.Caching
 
 		bool NeedsUpdate (CacheItem item, CacheItemUpdateReason reason, bool needLock)
 		{
-			bool locked = false;
-			
 			try {
-				if (needLock) {
+				if (needLock)
 					cacheLock.EnterWriteLock ();
-					locked = true;
-				}
 				
 				if (item == null || item.OnUpdateCallback == null)
 					return false;
@@ -525,8 +520,10 @@ namespace System.Web.Caching
 			} catch (Exception) {
 				return false;
 			} finally {
-				if (locked)
+				if (needLock) {
+					// See comment at the top of the file, above cacheLock declaration
 					cacheLock.ExitWriteLock ();
+				}
 			}
 		}
 		
@@ -534,11 +531,9 @@ namespace System.Web.Caching
 		{
 			DateTime now = DateTime.Now;
 			CacheItem item = timedItems.Peek ();
-			bool locked = false;
 
 			try {
 				cacheLock.EnterWriteLock ();
-				locked = true;
 
 				while (item != null) {
 					if (!item.Disabled && item.ExpiresAt > now.Ticks)
@@ -554,8 +549,8 @@ namespace System.Web.Caching
 					item = timedItems.Peek ();
 				}
 			} finally {
-				if (locked)
-					cacheLock.ExitWriteLock ();
+				// See comment at the top of the file, above cacheLock declaration
+				cacheLock.ExitWriteLock ();
 			}
 
 			if (item != null) {
@@ -574,10 +569,8 @@ namespace System.Web.Caching
 		internal void CheckDependencies ()
 		{
 			IList list;
-			bool locked = false;
 			try {
 				cacheLock.EnterWriteLock ();
-				locked = true;
 				list = new List <CacheItem> ();
 				foreach (CacheItem it in cache.Values)
 					list.Add (it);
@@ -587,18 +580,15 @@ namespace System.Web.Caching
 						Remove (it.Key, CacheItemRemovedReason.DependencyChanged, false, true);
 				}
 			} finally {
-				if (locked) {
-					cacheLock.ExitWriteLock ();
-				}
+				// See comment at the top of the file, above cacheLock declaration
+				cacheLock.ExitWriteLock ();
 			}
 		}
 		
 		internal DateTime GetKeyLastChange (string key)
 		{
-			bool locked = false;
 			try {
 				cacheLock.EnterReadLock ();
-				locked = true;
 				CacheItem it = GetCacheItem (key);
 
 				if (it == null)
@@ -606,9 +596,8 @@ namespace System.Web.Caching
 				
 				return it.LastChange;
 			} finally {
-				if (locked) {
-					cacheLock.ExitReadLock ();
-				}
+				// See comment at the top of the file, above cacheLock declaration
+				cacheLock.ExitReadLock ();
 			}
 		}
 

mcs/class/System.Web/System.Web.Caching/CacheItemPriorityQueue.cs

@@ -41,6 +41,8 @@ namespace System.Web.Caching
 		CacheItem[] heap;
 		int heapSize = 0;
 		int heapCount = 0;
+
+		// See comment for the cacheLock field at top of System.Web.Caching/Cache.cs
 		ReaderWriterLockSlim queueLock;
 
 		public int Count {
@@ -94,20 +96,18 @@ namespace System.Web.Caching
 			if (item == null)
 				return;
 
-			bool locked = false;
 			CacheItem[] heap;
 			
 			try {
 				queueLock.EnterWriteLock ();
-				locked = true;
 				heap = GetHeapWithGrow ();
 				heap [heapCount++] = item;
 				BubbleUp (heap);
 				
 				AddSequenceEntry (item, EDSequenceEntryType.Enqueue);
 			} finally {
-				if (locked)
-					queueLock.ExitWriteLock ();
+				// See comment at the top of the file, above queueLock declaration
+				queueLock.ExitWriteLock ();
 			}
 		}
 
@@ -115,12 +115,10 @@ namespace System.Web.Caching
 		{
 			CacheItem ret = null;
 			CacheItem[] heap;
-			bool locked = false;
 			int index;
 			
 			try {
 				queueLock.EnterWriteLock ();
-				locked = true;
 				heap = GetHeapWithShrink ();
 				if (heap == null || heapCount == 0)
 					return null;
@@ -136,19 +134,17 @@ namespace System.Web.Caching
 				AddSequenceEntry (ret, EDSequenceEntryType.Dequeue);
 				return ret;
 			} finally {
-				if (locked)
-					queueLock.ExitWriteLock ();
+				// See comment at the top of the file, above queueLock declaration
+				queueLock.ExitWriteLock ();
 			}
 		}
 
 		public CacheItem Peek ()
 		{
-			bool locked = false;
 			CacheItem ret;
 			
 			try {
 				queueLock.EnterReadLock ();
-				locked = true;
 				if (heap == null || heapCount == 0)
 					return null;
 
@@ -157,8 +153,8 @@ namespace System.Web.Caching
 				
 				return ret;
 			} finally {
-				if (locked)
-					queueLock.ExitReadLock ();
+				// See comment at the top of the file, above queueLock declaration
+				queueLock.ExitReadLock ();
 			}
 		}
 		

mcs/class/System.Web/System.Web.Compilation/BuildManager.cs

@@ -94,6 +94,7 @@ namespace System.Web.Compilation
 		// This is here _only_ for the purpose of unit tests!
 		internal static bool suppressDebugModeMessages;
 
+		// See comment for the cacheLock field at top of System.Web.Caching/Cache.cs
 #if SYSTEMCORE_DEP
 		static ReaderWriterLockSlim buildCacheLock;
 #else
@@ -817,14 +818,12 @@ namespace System.Web.Compilation
 			// to the assembly builder and, in effect, no assembly is produced but there are STILL types that need
 			// to be added to the cache.
 			Assembly compiledAssembly = results != null ? results.CompiledAssembly : null;
-			bool locked = false;
 			try {
 #if SYSTEMCORE_DEP
 				buildCacheLock.EnterWriteLock ();
 #else
 				buildCacheLock.AcquireWriterLock (-1);
 #endif
-				locked = true;
 				if (compiledAssembly != null)
 					referencedAssemblies.Add (compiledAssembly);
 				
@@ -835,13 +834,11 @@ namespace System.Web.Compilation
 					StoreInCache (bp, compiledAssembly, results);
 				}
 			} finally {
-				if (locked) {
 #if SYSTEMCORE_DEP
-					buildCacheLock.ExitWriteLock ();
+				buildCacheLock.ExitWriteLock ();
 #else
-					buildCacheLock.ReleaseWriterLock ();
+				buildCacheLock.ReleaseWriterLock ();
 #endif
-				}
 			}
 		}
 		
@@ -883,24 +880,19 @@ namespace System.Web.Compilation
 #endif
 		static BuildManagerCacheItem GetCachedItem (string vp)
 		{
-			bool locked = false;
-			
 			try {
 #if SYSTEMCORE_DEP
 				buildCacheLock.EnterReadLock ();
 #else
 				buildCacheLock.AcquireReaderLock (-1);
 #endif
-				locked = true;
 				return GetCachedItemNoLock (vp);
 			} finally {
-				if (locked) {
 #if SYSTEMCORE_DEP
-					buildCacheLock.ExitReadLock ();
+				buildCacheLock.ExitReadLock ();
 #else
-					buildCacheLock.ReleaseReaderLock ();
+				buildCacheLock.ReleaseReaderLock ();
 #endif
-				}
 			}
 		}
 
@@ -1290,27 +1282,22 @@ namespace System.Web.Compilation
 			else
 				return;
 			
-			bool locked = false;
 			try {
 #if SYSTEMCORE_DEP
 				buildCacheLock.EnterWriteLock ();
 #else
 				buildCacheLock.AcquireWriterLock (-1);
 #endif
-				locked = true;
-
 				if (HasCachedItemNoLock (virtualPath)) {
 					buildCache [virtualPath] = null;
 					OnEntryRemoved (virtualPath);
 				}
 			} finally {
-				if (locked) {
 #if SYSTEMCORE_DEP
-					buildCacheLock.ExitWriteLock ();
+				buildCacheLock.ExitWriteLock ();
 #else
-					buildCacheLock.ReleaseWriterLock ();
+				buildCacheLock.ReleaseWriterLock ();
 #endif
-				}
 			}
 		}
 		

mcs/class/System.Web/System.Web.Configuration_2.0/WebConfigurationManager.cs

@@ -67,6 +67,8 @@ namespace System.Web.Configuration {
 		static readonly char[] pathTrimChars = { '/' };
 		static readonly object suppressAppReloadLock = new object ();
 		static readonly object saveLocationsCacheLock = new object ();
+		
+		// See comment for the cacheLock field at top of System.Web.Caching/Cache.cs
 		static readonly ReaderWriterLockSlim sectionCacheLock;
 		
 #if !TARGET_J2EE
@@ -225,14 +227,11 @@ namespace System.Web.Configuration {
 		
 		static void ConfigurationSaveHandler (_Configuration sender, ConfigurationSaveEventArgs args)
 		{
-			bool locked = false;
 			try {
 				sectionCacheLock.EnterWriteLock ();
-				locked = true;
 				sectionCache.Clear ();
 			} finally {
-				if (locked)
-					sectionCacheLock.ExitWriteLock ();
+				sectionCacheLock.ExitWriteLock ();
 			}
 			
 			lock (suppressAppReloadLock) {
@@ -442,7 +441,6 @@ namespace System.Web.Configuration {
 			int cacheKey;
 			bool pathPresent = !String.IsNullOrEmpty (path);
 			string locationPath = null;
-			bool locked = false;
 
 			if (pathPresent)
 				locationPath = "location_" + path;
@@ -453,7 +451,6 @@ namespace System.Web.Configuration {
 			
 			try {
 				sectionCacheLock.EnterReadLock ();
-				locked = true;
 				
 				object o;
 				if (pathPresent) {
@@ -469,8 +466,7 @@ namespace System.Web.Configuration {
 				if (sectionCache.TryGetValue (baseCacheKey, out o))
 					return o;
 			} finally {
-				if (locked)
-					sectionCacheLock.ExitReadLock ();
+				sectionCacheLock.ExitReadLock ();
 			}
 
 			string cachePath = null;
@@ -687,27 +683,21 @@ namespace System.Web.Configuration {
 		static void AddSectionToCache (int key, object section)
 		{
 			object cachedSection;
-			bool locked = false;
 
 			try {
 				sectionCacheLock.EnterUpgradeableReadLock ();
-				locked = true;
 					
 				if (sectionCache.TryGetValue (key, out cachedSection) && cachedSection != null)
 					return;
 
-				bool innerLocked = false;
 				try {
 					sectionCacheLock.EnterWriteLock ();
-					innerLocked = true;
 					sectionCache.Add (key, section);
 				} finally {
-					if (innerLocked)
-						sectionCacheLock.ExitWriteLock ();
+					sectionCacheLock.ExitWriteLock ();
 				}
 			} finally {
-				if (locked)
-					sectionCacheLock.ExitUpgradeableReadLock ();
+				sectionCacheLock.ExitUpgradeableReadLock ();
 			}
 		}