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?

Martella

Answer 1

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).

Necrophobia

It irks me that .net languages don't provide a nice way of knowing whether a finally block is running because of an exception (much less what it is). I would posit that a substantial plurality if not a majority of catch blocks should either be fault blocks or conditional code in finally; code should only catch an exception if it has a reasonable expectation of resolving it, or will actively wrap it, as opposed to simply doing a blank throw. Prime example: an exception within an IDisposable object's constructor should clean up the object, but should not be "caught". – Dwt

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.

Reactionary

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.

Gallipot

Ah, good old "let the user decide." AKA "I refuse to make any decision because there's a chance I might be wrong." All this does is move the problem upstream, forcing consumers of your class to think about something that they really don't care about at the time. – Necrophobia

Obviously you're a fan of the Henry Ford "You can have it any color you want as long as it's black" design pattern:-) Sometimes customer choice can be good too! I doubt there would be many takers for the second constructor - but without knowing the application, as you say, there isn't a right answer. – Gallipot

Actually, your code is a bad practice, as described in the Framework Design Guidelines. Here is a section from the book: blogs.msdn.com/b/kcwalina/archive/2005/03/16/396787.aspx You can see the following paragraph there: "Do not have public members that can either throw or not based on some option. Type GetType(string name, bool throwOnError)". – Alteration

Answer 4

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.

Suggestibility

Antivirus is a good point - I hadn't really thought about that. It makes it a much more murky question as to whether we should consider it to be a critical failure if we can't delete the directory. However, talking about the user creating files in our temporary directory - is that something applications normally guard against? After all, the user can break all sorts of things by messing about with an application's private files while it's running - how many applications are actually required to be robust to this sort of thing? – Martella

Answer 5

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.

Polaroid

I'm beginning to think this may be best, but do you have any particular reasoning? Is it because it's particularly likely to fail in this situation? – Martella

Answer 6

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.

Ironmonger

Answer 7

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.

Dozer

I think @Martella understands the disposal pattern, (s?)he's concerned with the consequences of exceptions that could be thrown within the Dispose method. – Reactionary