2004-05-27 Patrik Torstensson <[email protected]>

	* CacheEntry.cs,CacheExpires.cs,ExpiresBucket.cs,
	  Cache.cs : Fixed deadlock issues, fixed
	  items not correctly being flushed, fixed update
	  of item when expiration has been updated more
	  than 1 min (placed in wrong bucket),
	  fixed deadlock during cache callback when item
	  is removed due to expiriation.

	  Rewrite of locking handling in Cache class, leading
	  to better performance and less bugs.

	  This rewrite is due to a number of bugs found
	  in the output caching during load (leading to memory
	  leaks and deadlocks)

svn path=/trunk/mcs/; revision=28254

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

@@ -2,68 +2,32 @@
 // System.Web.Caching
 //
 // Author:
-//   Patrik Torstensson ([email protected])
-// Changes:
+//   Patrik Torstensson
 //   Daniel Cazzulino [DHC] ([email protected])
 //
-// (C) Copyright Patrik Torstensson, 2001
-//
 
 using System;
 using System.Collections;
 using System.Threading;
 
-namespace System.Web.Caching
-{
-	/// <summary>
-	/// Implements a cache for Web applications and other. The difference
-	/// from the MS.NET implementation is that we / support to use the Cache
-	/// object as cache in our applications.
-	/// </summary>
-	/// <remarks>
-	/// The Singleton cache is created per application domain, and it
-	/// remains valid as long as the application domain remains active. 
-	/// </remarks>
-	/// <example>
-	/// Usage of the singleton cache:
-	/// 
-	/// Cache objManager = Cache.SingletonCache;
-	/// 
-	/// String obj = "tobecached";
-	/// objManager.Add("kalle", obj);
-	/// </example>
-	public sealed class Cache : IEnumerable
-	{
-		// Declarations
+namespace System.Web.Caching {
+	public sealed class Cache : IEnumerable {
 
-		/// <summary>
-		/// Used in the absoluteExpiration parameter in an Insert
-		/// method call to indicate the item should never expire. This
-		/// field is read-only.
-		/// </summary>
 		public static readonly DateTime NoAbsoluteExpiration = DateTime.MaxValue;
-
-		/// <summary>
-		/// Used as the slidingExpiration parameter in an Insert method
-		/// call to disable sliding expirations. This field is read-only.
-		/// </summary>
 		public static readonly TimeSpan NoSlidingExpiration = TimeSpan.Zero;
 
 		// Helper objects
-		CacheExpires _objExpires;
+		private CacheExpires _objExpires;
 
 		// The data storage 
-		// Todo: Make a specialized storage for the cache entries?
-		// todo: allow system to replace the storage?
-		Hashtable _arrEntries;
-		ReaderWriterLock _lockEntries;
-
-		static TimeSpan _datetimeOneYear = TimeSpan.FromDays (365);
+		private Hashtable _arrEntries;
+		private ReaderWriterLock _lockEntries;
+		private int _nItems;
+		
+		static private TimeSpan _datetimeOneYear = TimeSpan.FromDays (365);
 
-		int _nItems; // Number of items in the cache
 
-		public Cache ()
-		{
+		public Cache () {
 			_nItems = 0;
 
 			_lockEntries = new ReaderWriterLock ();
@@ -79,9 +43,7 @@ namespace System.Web.Caching
 		/// <returns>
 		/// Returns IDictionaryEnumerator that contains all public items in the cache
 		/// </returns>
-
-		private IDictionaryEnumerator CreateEnumerator ()
-		{
+		private IDictionaryEnumerator CreateEnumerator () {
 			Hashtable objTable;
 
 			//Locking with -1 provides a non-expiring lock.
@@ -95,7 +57,7 @@ namespace System.Web.Caching
 						continue;
 
 					CacheEntry entry = (CacheEntry) objEntry.Value;
-					if (entry.TestFlag (CacheEntry.Flags.Public))
+					if (entry.IsPublic)
 						objTable.Add (objEntry.Key, entry.Item);
 				}
 			} finally {
@@ -105,31 +67,17 @@ namespace System.Web.Caching
 			return objTable.GetEnumerator ();
 		}
 
-		/// <summary>
-		/// Implementation of IEnumerable interface and calls the GetEnumerator that returns
-		/// IDictionaryEnumerator.
-		/// </summary>
-		IEnumerator IEnumerable.GetEnumerator ()
-		{
+
+		IEnumerator IEnumerable.GetEnumerator () {
 			return GetEnumerator ();
 		}
 
-		/// <summary>
-		/// Virtual override of the IEnumerable.GetEnumerator() method,
-		/// returns a specialized enumerator.
-		/// </summary>
-		public IDictionaryEnumerator GetEnumerator ()
-		{
+		public IDictionaryEnumerator GetEnumerator () {
 			return CreateEnumerator ();
 		}
 
-		/// <summary>
-		/// Touches a object in the cache. Used to update expire time and hit count.
-		/// </summary>
-		/// <param name="strKey">The identifier for the cache item to retrieve.</param>
-		internal void Touch(string strKey)
-		{
-			Get (strKey);
+		internal void Touch(string strKey) {
+			GetEntry (strKey);
 		}
 
 		/// <summary>
@@ -168,27 +116,24 @@ namespace System.Web.Caching
 		/// cache.
 		/// </param>
 		/// <returns>The Object item added to the Cache.</returns>
-		public object Add (string strKey,
-				   object objItem,
-				   CacheDependency objDependency,
-				   DateTime absolutExpiration,
-				   TimeSpan slidingExpiration,
-				   CacheItemPriority enumPriority,
-				   CacheItemRemovedCallback eventRemoveCallback)
-		{
+		public object Add (string strKey, object objItem, CacheDependency objDependency,
+							DateTime absolutExpiration, TimeSpan slidingExpiration, 
+							CacheItemPriority enumPriority, CacheItemRemovedCallback eventRemoveCallback) {
+			
 			return Add (strKey, objItem, objDependency, absolutExpiration,
-					slidingExpiration, enumPriority, eventRemoveCallback, true);
+				slidingExpiration, enumPriority, eventRemoveCallback, true, false);
 		}
 
 		private object Add (string strKey,
-				   object objItem,
-				   CacheDependency objDependency,
-				   DateTime absolutExpiration,
-				   TimeSpan slidingExpiration,
-				   CacheItemPriority enumPriority,
-				   CacheItemRemovedCallback eventRemoveCallback,
-				   bool pub)
-		{
+			object objItem,
+			CacheDependency objDependency,
+			DateTime absolutExpiration,
+			TimeSpan slidingExpiration,
+			CacheItemPriority enumPriority,
+			CacheItemRemovedCallback eventRemoveCallback,
+			bool pub,
+			bool overwrite) {
+
 			if (strKey == null)
 				throw new ArgumentNullException ("strKey");
 
@@ -199,34 +144,54 @@ namespace System.Web.Caching
 				throw new ArgumentOutOfRangeException ("slidingExpiration");
 
 			CacheEntry objEntry;
-			CacheEntry objNewEntry;
+			CacheEntry objOldEntry = null;
 
 			long longHitRange = 10000;
 
 			// todo: check decay and make up the minHit range
 
 			objEntry = new CacheEntry (this,
-						   strKey,
-						   objItem,
-						   objDependency,
-						   eventRemoveCallback,
-						   absolutExpiration,
-						   slidingExpiration,
-						   longHitRange,
-						   pub,
-						   enumPriority);
+										strKey,
+										objItem,
+										objDependency,
+										eventRemoveCallback,
+										absolutExpiration,
+										slidingExpiration,
+										longHitRange,
+										pub,
+										enumPriority);
 
 			Interlocked.Increment (ref _nItems);
 
-			// If we have any kind of expiration add into the CacheExpires class
-			if (objEntry.HasSlidingExpiration || objEntry.HasAbsoluteExpiration)
-				_objExpires.Add (objEntry);
+			_lockEntries.AcquireWriterLock (-1);
+			try {
+				if (_arrEntries.Contains (strKey)) {
+					if (overwrite)
+						objOldEntry = _arrEntries [strKey] as CacheEntry;
+					else
+						return null;
+				}
+				
+				objEntry.Hit ();
+				_arrEntries [strKey] = objEntry;
+			} finally {
+				_lockEntries.ReleaseLock ();
+			}
 
-			// Check and get the new item..
-			objNewEntry = UpdateCache (strKey, objEntry, true, CacheItemRemovedReason.Removed);
+			if (objOldEntry != null) {
+				if (objOldEntry.HasAbsoluteExpiration || objOldEntry.HasSlidingExpiration)
+					_objExpires.Remove (objOldEntry);
 
-			if (objNewEntry == null)
-				return null;
+				objOldEntry.Close (CacheItemRemovedReason.Removed);
+			}
+
+			// If we have any kind of expiration add into the CacheExpires class
+			if (objEntry.HasSlidingExpiration || objEntry.HasAbsoluteExpiration) {
+				if (objEntry.HasSlidingExpiration)
+					objEntry.Expires = DateTime.Now.Ticks + objEntry.SlidingExpiration;
+
+				_objExpires.Add (objEntry);
+			}
 
 			return objEntry.Item;
 		}
@@ -238,15 +203,9 @@ namespace System.Web.Caching
 		/// </summary>
 		/// <param name="strKey">The cache key used to reference the item.</param>
 		/// <param name="objItem">The item to be added to the cache.</param>
-		public void Insert (string strKey, object objItem)
-		{
-			Add (strKey,
-			     objItem,
-			     null,
-			     NoAbsoluteExpiration,
-			     NoSlidingExpiration,
-			     CacheItemPriority.Default,
-			     null);
+		public void Insert (string strKey, object objItem) {
+			Add (strKey, objItem, null, NoAbsoluteExpiration, NoSlidingExpiration,
+				CacheItemPriority.Default, null, true, true);
 		}
 
 		/// <summary>
@@ -260,15 +219,9 @@ namespace System.Web.Caching
 		/// from the cache. If there are no dependencies, this paramter
 		/// contains a null reference.
 		/// </param>
-		public void Insert (string strKey, object objItem, CacheDependency objDependency)
-		{
-			Add (strKey,
-			     objItem,
-			     objDependency,
-			     NoAbsoluteExpiration,
-			     NoSlidingExpiration,
-			     CacheItemPriority.Default,
-			     null);
+		public void Insert (string strKey, object objItem, CacheDependency objDependency) {
+			Add (strKey, objItem, objDependency, NoAbsoluteExpiration, NoSlidingExpiration,
+				CacheItemPriority.Default, null, true, true);
 		}
 
 		/// <summary>
@@ -291,19 +244,11 @@ namespace System.Web.Caching
 		/// object expires and is removed from the cache 20 minutes after
 		/// it is last accessed.
 		/// </param>
-		public void Insert (string strKey,
-				    object objItem,
-				    CacheDependency objDependency,
-				    DateTime absolutExpiration,
-				    TimeSpan slidingExpiration)
-		{
-			Add (strKey,
-			     objItem,
-			     objDependency,
-			     absolutExpiration,
-			     slidingExpiration,
-			     CacheItemPriority.Default,
-			     null);
+		public void Insert (string strKey, object objItem, CacheDependency objDependency,
+							DateTime absolutExpiration, TimeSpan slidingExpiration) {
+
+			Add (strKey, objItem, objDependency, absolutExpiration, slidingExpiration, 
+				CacheItemPriority.Default, null, true, true);
 		}
 
 		/// <summary>
@@ -340,38 +285,22 @@ namespace System.Web.Caching
 		/// You can use this to notify applications when their objects
 		/// are deleted from the cache.
 		/// </param>
-		public void Insert (string strKey,
-				    object objItem,
-				    CacheDependency objDependency,
-				    DateTime absolutExpiration,
-				    TimeSpan slidingExpiration,
-				    CacheItemPriority enumPriority,
-				    CacheItemRemovedCallback eventRemoveCallback)
-		{
-			Add (strKey,
-			     objItem,
-			     objDependency,
-			     absolutExpiration,
-			     slidingExpiration,
-			     enumPriority,
-			     eventRemoveCallback);
+		public void Insert (string strKey, object objItem, CacheDependency objDependency,
+							DateTime absolutExpiration, TimeSpan slidingExpiration,
+							CacheItemPriority enumPriority, CacheItemRemovedCallback eventRemoveCallback) {
+
+			Add (strKey, objItem, objDependency, absolutExpiration, slidingExpiration, 
+				enumPriority, eventRemoveCallback, true, true);
 		}
 
-		internal void InsertPrivate (string strKey,
-				object objItem,
-				CacheDependency objDependency,
-				DateTime absolutExpiration,
-				TimeSpan slidingExpiration,
-				CacheItemPriority enumPriority,
-				CacheItemRemovedCallback eventRemoveCallback)
-		{
-			Add (strKey,
-			     objItem,
-			     objDependency,
-			     absolutExpiration,
-			     slidingExpiration,
-			     enumPriority,
-			     eventRemoveCallback, false);
+		// Called from other internal System.Web methods to add non-public objects into
+		// cache, like output cache etc
+		internal void InsertPrivate (string strKey, object objItem, CacheDependency objDependency,
+									DateTime absolutExpiration, TimeSpan slidingExpiration,
+									CacheItemPriority enumPriority, CacheItemRemovedCallback eventRemoveCallback) {
+
+			Add (strKey, objItem, objDependency, absolutExpiration, slidingExpiration, 
+				enumPriority, eventRemoveCallback, false, true);
 		}
 
 		/// <summary>
@@ -382,8 +311,7 @@ namespace System.Web.Caching
 		/// The item removed from the Cache. If the value in the key
 		/// parameter is not found, returns a null reference.
 		/// </returns>
-		public object Remove (string strKey)
-		{
+		public object Remove (string strKey) {
 			return Remove (strKey, CacheItemRemovedReason.Removed);
 		}
 
@@ -399,17 +327,31 @@ namespace System.Web.Caching
 		/// The item removed from the Cache. If the value in the key
 		/// parameter is not found, returns a null reference.
 		/// </returns>
-		internal object Remove (string strKey, CacheItemRemovedReason enumReason)
-		{
-			CacheEntry objEntry = UpdateCache (strKey, null, true, enumReason);
-			if (objEntry == null)
-				return null;
+		internal object Remove (string strKey, CacheItemRemovedReason enumReason) {
+			CacheEntry objEntry = null;
 
-			Interlocked.Decrement (ref _nItems);
+			if (strKey == null)
+				throw new ArgumentNullException ("strKey");
+
+			_lockEntries.AcquireWriterLock (-1);
+			try {
+				objEntry = _arrEntries [strKey] as CacheEntry;
+				if (null == objEntry)
+					return null;
+
+				_arrEntries.Remove (strKey);
+			}
+			finally {
+				_lockEntries.ReleaseWriterLock ();
+			}
+
+			if (objEntry.HasAbsoluteExpiration || objEntry.HasSlidingExpiration)
+				_objExpires.Remove (objEntry);
 
-			// Close the cache entry (calls the remove delegate)
 			objEntry.Close (enumReason);
 
+			Interlocked.Decrement (ref _nItems);
+
 			return objEntry.Item;
 		}
 
@@ -418,9 +360,8 @@ namespace System.Web.Caching
 		/// </summary>
 		/// <param name="strKey">The identifier for the cache item to retrieve.</param>
 		/// <returns>The retrieved cache item, or a null reference.</returns>
-		public object Get (string strKey)
-		{
-			CacheEntry objEntry = UpdateCache (strKey, null, false, CacheItemRemovedReason.Expired);
+		public object Get (string strKey) {
+			CacheEntry objEntry = GetEntry (strKey);
 
 			if (objEntry == null)
 				return null;
@@ -428,120 +369,38 @@ namespace System.Web.Caching
 			return objEntry.Item;
 		}
 
-		internal CacheEntry GetEntry (string strKey)
-		{
-			CacheEntry objEntry = UpdateCache (strKey, null, false, CacheItemRemovedReason.Expired);
-
-			if (objEntry == null)
-				return null;
-
-			return objEntry;
-		}
-
-		/// <summary>
-		/// Internal method used for removing, updating and adding CacheEntries into the cache.
-		/// </summary>
-		/// <param name="strKey">The identifier for the cache item to modify</param>
-		/// <param name="objEntry">
-		/// CacheEntry to use for overwrite operation, if this
-		/// parameter is null and overwrite true the item is going to be
-		/// removed
-		/// </param>
-		/// <param name="boolOverwrite">
-		/// If true the objEntry parameter is used to overwrite the
-		/// strKey entry
-		/// </param>
-		/// <param name="enumReason">Reason why an item was removed</param>
-		/// <returns></returns>
-		private CacheEntry UpdateCache (string strKey,
-						CacheEntry objEntry,
-						bool boolOverwrite,
-						CacheItemRemovedReason enumReason)
-		{
+		internal CacheEntry GetEntry (string strKey) {
+			CacheEntry objEntry = null;
+			long ticksNow = DateTime.Now.Ticks;
+			
 			if (strKey == null)
 				throw new ArgumentNullException ("strKey");
 
-			long ticksNow = DateTime.Now.Ticks;
-			long ticksExpires = long.MaxValue;
-
-			bool boolGetItem = false;
-			bool boolExpiried = false;
-			bool boolWrite = false;
-			bool boolRemoved = false;
-
-			// Are we getting the item from the hashtable
-			if (boolOverwrite == false && strKey.Length > 0 && objEntry == null)
-				boolGetItem = true;
-
-			// TODO: Optimize this method, move out functionality outside the lock
 			_lockEntries.AcquireReaderLock (-1);
 			try {
-				if (boolGetItem) {
-					objEntry = (CacheEntry) _arrEntries [strKey];
-					if (objEntry == null)
-						return null;
-				}
-
-				if (objEntry != null) {
-					// Check if we have expired
-					if (objEntry.HasSlidingExpiration || objEntry.HasAbsoluteExpiration) {
-						if (objEntry.Expires < ticksNow) {
-							// We have expired, remove the item from the cache
-							boolWrite = true;
-							boolExpiried = true;
-						} 
-					} 
-				}
-
-				// Check if we going to modify the hashtable
-				if (boolWrite || (boolOverwrite && !boolExpiried)) {
-					// Upgrade our lock to write
-					Threading.LockCookie objCookie = _lockEntries.UpgradeToWriterLock (-1);
-					try {
-						// Check if we going to just modify an existing entry (or add)
-						if (boolOverwrite && objEntry != null) {
-							_arrEntries [strKey] = objEntry;
-						} else {
-							// We need to remove the item, fetch the item first
-							objEntry = (CacheEntry) _arrEntries [strKey];
-							if (objEntry != null)
-								_arrEntries.Remove (strKey);
-
-							boolRemoved = true;
-						}
-					} finally {
-						_lockEntries.DowngradeFromWriterLock (ref objCookie);
-					}
-				}
-
-				// If the entry haven't expired or been removed update the info
-				if (!boolExpiried && !boolRemoved) {
-					// Update that we got a hit
-					objEntry.Hits++;
-					if (objEntry.HasSlidingExpiration)
-						ticksExpires = ticksNow + objEntry.SlidingExpiration;
-				}
-			} finally {
-				_lockEntries.ReleaseLock ();
+				objEntry = _arrEntries [strKey] as CacheEntry;
+				if (null == objEntry)
+					return null;
+			}
+			finally {
+				_lockEntries.ReleaseReaderLock ();
 			}
 
-			// If the item was removed we need to remove it from the CacheExpired class also
-			if (boolRemoved) {
-				if (objEntry != null) {
-					if (objEntry.HasAbsoluteExpiration || objEntry.HasSlidingExpiration)
-						_objExpires.Remove (objEntry);
-					objEntry.Close (enumReason);
+			if (objEntry.HasSlidingExpiration || objEntry.HasAbsoluteExpiration) {
+				if (objEntry.Expires < ticksNow) {
+					Remove (strKey, CacheItemRemovedReason.Expired);
+					return null;
 				}
-				return null;
-			}
+			} 
 
-			// If we have sliding expiration and we have a correct hit, update the expiration manager
+			objEntry.Hit ();
 			if (objEntry.HasSlidingExpiration) {
+				long ticksExpires = ticksNow + objEntry.SlidingExpiration;
+
 				_objExpires.Update (objEntry, ticksExpires);
 				objEntry.Expires = ticksExpires;
 			}
 
-			// Return the cache entry
 			return objEntry;
 		}
 

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

@@ -2,10 +2,9 @@
 // System.Web.Caching
 //
 // Author:
-//   Patrik Torstensson ([email protected])
-//
-// (C) Copyright Patrik Torstensson, 2001
+//   Patrik Torstensson
 //
+
 namespace System.Web.Caching
 {
 	/// <summary>

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

@@ -2,10 +2,9 @@
 // System.Web.Caching.CacheDependency
 //
 // Authors:
-// 	Patrik Torstensson ([email protected])
+// 	Patrik Torstensson
 // 	Gonzalo Paniagua Javier ([email protected])
 //
-// (C) Copyright Patrik Torstensson, 2001
 // (c) 2003 Ximian, Inc. (http://www.ximian.com)
 //
 using System;

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

@@ -2,30 +2,25 @@
 // System.Web.Caching
 //
 // Author:
-//   Patrik Torstensson ([email protected])
-// Changes:
-//   Daniel Cazzulino [DHC] ([email protected])
+//   Patrik Torstensson
+//   Daniel Cazzulino ([email protected])
 //
-// (C) Copyright Patrik Torstensson, 2001
-//
-namespace System.Web.Caching
-{
+
+using System;
+using System.Threading;
+
+namespace System.Web.Caching {
 	/// <summary>
 	/// Class responsible for representing a cache entry.
 	/// </summary>
-	internal class CacheEntry
-	{
-		/// <summary>
-		/// Defines the status of the current cache entry
-		/// </summary>
-		public enum Flags
-		{
+	internal class CacheEntry {
+		internal enum Flags {
 			Removed	= 0,
 			Public	= 1
 		}
 
 		private CacheItemPriority		_enumPriority;
-        
+       
 		private long	_longHits;
 
 		private byte	_byteExpiresBucket;
@@ -44,20 +39,12 @@ namespace System.Web.Caching
 		private CacheDependency		_objDependency;
 		private Cache				_objCache;
 
-		/// <summary>
-		/// The item is not placed in a bucket. [DHC]
-		/// </summary>
 		internal static readonly byte NoBucketHash = byte.MaxValue;
-		
-		/// <summary>
-		/// The item is not placed in a bucket. [DHC]
-		/// </summary>
 		internal static readonly int NoIndexInBucket = int.MaxValue;
 
-		/// <summary>
-		/// Lock for syncronized operations. [DHC]
-		/// </summary>
-		System.Threading.ReaderWriterLock _lock = new System.Threading.ReaderWriterLock();
+		internal event CacheItemRemovedCallback _onRemoved;
+
+		private ReaderWriterLock _lock = new ReaderWriterLock();
 
 		/// <summary>
 		/// Constructs a new cache entry
@@ -70,13 +57,10 @@ namespace System.Web.Caching
 		/// <param name="longMinHits">Used to detect and control if the item should be flushed due to under usage</param>
 		/// <param name="boolPublic">Defines if the item is public or not</param>
 		/// <param name="enumPriority">The relative cost of the object, as expressed by the CacheItemPriority enumeration. The cache uses this value when it evicts objects; objects with a lower cost are removed from the cache before objects with a higher cost.</param>
-		internal CacheEntry(	Cache objManager, string strKey, object objItem, CacheDependency objDependency, CacheItemRemovedCallback eventRemove, 
-			System.DateTime dtExpires, System.TimeSpan tsSpan, long longMinHits, bool boolPublic, CacheItemPriority enumPriority )
-		{
+		internal CacheEntry (Cache objManager, string strKey, object objItem, CacheDependency objDependency, CacheItemRemovedCallback eventRemove, 
+			System.DateTime dtExpires, System.TimeSpan tsSpan, long longMinHits, bool boolPublic, CacheItemPriority enumPriority ) {
 			if (boolPublic)
-			{
-				SetFlag(Flags.Public);
-			}
+				_enumFlags |= Flags.Public;
 
 			_strKey = strKey;
 			_objItem = objItem;
@@ -94,27 +78,19 @@ namespace System.Web.Caching
 			// This is because sliding expiration causes the absolute expiration to be 
 			// moved after each period, and the absolute expiration is the value used 
 			// for all expiration calculations.
-			//HACK: [DHC] Use constants defined in Cache.
-			//if (tsSpan.Ticks != System.TimeSpan.Zero.Ticks)
 			if (tsSpan.Ticks != Cache.NoSlidingExpiration.Ticks)
-			{
 				_ticksExpires = System.DateTime.Now.AddTicks(_ticksSlidingExpiration).Ticks;
-			}
 			
 			_objDependency = objDependency;
 			if (_objDependency != null)
-			{
 				// Add the entry to the cache dependency handler (we support multiple entries per handler)
 				_objDependency.Changed += new CacheDependencyChangedHandler (OnChanged); 
-			}
 
 			_longMinHits = longMinHits;
 		}
 
-		internal event CacheItemRemovedCallback _onRemoved;
 
-		internal void OnChanged (object sender, CacheDependencyChangedArgs objDependency)
-		{
+		internal void OnChanged (object sender, CacheDependencyChangedArgs objDependency) {
 			_objCache.Remove (_strKey, CacheItemRemovedReason.DependencyChanged);
 		}
 
@@ -122,315 +98,159 @@ namespace System.Web.Caching
 		/// Cleans up the cache entry, removes the cache dependency and calls the remove delegate.
 		/// </summary>
 		/// <param name="enumReason">The reason why the cache entry are going to be removed</param>
-		internal void Close(CacheItemRemovedReason enumReason)
-		{	
-			//HACK: optimized locks. [DHC]
+		internal void Close(CacheItemRemovedReason enumReason) {	
+			Delegate [] removedEvents = null;
+
 			_lock.AcquireWriterLock(-1);
-			try
-			{
+			try {
 				// Check if the item already is removed
-				if (TestFlag(Flags.Removed))
-				{
+				if ((_enumFlags & Flags.Removed) != 0)
 					return;
-				}
 
-				SetFlag(Flags.Removed);
+				_enumFlags |= Flags.Removed;
 
 				if (_onRemoved != null)
-				{
-					// Call the delegate to tell that we are now removing the entry
-					try 
-					{
-						_onRemoved(_strKey, _objItem, enumReason);		
-					}
-					catch (System.Exception objException)
-					{
-						System.Diagnostics.Debug.Fail("System.Web.CacheEntry.Close() Exception when calling remove delegate", "Message: " + objException.Message + " Stack: " + objException.StackTrace + " Source:" + objException.Source);
-					}
-				}
-
-				// If we have a dependency, remove the entry
-				if (_objDependency != null)
-				{
-					_objDependency.Changed -= new CacheDependencyChangedHandler (OnChanged);
-				}
+					removedEvents = _onRemoved.GetInvocationList ();
 			}
-			finally
-			{
+			finally {
 				_lock.ReleaseWriterLock();
 			}
-		}
 
-		/// <summary>
-		/// Tests a specific flag is set or not.
-		/// </summary>
-		/// <param name="oFlag">Flag to test agains</param>
-		/// <returns>Returns true if the flag is set.</returns>
-		internal bool TestFlag(Flags oFlag)
-		{
-			_lock.AcquireReaderLock(-1);
-			try
-			{
-				if ((_enumFlags & oFlag) != 0)
-				{
-					return true;
+			if (removedEvents != null) {
+				// Call the delegate to tell that we are now removing the entry
+				if ((_enumFlags & Flags.Public) != 0) {
+					foreach (Delegate del in removedEvents) {
+						CacheItemRemovedCallback removed = (CacheItemRemovedCallback) del;
+						try {
+							removed (_strKey, _objItem, enumReason);		
+						}
+						catch (System.Exception obj) {
+							HttpApplicationFactory.SignalError (obj);
+						}
+					}
 				} 
-			}
-			finally
-			{
-				_lock.ReleaseReaderLock();
+				else {
+					foreach (Delegate del in removedEvents) {
+						CacheItemRemovedCallback removed = (CacheItemRemovedCallback) del;
+						try {
+							removed (_strKey, _objItem, enumReason);		
+						}
+						catch (Exception) {
+						}
+					}
+				}
 			}
 
-			return false;
-		}
-
-		/// <summary>
-		/// Sets a specific flag.
-		/// </summary>
-		/// <param name="oFlag">Flag to set.</param>
-		internal void SetFlag(Flags oFlag)
-		{
 			_lock.AcquireWriterLock(-1);
-			try
-			{
-				_enumFlags |= oFlag;
+			try {
+				// If we have a dependency, remove the entry
+				if (_objDependency != null)
+					_objDependency.Changed -= new CacheDependencyChangedHandler (OnChanged);
 			}
-			finally
-			{
-				_lock.ReleaseWriterLock	();
+			finally {
+				_lock.ReleaseWriterLock();
 			}
 		}
-
-		/// <summary>
-		/// Returns true if the object has minimum hit usage flushing enabled.
-		/// </summary>
-		internal bool HasUsage
-		{
+	
+		internal bool HasUsage {
 			get { 
-				if (_longMinHits == System.Int64.MaxValue) 
-				{
+				if (_longMinHits == System.Int64.MaxValue)
 					return false;
-				}
 
 				return true;
 			}
 		}
 
-		/// <summary>
-		/// Returns true if the entry has absolute expiration.
-		/// </summary>
-		internal bool HasAbsoluteExpiration
-		{
-			get 
-			{ 
-				//HACK: [DHC] Use constant defined in Cache.
-				//if (_ticksExpires == System.DateTime.MaxValue.Ticks) 
+		internal bool HasAbsoluteExpiration {
+			get { 
 				if (_ticksExpires == Cache.NoAbsoluteExpiration.Ticks) 
-				{
 					return false;
-				}
 
 				return true;
 			}
 		}
 
-		/// <summary>
-		/// Returns true if the entry has sliding expiration enabled.
-		/// </summary>
-		internal bool HasSlidingExpiration
-		{
-			get 
-			{ 
-				//HACK: [DHC] Use constants defined in Cache.
-				//if (_ticksSlidingExpiration == System.TimeSpan.Zero.Ticks) 
+		internal bool HasSlidingExpiration {
+			get { 
 				if (_ticksSlidingExpiration == Cache.NoSlidingExpiration.Ticks) 
-				{
 					return false;
-				}
 
 				return true;
 			}
 		}
 		
-		/// <summary>
-		/// Gets and sets the current expires bucket the entry is active in.
-		/// </summary>
-		internal byte ExpiresBucket
-		{
-			get 
-			{ 
-				_lock.AcquireReaderLock(-1);
-				try
-				{
-					return _byteExpiresBucket; 
-				}
-				finally
-				{
-					_lock.ReleaseReaderLock();
-				}
+		internal byte ExpiresBucket {
+			get { 
+				return _byteExpiresBucket; 
 			}
-			set 
-			{ 
-				_lock.AcquireWriterLock(-1);
-				try
-				{
-					_byteExpiresBucket = value; 
-				}
-				finally
-				{
-					_lock.ReleaseWriterLock	();
-				}
+			set { 
+				_byteExpiresBucket = value; 
 			}
 		}
 
-		/// <summary>
-		/// Gets and sets the current index in the expires bucket of the current cache entry.
-		/// </summary>
-		internal int ExpiresIndex
-		{
-			get 
-			{ 
-				_lock.AcquireReaderLock(-1);
-				try
-				{
-					return _intExpiresIndex; 
-				}
-				finally
-				{
-					_lock.ReleaseReaderLock();
-				}
+		internal int ExpiresIndex {
+			get { 
+				return _intExpiresIndex; 
 			}
 			
-			set 
-			{ 
-				_lock.AcquireWriterLock(-1);
-				try
-				{
-					_intExpiresIndex = value; 
-				}
-				finally
-				{
-					_lock.ReleaseWriterLock();
-				}
+			set { 
+				_intExpiresIndex = value; 
 			}
 		}
         
-		/// <summary>
-		/// Gets and sets the expiration of the cache entry.
-		/// </summary>
-		internal long Expires
-		{
-			get 
-			{ 
-				_lock.AcquireReaderLock(-1);
-				try
-				{
-					return _ticksExpires; 
-				}
-				finally
-				{
-					_lock.ReleaseReaderLock();
-				}
+		internal long Expires {
+			get { 
+				return _ticksExpires; 
 			}
-			set 
-			{ 
-				_lock.AcquireWriterLock(-1);
-				try
-				{
-					_ticksExpires = value; 
-				}
-				finally
-				{
-					_lock.ReleaseWriterLock();
-				}
+			set { 
+				_ticksExpires = value; 
 			}
 		}
 
-		/// <summary>
-		/// Gets the sliding expiration value. The return value is in ticks (since 0/0-01 in 100nanosec)
-		/// </summary>
-		internal long SlidingExpiration
-		{
-			get 
-			{ 
+		internal long SlidingExpiration {
+			get { 
 				return _ticksSlidingExpiration; 
 			}
 		}
 
-		/// <summary>
-		/// Returns the current cached item.
-		/// </summary>
-		internal object Item
-		{
-			get
-			{
+		internal object Item {
+			get {
 				return _objItem; 
 			}
 		}
 
-		/// <summary>
-		/// Returns the current cache identifier.
-		/// </summary>
-		internal string Key
-		{
-			get 
-			{ 
+		internal string Key {
+			get { 
 				return _strKey; 
 			}
 		}
 
-		/// <summary>
-		/// Gets and sets the current number of hits on the cache entry.
-		/// </summary>
-		internal long Hits
-		{
-			// todo: Could be optimized by using interlocked methods..
-			get 
-			{
-				_lock.AcquireReaderLock(-1);
-				try
-				{
-					return _longHits; 
-				}
-				finally
-				{
-					_lock.ReleaseReaderLock();
-				}
-			}
-			set 
-			{ 
-				_lock.AcquireWriterLock(-1);
-				try
-				{
-					_longHits = value; 
-				}
-				finally
-				{
-					_lock.ReleaseWriterLock();
-				}
+		internal long Hits {
+			get {
+				return _longHits; 
 			}
 		}
 
-		/// <summary>
-		/// Returns minimum hits for the usage flushing rutine.
-		/// </summary>
-		internal long MinimumHits
-		{
-			get 
-			{ 
+		internal long MinimumHits {
+			get { 
 				return _longMinHits; 
 			}
 		}
 
-		/// <summary>
-		/// Returns the priority of the cache entry.
-		/// </summary>
-		internal CacheItemPriority Priority
-		{
-			get 
-			{ 
+		internal CacheItemPriority Priority {
+			get { 
 				return _enumPriority; 
 			}
 		}
+
+		internal bool IsPublic {
+			get {
+				return (_enumFlags & Flags.Public) != 0;
+			}
+		}
+
+		internal void Hit () {
+			Interlocked.Increment (ref _longHits);
+		}
 	}
 }

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

@@ -2,22 +2,22 @@
 // System.Web.Caching
 //
 // Author:
-//   Patrik Torstensson ([email protected])
-// Changes:
-//   Daniel Cazzulino [DHC] ([email protected])
+//   Patrik Torstensson
+//   Daniel Cazzulino ([email protected])
 //
-// (C) Copyright Patrik Torstensson, 2001
-//
-namespace System.Web.Caching
-{
+
+using System;
+using System.Collections;
+using System.Threading;
+
+namespace System.Web.Caching {
 	/// <summary>
 	/// Class responsible for handling time based flushing of entries in the cache. The class creates
 	/// and manages 60 buckets each holding every item that expires that minute. The bucket calculated
 	/// for an entry is one minute more than the timeout just to make sure that the item end up in the
 	/// bucket where it should be flushed.
 	/// </summary>
-	internal class CacheExpires : System.IDisposable
-	{
+	internal class CacheExpires : IDisposable {
 		static int	_intFlush;
 		/// <summary>
 		/// 1 bucket == 1 minute == 10M ticks (1 second) * 60
@@ -29,101 +29,79 @@ namespace System.Web.Caching
 		static long _ticksPerCycle = _ticksPerBucket * 60;
 
 		private ExpiresBucket[] _arrBuckets;
-		private System.Threading.Timer _objTimer;
+		private Timer _objTimer;
 		private Cache _objManager;
 
-		/// <summary>
-		/// Constructor
-		/// </summary>
-		/// <param name="objManager">The cache manager, used when flushing items in a bucket.</param>
-		internal CacheExpires(Cache objManager)
-		{
+		private object _lockObj = new object ();
+
+		internal CacheExpires (Cache objManager) {
 			_objManager = objManager;
 			Initialize();
 		}
 
-		/// <summary>
-		/// Initializes the class.
-		/// </summary>
-		private void Initialize()
-		{
+		private void Initialize () {
 			// Create one bucket per minute
-			_arrBuckets = new ExpiresBucket[60];
+			_arrBuckets = new ExpiresBucket [60];
 
 			byte bytePos = 0;
-			do 
-			{
-				_arrBuckets[bytePos] = new ExpiresBucket(bytePos, _objManager);
+			do {
+				_arrBuckets [bytePos] = new ExpiresBucket (bytePos, _objManager);
 				bytePos++;
 			} while (bytePos < 60);
 
 			// GC Bucket controller
 			_intFlush = System.DateTime.Now.Minute - 1;
-			_objTimer = new System.Threading.Timer(new System.Threading.TimerCallback(GarbageCleanup), null, 10000, 60000);
+			_objTimer = new System.Threading.Timer (new System.Threading.TimerCallback (GarbageCleanup), null, 10000, 60000);
 		}
 
 		/// <summary>
 		/// Adds a Cache entry to the correct flush bucket.
 		/// </summary>
 		/// <param name="objEntry">Cache entry to add.</param>
-		internal void Add (CacheEntry objEntry)
-		{
-			lock(this) 
-			{
-				// If the entry doesn't have a expires time we assume that the entry is due to expire now.
-				if (objEntry.Expires == 0) 
-				{
-					objEntry.Expires = System.DateTime.Now.Ticks;
-				}
-
-                _arrBuckets[GetHashBucket(objEntry.Expires)].Add(objEntry);
-			}
+		internal void Add (CacheEntry objEntry) {
+			long now = DateTime.Now.Ticks;
+			if (objEntry.Expires < now)
+				objEntry.Expires = now;
+
+			_arrBuckets [GetHashBucket (objEntry.Expires)].Add (objEntry);
 		}
 
-		internal void Remove(CacheEntry objEntry)
-		{
-			lock(this) 
-			{
-				// If the entry doesn't have a expires time we assume that the entry is due to expire now.
-				if (objEntry.Expires == 0) 
-				{
-					objEntry.Expires = System.DateTime.Now.Ticks;
-				}
-
-				_arrBuckets[GetHashBucket(objEntry.Expires)].Remove(objEntry);
-			}
+		internal void Remove (CacheEntry objEntry) {
+			if (objEntry.ExpiresBucket != CacheEntry.NoBucketHash)
+				_arrBuckets [objEntry.ExpiresBucket].Remove (objEntry);
 		}
 
-		internal void Update(CacheEntry objEntry, long ticksExpires)
-		{
-			lock(this) 
-			{
-				// If the entry doesn't have a expires time we assume that the entry is due to expire now.
-				if (objEntry.Expires == 0) 
-				{
-					objEntry.Expires = System.DateTime.Now.Ticks;
-				}
-
-				_arrBuckets[GetHashBucket(objEntry.Expires)].Update(objEntry, ticksExpires);
-			}		
+		internal void Update (CacheEntry objEntry, long ticksExpires) {
+			// If the entry doesn't have a expires time we assume that the entry is due to expire now.
+			int oldBucket = objEntry.ExpiresBucket;
+			int newBucket = GetHashBucket (ticksExpires);
+
+			if (oldBucket == CacheEntry.NoBucketHash)
+				return;
+
+			// Check if we need to move the item
+			if (oldBucket != newBucket) {
+				_arrBuckets [oldBucket].Remove (objEntry);
+				objEntry.Expires = ticksExpires;
+				_arrBuckets [newBucket].Add (objEntry);
+			} else
+				_arrBuckets [oldBucket].Update (objEntry, ticksExpires);
 		}
 
-		internal void GarbageCleanup(object State)
-		{
-			ExpiresBucket	objBucket;
+		internal void GarbageCleanup (object State) {
+			ExpiresBucket objBucket;
+			int bucket;
 
-			lock(this)
-			{
-				// Do cleanup of the bucket 
-				objBucket = _arrBuckets[(++_intFlush) % 60];
-			}	
+			// We lock here if FlushExpiredItems take time
+			lock (_lockObj) {
+				bucket = (++_intFlush) % 60;
+			}
 
 			// Flush expired items in the current bucket (defined by _intFlush)
-			objBucket.FlushExpiredItems();
+			_arrBuckets [bucket].FlushExpiredItems ();
 		}
 
-		private int GetHashBucket(long ticks)
-		{
+		private int GetHashBucket (long ticks) {
 			// Get bucket to add expire item into, add one minute to the bucket just to make sure that we get it in the bucket gc
 			return (int) (((((ticks + 60000) % _ticksPerCycle) / _ticksPerBucket) + 1) % 60);
 		}
@@ -131,16 +109,11 @@ namespace System.Web.Caching
 		/// <summary>
 		/// Called by the cache for cleanup.
 		/// </summary>
-		public void Dispose()
-		{
-			lock(this)
-			{
-				// Cleanup the internal timer
-				if (_objTimer != null)
-				{
-					_objTimer.Dispose();
-					_objTimer = null;
-				}
+		public void Dispose () {
+			// Cleanup the internal timer
+			if (_objTimer != null) {
+				_objTimer.Dispose();
+				_objTimer = null;
 			}
 		}
 	}

mcs/class/System.Web/System.Web.Caching/ChangeLog

@@ -1,3 +1,20 @@
+2004-05-27	Patrik Torstensson <[email protected]>
+
+	* CacheEntry.cs,CacheExpires.cs,ExpiresBucket.cs,
+	  Cache.cs : Fixed deadlock issues, fixed
+	  items not correctly being flushed, fixed update
+	  of item when expiration has been updated more
+	  than 1 min (placed in wrong bucket), 
+	  fixed deadlock during cache callback when item 
+	  is removed due to expiriation.
+	  
+	  Rewrite of locking handling in Cache class, leading
+	  to better performance and less bugs.
+	  
+	  This rewrite is due to a number of bugs found
+	  during load and output caching (leading to memory 
+	  leaks and deadlocks)
+
 2004-05-16	Patrik Torstensson <[email protected]>
 
 	* ExpiresBucket.cs: Style changes plus;

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

@@ -3,56 +3,51 @@
 //
 // Author:
 //   Patrik Torstensson ([email protected])
-// Changes:
 //   Daniel Cazzulino [DHC] ([email protected])
 //
-// (C) Copyright Patrik Torstensson, 2001
-//
-namespace System.Web.Caching
-{
+
+using System;
+using System.Collections;
+using System.Threading;
+
+namespace System.Web.Caching {
 	/// <summary>
 	/// Responsible for holding a cache entry in the linked list bucket.
 	/// </summary>
-	internal struct ExpiresEntry
-	{
-		internal CacheEntry _objEntry;
-		internal long _ticksExpires;
+	internal struct ExpiresEntry {
+		internal CacheEntry Entry;
+		internal long TicksExpires;
 		internal int _intNext;
 	}
 
 	/// <summary>
 	/// Holds cache entries that has a expiration in a bucket list.
 	/// </summary>
-	internal class ExpiresBucket
-	{
-		private static int MIN_ENTRIES = 16;
+	internal class ExpiresBucket {
+		private static int MIN_ENTRIES = 4;
 		
-		private byte	_byteID;
-		private int		_intSize;
-		private int		_intCount;
-		private	int		_intNext;
+		private byte _byteID;
+		private int	_intSize;
+		private int	_intCount;
+		private	int	_intNext;
 
-		private Cache	_objManager;
+		private Cache _objManager;
 
 		private ExpiresEntry [] _arrEntries;
 
-		/// <summary>
-		/// A lock to use for syncronized operations. [DHC]
-		/// </summary>
 		private System.Threading.ReaderWriterLock _lock = new System.Threading.ReaderWriterLock();
 
 		/// <summary>
 		/// Keeps a list of indexes in the list which are available to place new items. [DHC]
 		/// </summary>
-		Int32Collection _freeidx = new Int32Collection();
+		private Int32Collection _freeidx = new Int32Collection();
 
 		/// <summary>
 		/// Constructs a new bucket.
 		/// </summary>
 		/// <param name="bucket">Current bucket ID.</param>
 		/// <param name="objManager">Cache manager reponsible for the item(s) in the expires bucket.</param>
-		internal ExpiresBucket(byte bucket, Cache objManager) 
-		{
+		internal ExpiresBucket (byte bucket, Cache objManager) {
 			_objManager = objManager;
 			Initialize(bucket);
 		}
@@ -61,22 +56,18 @@ namespace System.Web.Caching
 		/// Initializes the expires bucket, creates a linked list of MIN_ENTRIES.
 		/// </summary>
 		/// <param name="bucket">Bucket ID.</param>
-		private void Initialize(byte bucket)
-		{
+		private void Initialize (byte bucket) {
 			_byteID = bucket;
 			_intNext = 0;
 			_intCount = 0;
 
-			_arrEntries = new ExpiresEntry[MIN_ENTRIES];
+			_arrEntries = new ExpiresEntry [MIN_ENTRIES];
 			_intSize = MIN_ENTRIES;
 
 			int intPos = 0;
-			do 
-			{
+			do {
 				_arrEntries[intPos]._intNext = intPos + 1;
-				//HACK: [DHC] Use constant defined in Cache.
-				//_arrEntries[intPos]._ticksExpires = System.DateTime.MaxValue.Ticks;
-				_arrEntries[intPos]._ticksExpires = Cache.NoAbsoluteExpiration.Ticks;
+				_arrEntries[intPos].TicksExpires = Cache.NoAbsoluteExpiration.Ticks;
 				
 				intPos++;
 			} while (intPos < _intSize);
@@ -87,11 +78,9 @@ namespace System.Web.Caching
 		/// <summary>
 		/// Expands the bucket linked array list.
 		/// </summary>
-		private void Expand() 
-		{
+		private void Expand () {
 			_lock.AcquireWriterLock(-1);
-			try
-			{
+			try {
 				int oldsize = _intSize;
 				_intSize *= 2;
 
@@ -104,10 +93,9 @@ namespace System.Web.Caching
 				newlist[oldsize - 1]._intNext = oldsize;
 
 				// Initialize positions for the rest of new elements.
-				for (int i = oldsize; i < _intSize; i++)
-				{
+				for (int i = oldsize; i < _intSize; i++) {
 					newlist[i]._intNext = i + 1;
-					newlist[i]._ticksExpires = Cache.NoAbsoluteExpiration.Ticks;
+					newlist[i].TicksExpires = Cache.NoAbsoluteExpiration.Ticks;
 				}
 
 				// Last item signals the expansion of the list.
@@ -116,8 +104,7 @@ namespace System.Web.Caching
 				// Replace the existing list.
 				_arrEntries = newlist;
 			}
-			finally
-			{
+			finally {
 				_lock.ReleaseWriterLock();
 			}
 
@@ -127,61 +114,34 @@ namespace System.Web.Caching
 		/// Adds a cache entry into the expires bucket.
 		/// </summary>
 		/// <param name="objEntry">Cache Entry object to be added.</param>
-		internal void Add(CacheEntry objEntry)
-		{
-			bool dogrow = false;
-
-			// Check the need for expansion or reuse free index.
-			if (_intNext == -1)
-			{
-				if (_freeidx.Count == 0)
-				{
-					dogrow = true;
-				}
-				else
-				{
-					_lock.AcquireReaderLock(-1);
-					try
-					{
-						// Elements may have been removed before the lock statement.
-						if (_freeidx.Count == 0)
-							dogrow = true;
-						else
-						{
-							_intNext = _freeidx[0];
-							_freeidx.Remove(_intNext);
-						}
-					}
-					finally
-					{
-						_lock.ReleaseReaderLock();
+		internal void Add (CacheEntry objEntry) {
+			_lock.AcquireWriterLock(-1);
+			try {
+				if (_intNext == -1) {
+					if (_freeidx.Count == 0)
+						Expand();
+					else {
+						_intNext = _freeidx[0];
+						_freeidx.Remove(_intNext);
 					}
 				}
-			}
-
-			if (dogrow) 
-				Expand();
 			
-			_lock.AcquireWriterLock(-1);
-			try
-			{
-				_arrEntries[_intNext]._ticksExpires = objEntry.Expires;
-				_arrEntries[_intNext]._objEntry = objEntry;
+				_arrEntries[_intNext].TicksExpires = objEntry.Expires;
+				_arrEntries[_intNext].Entry = objEntry;
+
+				objEntry.ExpiresBucket = _byteID;
+				objEntry.ExpiresIndex = _intNext;
+
 				// If there are free indexes in the list, reuse them for the _next value.
-				if (_freeidx.Count != 0)
-				{
-					_intNext = _freeidx[0];
+				if (_freeidx.Count != 0) {
+					_intNext = _freeidx [0];
 					_freeidx.Remove(_intNext);
-				}
-				else
-				{
+				} else
 					_intNext = _arrEntries[_intNext]._intNext;
-				}
 
 				_intCount++;
 			}
-			finally
-			{
+			finally {
 				_lock.ReleaseWriterLock();
 			}
 		}
@@ -190,28 +150,25 @@ namespace System.Web.Caching
 		/// Removes a cache entry from the expires bucket.
 		/// </summary>
 		/// <param name="objEntry">Cache entry to be removed.</param>
-		internal void Remove(CacheEntry objEntry)
-		{
+		internal void Remove(CacheEntry objEntry) {
 			// Check if this is our bucket
-			if (objEntry.ExpiresIndex != _byteID) return;
+			if (objEntry.ExpiresBucket != _byteID) return;
 			if (objEntry.ExpiresIndex == CacheEntry.NoIndexInBucket) return;
 
 			_lock.AcquireWriterLock(-1);
-			try
-			{
+			try {
 				if (_arrEntries.Length < objEntry.ExpiresIndex) return;
 				_intCount--;
 
 				// Push the index as a free one.
 				_freeidx.Add(objEntry.ExpiresIndex);
 
-				_arrEntries[objEntry.ExpiresIndex]._objEntry = null;
+				_arrEntries[objEntry.ExpiresIndex].Entry = null;
 				// Clear bucket-related values from the item.
 				objEntry.ExpiresBucket = CacheEntry.NoBucketHash;
 				objEntry.ExpiresIndex = CacheEntry.NoIndexInBucket;
 			}
-			finally
-			{
+			finally {
 				//Releases both reader & writer locks
 				_lock.ReleaseWriterLock();
 			}
@@ -224,23 +181,20 @@ namespace System.Web.Caching
 		/// </summary>
 		/// <param name="objEntry">Cache entry to update.</param>
 		/// <param name="ticksExpires">New expiration value for the cache entry.</param>
-		internal void Update(CacheEntry objEntry, long ticksExpires)
-		{
+		internal void Update(CacheEntry objEntry, long ticksExpires) {
 			// Check if this is our bucket
-			if (objEntry.ExpiresIndex != _byteID) return;
+			if (objEntry.ExpiresBucket != _byteID) return;
 			if (objEntry.ExpiresIndex == CacheEntry.NoIndexInBucket) return;
 
 			_lock.AcquireWriterLock(-1);
-			try
-			{
+			try {
 				if (_arrEntries.Length < objEntry.ExpiresIndex) return;
 
 				// Proceed to update.
-				_arrEntries[objEntry.ExpiresIndex]._ticksExpires = ticksExpires;
-				_arrEntries[objEntry.ExpiresIndex]._objEntry.Expires = ticksExpires;
+				_arrEntries[objEntry.ExpiresIndex].TicksExpires = ticksExpires;
+				_arrEntries[objEntry.ExpiresIndex].Entry.Expires = ticksExpires;
 			}
-			finally
-			{
+			finally {
 				//Releases both read & write locks
 				_lock.ReleaseWriterLock();
 			}
@@ -249,82 +203,82 @@ namespace System.Web.Caching
 		/// <summary>
 		/// Flushes all cache entries that has expired and removes them from the cache manager.
 		/// </summary>
-		internal void FlushExpiredItems()
-		{
+		internal void FlushExpiredItems() {
 			ExpiresEntry objEntry;
-			CacheEntry [] arrCacheEntries;
-			
-			int		intCachePos;
-			int		intPos;
-			long	ticksNow;
+			ArrayList removeList = null;
+			ArrayList flushList = null;
+			int	intPos;
+			long ticksNow;
 	
-			ticksNow = System.DateTime.Now.Ticks;
-
-			intCachePos = 0;
+			ticksNow = DateTime.Now.Ticks;
 
+			intPos = 0;
 			// Lookup all items that needs to be removed, this is done in a two part
 			// operation to minimize the locking time.
-			_lock.AcquireReaderLock(-1);
-			try
-			{
-				arrCacheEntries = new CacheEntry[_intSize];
-
-				intPos = 0;
-				do 
-				{
-					objEntry = _arrEntries[intPos];
-					if (objEntry._objEntry != null)
+			_lock.AcquireReaderLock (-1);
+			try {
+				do {
+					objEntry = _arrEntries [intPos];
+					if (null != objEntry.Entry && 
+						((objEntry.TicksExpires < ticksNow) || objEntry.Entry.ExpiresBucket != _byteID))
 					{
-						if (objEntry._ticksExpires < ticksNow)
-						{
-							System.Threading.LockCookie ck = _lock.UpgradeToWriterLock(-1);
-							try
-							{
-								//push the index for reuse
-								_freeidx.Add(intPos);
-								arrCacheEntries[intCachePos++] = objEntry._objEntry;
-
-								objEntry._objEntry.ExpiresBucket = CacheEntry.NoBucketHash;
-								objEntry._objEntry.ExpiresIndex = CacheEntry.NoIndexInBucket;
-								objEntry._objEntry = null;
-								_arrEntries [intPos] = objEntry;
-							}
-							finally
-							{
-								_lock.DowngradeFromWriterLock(ref ck);
-							}
-						}
+						if (null == removeList)
+							removeList = new ArrayList ();
+						
+						removeList.Add (objEntry);
 					}
-					
+						
 					intPos++;
 				} while (intPos < _intSize);
 			}
-			finally
-			{
-				_lock.ReleaseReaderLock();
-			}
+			finally {
+				_lock.ReleaseReaderLock ();
+			}			
+
+			if (null != removeList) {
+				flushList = new ArrayList (removeList.Count);
+
+				_lock.AcquireWriterLock (-1);	
+				try {
+					foreach (ExpiresEntry entry in removeList) { 
+						int id = entry.Entry.ExpiresIndex;
+
+						//push the index for reuse
+						_freeidx.Add (id);
+
+						if (entry.Entry.ExpiresBucket == _byteID) {
+							// add to our flush list
+							flushList.Add (entry.Entry);
+
+							// Remove from bucket
+							entry.Entry.ExpiresBucket = CacheEntry.NoBucketHash;
+							entry.Entry.ExpiresIndex = CacheEntry.NoIndexInBucket;
+						} 
+						
+						entry.Entry = null;
+						
+						// Entries is structs, put it back
+						_arrEntries [id] = objEntry;
+					}
+				}
+				finally {
+					_lock.ReleaseWriterLock ();
+				}
 
-			// If we have any entries to remove, go ahead and call the cache manager remove.
-			if (intCachePos > 0)
-			{
-				intPos = 0;
-				do 
-				{
-					_objManager.Remove(arrCacheEntries[intPos].Key, CacheItemRemovedReason.Expired);
+				// We can call this without locks, it can takes time due to callbacks to user code
+				foreach (CacheEntry entry in flushList)
+					_objManager.Remove (entry.Key, CacheItemRemovedReason.Expired);
 
-					intPos++;
-				} while (intPos < intCachePos);
+				flushList = null;
+				removeList = null;
 			}
 		}
 
 		/// <summary>
 		/// Returns the current size of the expires bucket.
 		/// </summary>
-		internal int Size
-		{
-			get 
-			{ 
-				//HACK: reuse the _intSize field!!! [DHC]
+		internal int Size {
+			get { 
 				return _intSize;
 			}
 		}
@@ -332,10 +286,8 @@ namespace System.Web.Caching
 		/// <summary>
 		/// Returns number of items in the bucket.
 		/// </summary>
-		internal int Count
-		{
-			get 
-			{
+		internal int Count {
+			get {
 				return  _intCount;
 			}
 		}
@@ -379,9 +331,8 @@ namespace System.Web.Caching
 		/// An optimized collection for holding <see cref="Int32"/> values.
 		/// </summary>
 		[System.Serializable]
-		private class Int32Collection : System.Collections.ICollection, System.Collections.IList, System.Collections.IEnumerable
-		{
-		#region Private vars & ctors
+			private class Int32Collection : System.Collections.ICollection, System.Collections.IList, System.Collections.IEnumerable {
+			#region Private vars & ctors
 			private const int DefaultMinimumCapacity = 16;
 
 			private System.Int32[] m_array = new System.Int32[DefaultMinimumCapacity];
@@ -389,36 +340,33 @@ namespace System.Web.Caching
 			private int m_version = 0;
 
 			/// <summary />
-			public Int32Collection()
-			{ }
+			public Int32Collection() {
+			}
 
 			/// <summary />
-			public Int32Collection(Int32Collection collection)
-			{ AddRange(collection); }
+			public Int32Collection(Int32Collection collection) {
+				AddRange(collection); }
 
 			/// <summary />
-			public Int32Collection(System.Int32[] array)
-			{ AddRange(array); }
-		#endregion
+			public Int32Collection(System.Int32[] array) {
+				AddRange(array); }
+			#endregion
 
-		#region Public members
+			#region Public members
 
 			/// <summary />
-			public int Count
-			{
-				get
-				{ return m_count; }
+			public int Count {
+				get {
+					return m_count; }
 			}
 
 			/// <summary />
-			public void CopyTo(System.Int32[] array)
-			{
+			public void CopyTo(System.Int32[] array) {
 				this.CopyTo(array, 0);
 			}
 
 			/// <summary />
-			public void CopyTo(System.Int32[] array, int start)
-			{
+			public void CopyTo(System.Int32[] array, int start) {
 				if (m_count > array.GetUpperBound(0)+1-start)
 					throw new System.ArgumentException("Destination array was not long enough.");
 
@@ -427,15 +375,12 @@ namespace System.Web.Caching
 			}
 
 			/// <summary />
-			public System.Int32 this[int index]
-			{
-				get
-				{
+			public System.Int32 this[int index] {
+				get {
 					ValidateIndex(index); // throws
 					return m_array[index]; 
 				}
-				set
-				{
+				set {
 					ValidateIndex(index); // throws
 				
 					++m_version; 
@@ -444,8 +389,7 @@ namespace System.Web.Caching
 			}
 
 			/// <summary />
-			public int Add(System.Int32 item)
-			{			
+			public int Add(System.Int32 item) {			
 				if (NeedsGrowth())
 					Grow();
 
@@ -456,22 +400,19 @@ namespace System.Web.Caching
 			}
 		
 			/// <summary />
-			public void Clear()
-			{
+			public void Clear() {
 				++m_version;
 				m_array = new System.Int32[DefaultMinimumCapacity];
 				m_count = 0;
 			}
 
 			/// <summary />
-			public bool Contains(System.Int32 item)
-			{
+			public bool Contains(System.Int32 item) {
 				return ((IndexOf(item) == -1)?false:true);
 			}
 
 			/// <summary />
-			public int IndexOf(System.Int32 item)
-			{
+			public int IndexOf(System.Int32 item) {
 				for (int i=0; i < m_count; ++i)
 					if (m_array[i].Equals(item))
 						return i;
@@ -479,8 +420,7 @@ namespace System.Web.Caching
 			}
 
 			/// <summary />
-			public void Insert(int position, System.Int32 item)
-			{
+			public void Insert(int position, System.Int32 item) {
 				ValidateIndex(position,true); // throws
 			
 				if (NeedsGrowth())
@@ -494,8 +434,7 @@ namespace System.Web.Caching
 			}
 
 			/// <summary />
-			public void Remove(System.Int32 item)
-			{			
+			public void Remove(System.Int32 item) {			
 				int index = IndexOf(item);
 				if (index < 0)
 					throw new System.ArgumentException("Cannot remove the specified item because it was not found in the specified Collection.");
@@ -504,8 +443,7 @@ namespace System.Web.Caching
 			}
 
 			/// <summary />
-			public void RemoveAt(int index)
-			{
+			public void RemoveAt(int index) {
 				ValidateIndex(index); // throws
 			
 				++m_version;
@@ -519,12 +457,10 @@ namespace System.Web.Caching
 			// Public helpers (just to mimic some nice features of ArrayList)
 
 			/// <summary />
-			public int Capacity
-			{
-				get
-				{ return m_array.Length; }
-				set
-				{
+			public int Capacity {
+				get {
+					return m_array.Length; }
+				set {
 					if (value < m_count) value = m_count;
 					if (value < DefaultMinimumCapacity) value = DefaultMinimumCapacity;
 
@@ -539,8 +475,7 @@ namespace System.Web.Caching
 			}
 
 			/// <summary />
-			public void AddRange(Int32Collection collection)
-			{
+			public void AddRange(Int32Collection collection) {
 				++m_version;
 
 				Capacity += collection.Count;
@@ -549,132 +484,111 @@ namespace System.Web.Caching
 			}
 
 			/// <summary />
-			public void AddRange(System.Int32[] array)
-			{
+			public void AddRange(System.Int32[] array) {
 				++m_version;
 
 				Capacity += array.Length;
 				System.Array.Copy(array, 0, this.m_array, m_count, array.Length);
 				m_count += array.Length;
 			}
-		#endregion
+			#endregion
 
-		#region Private helper methods
-			private void ValidateIndex(int index)
-			{
+			#region Private helper methods
+			private void ValidateIndex(int index) {
 				ValidateIndex(index,false);
 			}
 
-			private void ValidateIndex(int index, bool allowEqualEnd)
-			{
+			private void ValidateIndex(int index, bool allowEqualEnd) {
 				int max = (allowEqualEnd)?(m_count):(m_count-1);
 				if (index < 0 || index > max)
 					throw new System.ArgumentOutOfRangeException("Index was out of range.  Must be non-negative and less than the size of the collection.", (object)index, "Specified argument was out of the range of valid values.");
 			}
 
-			private bool NeedsGrowth()
-			{
+			private bool NeedsGrowth() {
 				return (m_count >= Capacity);
 			}
 
-			private void Grow()
-			{
+			private void Grow() {
 				if (NeedsGrowth())
 					Capacity = m_count*2;
 			}
 
-			private bool NeedsTrimming()
-			{
+			private bool NeedsTrimming() {
 				return (m_count <= Capacity/2);
 			}
 
-			private void Trim()
-			{
+			private void Trim() {
 				if (NeedsTrimming())
 					Capacity = m_count;
 			}
-		#endregion
+			#endregion
 
-		#region System.Collections.ICollection implementation
-			bool System.Collections.ICollection.IsSynchronized
-			{
-				get
-				{ return m_array.IsSynchronized; }
+			#region System.Collections.ICollection implementation
+			bool System.Collections.ICollection.IsSynchronized {
+				get {
+					return m_array.IsSynchronized; }
 			}
 
-			object System.Collections.ICollection.SyncRoot
-			{
-				get
-				{ return m_array.SyncRoot; }
+			object System.Collections.ICollection.SyncRoot {
+				get {
+					return m_array.SyncRoot; }
 			}
 
-			void System.Collections.ICollection.CopyTo(System.Array array, int start)
-			{
+			void System.Collections.ICollection.CopyTo(System.Array array, int start) {
 				this.CopyTo((System.Int32[])array, start);
 			}
-		#endregion
+			#endregion
 
-		#region System.Collections.IList implementation
-			bool System.Collections.IList.IsFixedSize
-			{
-				get
-				{ return false; }
+			#region System.Collections.IList implementation
+			bool System.Collections.IList.IsFixedSize {
+				get {
+					return false; }
 			}
 
-			bool System.Collections.IList.IsReadOnly
-			{
-				get
-				{ return false; }
+			bool System.Collections.IList.IsReadOnly {
+				get {
+					return false; }
 			}
 
-			object System.Collections.IList.this[int index]
-			{
+			object System.Collections.IList.this[int index] {
 				get { return (object)this[index]; }
 				set { this[index] = (System.Int32)value; }
 			}
 
-			int System.Collections.IList.Add(object item)
-			{
+			int System.Collections.IList.Add(object item) {
 				return this.Add((System.Int32)item);
 			}
 
-			bool System.Collections.IList.Contains(object item)
-			{
+			bool System.Collections.IList.Contains(object item) {
 				return this.Contains((System.Int32)item);
 			}
 
-			int System.Collections.IList.IndexOf(object item)
-			{
+			int System.Collections.IList.IndexOf(object item) {
 				return this.IndexOf((System.Int32)item);
 			}
 
-			void System.Collections.IList.Insert(int position, object item)
-			{
+			void System.Collections.IList.Insert(int position, object item) {
 				this.Insert(position, (System.Int32)item);
 			}
 
-			void System.Collections.IList.Remove(object item)
-			{
+			void System.Collections.IList.Remove(object item) {
 				this.Remove((System.Int32)item);
 			}
-		#endregion
+			#endregion
 
-		#region System.Collections.IEnumerable and enumerator implementation
+			#region System.Collections.IEnumerable and enumerator implementation
 			/// <summary />
-			public Enumerator GetEnumerator()
-			{
+			public Enumerator GetEnumerator() {
 				return new Enumerator(this);
 			}
 
-			System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator()
-			{
+			System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() {
 				return GetEnumerator();
 			}
 
 			// Nested enumerator class
 			/// <summary />
-			public class Enumerator : System.Collections.IEnumerator
-			{
+			public class Enumerator : System.Collections.IEnumerator {
 				private Int32Collection m_collection;
 				private int m_index;
 				private int m_version;
@@ -682,23 +596,20 @@ namespace System.Web.Caching
 				// Construction
 	
 				/// <summary />
-				public Enumerator(Int32Collection tc)
-				{
+				public Enumerator(Int32Collection tc) {
 					m_collection = tc;
 					m_index = -1;
 					m_version = tc.m_version;
 				}
 	
 				/// <summary />
-				public System.Int32 Current
-				{
-					get
-					{ return m_collection[m_index]; }
+				public System.Int32 Current {
+					get {
+						return m_collection[m_index]; }
 				}
 	
 				/// <summary />
-				public bool MoveNext()
-				{
+				public bool MoveNext() {
 					if (m_version != m_collection.m_version)
 						throw new System.InvalidOperationException("Collection was modified; enumeration operation may not execute.");
 
@@ -707,22 +618,21 @@ namespace System.Web.Caching
 				}
 	
 				/// <summary />
-				public void Reset()
-				{
+				public void Reset() {
 					if (m_version != m_collection.m_version)
 						throw new System.InvalidOperationException("Collection was modified; enumeration operation may not execute.");
 
 					m_index = -1;
 				}
 
-				object System.Collections.IEnumerator.Current
-				{
-					get
-					{ return (object)(this.Current); }
+				object System.Collections.IEnumerator.Current {
+					get {
+						return (object)(this.Current); }
 				}
 			}
-		#endregion
+			#endregion
 		}
 		#endregion
 	}
 }
+	

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

@@ -68,9 +68,7 @@ namespace System.Web.Caching {
 				context.Response.StatusDescription = c.StatusDescription;
 				
 				app.CompleteRequest ();
-			} else if (c != null) {
-				context.Cache.Remove (key);
-			}
+			} 
 
 		leave:
 			HttpAsyncResult result = new HttpAsyncResult (cb,this);