Binding code for the standard Dispose pattern?

Question

I recently read Effective C # and several other such books / blogs, and when I talked about the standard Dispose template (which I’m already using), they all recommend using the class variable “dispose” (as defined in this example MSDN code) at the beginning of each method. Essentially, to ensure that after calling Dispose, any attempt to use the object will result in an ObjectDisposedException. This makes sense, but is a huge amount of manual labor in a fairly large code base and relies on people who remember to do it. Therefore, I am looking for a better way.

I recently ran into and started using notifypropertyweaver , which automatically fills all the template code for calling PropertyChanged handlers (it works like msbuild, which does not require an additional delivery dependency). I wonder if anyone knows of a similar solution for a standard placement template. What this would essentially be is to take the variable name as config ( bool disposed in the example with the MSDN sample) and add the following code to each method that is not Finalizer or named Dispose in each class that implements IDisposable:

 if(disposed) throw new ObjectDisposedException(); 

Is there such a thing? Alternatively, what do people do to achieve this in their code, manually add an if-statement?

Goal refinement
A great need for this is not some kind of “best practice”, but we do have users who do not properly manage the life cycles of our facilities. Right now they just get NullReference or some other thing from the base resource, which may mean that we have an error in our library, I want to tell them what exactly they are creating the problem and how they break it (considering that I place to know). Therefore, suggestions that users of our types are the ones who should take care of this are not very effective here.

Answer 1

Update: My initial answer didn’t really answer the question, so here is another try ...

To solve the root problem, developers forget to drop ObjectDisposedExceptions , perhaps automatic testing of the device will do the trick. If you want to strictly comply with the requirement that all methods / properties immediately throw an ObjectDisposedException if Dispose has already been called, you can use the following unit test. Just specify the assemblies you want to test. You will probably need to change the IsExcluded method as needed, and object bullying may not work in all cases.

 using System; using System.Collections.Generic; using System.Linq; using System.Reflection; using MbUnit.Framework; using Moq; [TestFixture] public class IDisposableTests { [Test] public void ThrowsObjectDisposedExceptions() { var assemblyToTest = Assembly.LoadWithPartialName("MyAssembly"); // Get all types that implement IDisposable var disposableTypes = from type in assemblyToTest.GetTypes() where type.GetInterface(typeof(IDisposable).FullName) != null select type; foreach (var type in disposableTypes) { // Try to get default constructor first... var constructor = type.GetConstructor(Type.EmptyTypes); if (constructor == null) { // Otherwise get first parameter based constructor... var constructors = type.GetConstructors(); if (constructors != null && constructors.Length > 0) { constructor = constructors[0]; } } // If there is a public constructor... if (constructor != null) { object instance = Activator.CreateInstance(type, GetDefaultArguments(constructor)); (instance as IDisposable).Dispose(); foreach (var method in type.GetMethods()) { if (!this.IsExcluded(method)) { bool threwObjectDisposedException = false; try { method.Invoke(instance, GetDefaultArguments(method)); } catch (TargetInvocationException ex) { if (ex.InnerException.GetType() == typeof(ObjectDisposedException)) { threwObjectDisposedException = true; } } Assert.IsTrue(threwObjectDisposedException); } } } } } private bool IsExcluded(MethodInfo method) { // May want to include ToString, GetHashCode. // Doesn't handle checking overloads which would take more // logic to compare parameters etc. if (method.Name == "Dispose" || method.Name == "GetType") { return true; } return false; } private object[] GetDefaultArguments(MethodBase method) { var arguments = new List<object>(); foreach (var parameter in method.GetParameters()) { var type = parameter.ParameterType; if (type.IsValueType) { arguments.Add(Activator.CreateInstance(type)); } else if (!type.IsSealed) { dynamic mock = Activator.CreateInstance(typeof(Mock<>).MakeGenericType(type)); arguments.Add(mock.Object); } else { arguments.Add(null); } } return arguments.ToArray(); } } 

---

The original answer . It doesn't seem like there is something like NotifyPropertyWeaver for IDisposable , so if you want you to need to create a similar project yourself. You can save yourself a little work by having the base class Disposable, as in this blog post . Then you have only one liner at the top of each method: ThrowExceptionIfDisposed() .

However, none of the possible solutions seems right or seems necessary. In general, throwing an ObjectDisposedException not required, which is often. I did a quick search in Reflector, and an ObjectDisposedException is ObjectDisposedException directly by only 6 types in BCL, and for a sample outside BCL System.Windows.Forms only one type that throws: Cursor when receiving Handle.

Basically, you only need to throw an ObjectDisposedException if your object call fails because Dispose has already been called, for example, if you set some field to null, which is necessary for a method or property. ObjectDisposedException will be more informative than a random NullReferenceException , but if you do not clear unmanaged resources, there is usually no need to set the empty string to zero. Most of the time, if you just call Dispose on other objects, you need to throw an ObjectDisposedException .

Here is a trivial example: you can explicitly throw an ObjectDisposedException :

 public class ThrowObjectDisposedExplicity : IDisposable { private MemoryStream stream; public ThrowObjectDisposedExplicity() { this.stream = new MemoryStream(); } public void DoSomething() { if (this.stream == null) { throw new ObjectDisposedException(null); } this.stream.ReadByte(); } protected virtual void Dispose(bool disposing) { if (disposing) { if (this.stream != null) { this.stream.Dispose(); this.stream = null; } } } public void Dispose() { this.Dispose(true); GC.SuppressFinalize(this); } } 

With the above code, although there really is no need to set the stream to null. You can simply rely on MemoryStream.ReadByte() to throw an ObjectDisposedException on it, as in the code below:

 public class ThrowObjectDisposedImplicitly : IDisposable { private MemoryStream stream; public ThrowObjectDisposedImplicitly() { this.stream = new MemoryStream(); } public void DoSomething() { // This will throw ObjectDisposedException as necessary this.stream.ReadByte(); } protected virtual void Dispose(bool disposing) { if (disposing) { this.stream.Dispose(); } } public void Dispose() { this.Dispose(true); GC.SuppressFinalize(this); } } 

The first strategy to set the thread to null may make sense in some cases, for example, if you know that the object will throw an exception if Dispose is called multiple times. In this case, you want to be protected and make sure that your class does not throw an exception on multiple Dispose calls. Also, I can't think of any other cases where you need to set the fields to zero, which will probably require throwing ObjectDisposedExceptions .

Throwing an ObjectDisposedException not often required and should be carefully considered, so the tool you need is probably not needed. Use the Microsoft libraries as an example; see which methods actually implement throw ObjectDisposedException when the type implements IDisposable .

Note. GC.SuppressFinalize(this); is not strictly necessary because there is no finalizer, but it has stayed there since the subclass can implement the finalizer. If the class was marked as sealed , then GC.SupressFinalize could be safely removed.

Answer 2

Instead of using NotifyPropertyWeaver, I suggest you solve your problem differently. You claim that you have two problems:

• People will forget to implement the pattern.
• You want to avoid the cost of implementing it many times in a large code base.

To pay the cost of re-writing very similar code, I suggest you create and use code snippets in Visual Studio. The problem the fragments are trying to find exactly matches # 2 in the list above. See this article on the MSDN article for instructions.

The C # 1 problem in the above list is that the IDisposable template is not always 100% necessary for your classes. This makes static analysis difficult to “catch” if a given class should be one-time. (edit: thinking about it for a minute, it's actually not that hard to check. If your current class contains IDisposable instances, your class should be IDisposable)

Because of this, I recommend that you use a code review to catch those cases where a developer needs to make his class available. You can add it to the checklist of items that the developer himself must check before requesting a code review.