C# dispose unmanaged resource outside "using" and "try/finalize" block

Question

I want a method to return an unmanaged resource and later in the program dispose of that resource. Does the following implementation do what I intend ?

class DocxManager
{
    // DocX is the unmanaged resource
    public Docx createDocX(string pathToFile)
    {
       return new DocX(pathToFile);
    }

    public void makeChangesAndSaveDocX(DocX wordDoc)
    {
       wordDoc.Save();
       // IS THIS WAY THE UNMANAGED RESOURCE FREED CORRECTLY AND NO MEMORY LEAK ?
       ((IDisposable)wordDoc).Dispose();
    }
}

Answer 1

First off all, you seem to be misunderstanding the concept of managed and unmanaged resources. wordDoc is not an unmanaged resource, its a managed resource which happens to either directly hold an unmanaged resource or acts as a wrapper around some other IDisposable object (you don't care wich of the two is true). Its important you are clear on this because otherwise you will not implement correctly the IDisposable pattern when you need to. Read this for a very instructive answer on the subject and a few chuckles courtesy of Eric Lippert.

Does the following implementation do what I intend ?

No, it does not and to make things worse, the contract of DocXManager is simply horrible (more on that later).

What hapens if wordDoc.Save() throws and exception because the file is being used by another process, or maybe you've run out of space in the hard drive, or you've lost connection, etc.?

If the thrown exception is not recoverable (it's not handled anywhere in your code or it is but you will terminate it ASAP) then it's not really an issue and the runtime will clean up everything behind you when the process is terminated. On the other hand, if the exception is handled (warn the user the file is in use, the directory is not available, etc.) and the process keeps running then you've just maybe leaked a resource.

How to avoid this? Use a try-finally block:

public void makeChangesAndSaveDocX(DocX wordDoc)
{
   try
   {
       wordDoc.Save();
   }
   finally
   {
      ((IDisposable)wordDoc).Dispose();
   }
} 

Now you are sure that Dispose() will be called in any recoverable scenario.

So, is this good enough? Well....not exactly. The problem here is that makeChangesAndSaveDocX's (that should be MakeChangesAndSaveDocX by the way) contract is unclear. Who is responsible of disposing wordDoc? MakeChangesAndSaveDocX or the caller? Why one or the other? How does the consumer know that he doesn't need to worry about wordDoc once he's called MakeChangesAndSaveDocX? Or how is he supposed to know he can't use wordDoc after calling the public method MakeChangesAndSaveDocX? Why does DocXManager assume that the consumer will not need to use wordDoc after calling MakeChangesAndSaveDocX? Yuck...this is a mess.

I'd recommend you reconsider your approach and do one of the following:

1. If the method signature MakeChangesAndSaveDocX(DocX wordDoc) makes sense (somebody else can own wordDoc), then do not dispose wordDoc in MakeChangesAndSaveDocX. Let the caller carry that burden, he should be responsible because the object belongs to him not to MakeChangesAndSaveDocX.

2. If on the other hand, it makes no sense that somebody else who is not DocXManager owns wordDoc then wordDoc should be part of the state of DocXManager and you should reconsider the implementation of DocXManager to something allong the following lines:

    public class DocXManager: IDisposable
    {
        private readonly DocX docX;
        private bool disposed;
   
        public DocXManager(string filePath)
        {
            docX = new DocX(filePath);
        }
   
        public void MakeChangesAndSaveDocX()
        {
            if (disposed) 
               throw new ObjectDisposedException();
   
            docX.Save();
        }
   
        public void FrobDocX(Frobber frobber)
        {
            if (disposed) 
                throw new ObjectDisposedException();
   
            frobber.Frob(docX);
        }
   
        public void Dispose()
        {
            if (disposed)
               return;
   
            Dispose(true);
            disposed = true;
            GC.SupressFinalize(this);   
        }
   
        public void Dispose(bool disposing)
        {
            if (disposing)
            {
                //we can sefely dispose managed resources here
                ((IDisposable)docX).Dispose();
            }
   
            //unsafe to clean up managed resources here, only clean up unmanaged resources if any.
        }
   
        ~DocXManager()
        {
            Dispose(false);
        }
   } 
   

Now you've got a clear contract; DocManagerX is responsible of disposing correctly of DocX and the consumer is responsible of disposing correctly of any instance of DocManagerX he might use. Once responsibilities are clear, its easier to reason about code correctness and who should do what.

You'd use the manager in the following way:

using (var manager = new DocXManager(path))
{
     manager.FrobDoxX(frobber);
     manager.MakeChangesAndSaveDocX();
} //manager is guaranteed to be disposed at this point (ignoring scenarios where finally blocks are not executed; StackOverflowException, OutOfMemoryException, etc.)