How should I handle exceptions in my Dispose() method?

Question

I'd like to provide a class to manage creation and subsequent deletion of a temporary directory. Ideally, I'd like it to be usable in a using block to ensure that the directory gets deleted again regardless of how we leave the block:

static void DoSomethingThatNeedsATemporaryDirectory()
{
    using (var tempDir = new TemporaryDirectory())
    {
        // Use the directory here...
        File.WriteAllText(Path.Combine(tempDir.Path, "example.txt"), "foo\nbar\nbaz\n");
        // ...
        if (SomeCondition)
        {
            return;
        }
        if (SomethingIsWrong)
        {
            throw new Exception("This is an example of something going wrong.");
        }
    }
    // Regardless of whether we leave the using block via the return,
    // by throwing and exception or just normally dropping out the end,
    // the directory gets deleted by TemporaryDirectory.Dispose.
}

Creating the directory is no problem. The problem is how to write the Dispose method. When we try to delete the directory, we might fail; for example because we still have a file open in it. However, if we allow the exception to propagate, it might mask an exception that occurred inside the using block. In particular, if an exception occurred inside the using block, it might be one that caused us to be unable to delete the directory, but if we mask it we have lost the most useful information for fixing the problem.

It seems we have four options:

1. Catch and swallow any exception when trying to delete the directory. We might be left unaware that we're failing to clean up our temporary directory.
2. Somehow detect if the Dispose is running as part of stack unwinding when an exception was thrown and if so either suppress the IOException or throw an exception that amalgamates the IOException and whatever other exception was thrown. Might not even be possible. (I thought of this in part because it would be possible with Python's context managers, which are in many ways similar to .NET's IDisposable used with C#'s using statement.)
3. Never suppress the IOException from failing to delete the directory. If an exception was thrown inside the using block we will hide it, despite there being a good chance that it had more diagnostic value than our IOException.
4. Give up on deleting the directory in the Dispose method. Users of the class must remain responsible for requesting deletion of the directory. This seems unsatisfactory as a large part of the motivation for creating the class was to lessen the burden of managing this resource. Maybe there's another way to provide this functionality without making it very easy to screw up?

Is one of these options clearly best? Is there a better way to provide this functionality in a user-friendly API?

Answer 1

To use the type in a using statement, you want to implement the IDisposable pattern.

For creating the directory itself, use Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData) as a base and a new Guid as the name.

Answer 2

Ultimately, I would suggest it's best to follow FileStream as a guideline, which equates to options 3 and 4: close files or delete directories in your Dispose method, and allow any exceptions that occur as part of that action to bubble up (effectively swallowing any exceptions that occurred inside the using block) but allow for manually closing the resource without the need for a using block should the user of the component so choose.

Unlike MSDN's documentation of FileStream, I suggest you heavily document the consequences that could occur should the user chose to go with a using statement.

Answer 3

A question to ask here is whether the exception can be usefully handled by the caller. If there is nothing the use can reasonably do (manually delete the file in use in the directory?) it may be better to log the error and forget about it.

To cover both cases, why not have two constructors (or an argument to the constructor)?

public TemporaryDirectory()
: this( false )
{
}

public TemporaryDirectory( bool throwExceptionOnError )
{
}

Then you can push the decision off to the user of the class as to what the appropriate behavior might be.

One common error will be a directory which cannot be deleted because a file inside it is still in use: you could store a list of undeleted temporary directories and allow the option of a second explicit attempt at deletion during program shutdown (eg. a TemporaryDirectory.TidyUp() static method). If the list of problematic directories is non-empty the code could force a garbage collection to handle unclosed streams.

Answer 4

Instead of thinking of this as a special class implementing IDisposable, think of what it would be like in terms of normal program flow:

Directory dir = Directory.CreateDirectory(path);
try
{
    string fileName = Path.Combine(path, "data.txt");
    File.WriteAllText(fileName, myData);
    UploadFile(fileName);
    File.Delete(fileName);
}
finally
{
    Directory.Delete(dir);
}

How should this behave? It's the exact same question. Do you leave the content of the finally block as-is, thereby potentially masking an exception that occurs in the try block, or do you wrap the Directory.Delete in its own try-catch block, swallowing any exception in order to prevent masking the original?

I don't think there's any right answer - the fact is, you can only have one ambient exception, so you have to pick one. However, the .NET Framework does set some precedents; one example is WCF service proxies (ICommunicationObject). If you attempt to Dispose a channel that is faulted, it throws an exception and will mask any exception that is already on the stack. If I'm not mistaken, TransactionScope can do this too.

Of course, this very behaviour in WCF has been an endless source of confusion; most people actually consider it very annoying if not broken. Google "WCF dispose mask" and you'll see what I mean. So perhaps we shouldn't always try to do things the same way Microsoft does them.

Personally, I'm of the mind that Dispose should never mask an exception already on the stack. The using statement is effectively a finally block and most of the time (there are always edge cases), you would not want to throw (and not catch) exceptions in a finally block, either. The reason is simply debugging; it can be extremely hard to get to the bottom of an issue - especially an issue in production where you can't step through the source - when you don't even have the ability to find out where exactly the app is failing. I've been in this position before and I can confidently say that it will drive you completely and utterly insane.

My recommendation would be either to eat the exception in Dispose (log it, of course), or actually check to see if you're already in a stack-unwinding scenario due to an exception, and only eat subsequent exceptions if you know that you'll be masking them. The advantage of the latter is that you don't eat exceptions unless you really have to; the disadvantage is that you've introduced some non-deterministic behaviour into your program. Yet another trade-off.

Most people will probably just go with the former option and simply hide any exception occurring in finally (or using).

Answer 5

You cannot rely on the assumption that you can somehow remove your directory. Some other process/the user/whatever can create a file in it in the meantime. An antivirus may be busy checking files in it, etc.

The best thing you can do is to have not only temporary directory class, but temporary file class (which is to be created inside using block of your temporary directory. The temporary file classes should (attempt to) delete the corresponding files on Dispose. This way you are guaranteed that at least an attempt of cleaning up has been done.

Answer 6

Assuming that the created directory is located in a system temporary folder like the one returned by Path.GetTempPath then I would implement the Dispose in order to not throw an exception if the deletion of the temporary directory fails.

Update: I would take this option based on the fact that the operation could fail because off an external interference, like a lock from another process and also since the directory is placed in the system temporary directory I would not see an advantage of throwing an exception.

What would be a valid response to that exception? Trying to delete the directory again is not reasonable and if the reason is a lock from another process that it's something that it's not directly under your control.

Answer 7

I would say throwing an exception from the destructor for a locked file boils down to using using an exception to report an expected outcome - you shouldn't do it.

However if something else happens e.g. a variable is null, you may really have an error, and then the exception is valuable.

If you anticipate locked files, and there is something you, or potentially your caller, can do about them, then you need to include that response in your class. If you can respond, then just do it in the disposable call. If your caller may be able to respond, provide your caller with a way to to this e.g. a TempfilesLocked event.