Edited DontUseLockedRegionOutsideMethodRule description.

svn path=/trunk/mono-tools/; revision=140668

2 files changed, 52 insertions, 13 deletions

diff --git a/gendarme/rules/Gendarme.Rules.Concurrency/ChangeLog b/gendarme/rules/Gendarme.Rules.Concurrency/ChangeLog
index 47a469bc..28614771 100644
--- a/gendarme/rules/Gendarme.Rules.Concurrency/ChangeLog
+++ b/gendarme/rules/Gendarme.Rules.Concurrency/ChangeLog
@@ -1,3 +1,8 @@
+2009-08-25  Jesse Jones  <jesjones@mindspring.com>
+
+	* DontUseLockedRegionOutsideMethodRule.cs: Edited 
+	the description.
+
 2009-07-08  Jesse Jones  <jesjones@mindspring.com>
 
 	* *Rule.cs: Edited most of the rule descriptions.
diff --git a/gendarme/rules/Gendarme.Rules.Concurrency/DontUseLockedRegionOutsideMethodRule.cs b/gendarme/rules/Gendarme.Rules.Concurrency/DontUseLockedRegionOutsideMethodRule.cs
index 202ee7c7..872c5274 100644
--- a/gendarme/rules/Gendarme.Rules.Concurrency/DontUseLockedRegionOutsideMethodRule.cs
+++ b/gendarme/rules/Gendarme.Rules.Concurrency/DontUseLockedRegionOutsideMethodRule.cs
@@ -40,28 +40,48 @@ using Mono.Cecil.Cil;
 
 namespace Gendarme.Rules.Concurrency {
 
-	// TODO: The summary does a really poor job describing what the rule is looking for.
-	// It also does a poor job describing what the problem the rule addresses is.
-
 	/// <summary>
-	/// This rule ensures method atomicity.  You should put the
-	/// Monitor.Enter and Monitor.Exit call in the same method, otherwise
-	/// there may be headaches related to concurrency issues.
+	/// This rule will fire if a method calls <c>System.Threading.Monitor.Enter</c>, 
+	/// but not <c>System.Threading.Monitor.Exit</c>. This is a bad idea for public
+	/// methods because the callers must (indirectly) manage a lock which they do not
+	/// own. This increases the potential for problems such as dead locks because 
+	/// locking/unlocking may not be done together, the callers must do the unlocking
+	/// even in the presence of exceptions, and it may not be completely clear that
+	/// the public method is acquiring a lock without releasing it.
+	///
+	/// This is less of a problem for private methods because the lock is managed by
+	/// code that owns the lock. So, it's relatively easy to analyze the class to ensure
+	/// that the lock is locked and unlocked correctly and that any invariants are 
+	/// preserved when the lock is acquired and after it is released. However it is
+	/// usually simpler and more maintainable if methods unlock whatever they lock.
 	/// </summary>
 	/// <example>
 	/// Bad example:
 	/// <code>
 	/// class BadExample {
 	/// 	int producer = 0;
+	/// 	object lock = new object();
+	///
+	/// 	// This class is meant to be thread safe, but in the interests of
+	/// 	// performance it requires clients to manage its lock. This allows
+	/// 	// clients to grab the lock, batch up edits, and release the lock
+	/// 	// when they are done. But this means that the clients must
+	/// 	// now (implicitly) manage the lock which is problematic, especially
+	/// 	// if this object is shared across threads.
+	/// 	public void BeginEdits ()
+	/// 	{
+	///		Monitor.Enter (lock);
+	///	}
 	///
-	/// 	public void EnteringMethod ()
+	/// 	public void AddProducer ()
 	/// 	{
-	///		Monitor.Enter ();
+	/// 		// Real code would either assert or throw if the lock is not held. 
 	///		producer++;
 	///	}
-	///     public void ExitingMethod ()
+	///
+	/// 	public void EndEdits ()
 	/// 	{
-	///		Monitor.Exit ();
+	///		Monitor.Exit (lock);
 	///	}
 	/// }
 	/// </code>
@@ -71,12 +91,26 @@ namespace Gendarme.Rules.Concurrency {
 	/// <code>
 	/// class GoodExample {
 	/// 	int producer = 0;
+	/// 	object mutex = new object();
 	///	
 	///	public void AddProducer ()
 	///	{
-	///		Monitor.Enter ();
-	///		producer++;
-	///		Monitor.Exit ();
+	/// 		// We need a try block in case the assembly is compiled with
+	/// 		// checked arithmetic.
+	///		Monitor.Enter (mutex);
+	/// 		try {
+	///			producer++;
+	/// 		} finally {
+	///			Monitor.Exit (mutex);
+	/// 		}
+	///	}
+	///	
+	///	public void AddProducer2 ()
+	///	{
+	/// 		// Same as the above, but with C# sugar.
+	///		lock (mutex) {
+	///			producer++;
+	/// 		}
 	///	}
 	/// }
 	/// </code>