4

Narrowed question:

I need to implement reusable functionality that initializes a temporary file, runs some custom logic provided by the user, and then deletes the file. I can do this either through a static utility method that takes the user's logic as a delegate, or through a class that implements IDisposable to perform cleanup during Dispose. The two approaches would be used as follows:

// Delegate-parameterized method approach:
TempFile.Using(filePath =>
{
    // use temporary file here
});

// IDisposable implementation approach:
using (var tempFile = new TempFile())
{
    // use temporary file here
}

Which are the advantages and drawbacks to each approach? Which one should I use?

Original question:

I am developing some general-purpose functionalities that require a fixed “initialization – custom logic – clean-up” sequence. The clean-up must be performed as part of the same construct used for executing the operation; I don't want to confer the responsibility for calling a Cleanup or Close method to the user.

For example, I might want to provide functionality that automatically creates and deletes a temporary file. The simplest way to implement this is through a method that takes an Action<T> delegate:

public static void UsingTempFile(Action<string> action)
{
    // initialization
    string tempFilePath = Path.GetTempFileName();

    try
    {
        action(tempFilePath);
    }
    finally
    {
        // clean-up
        File.Delete(tempFilePath);
    }
}

This could be used like so:

UsingTempFile(filePath =>
{
     File.WriteAllText(filePath, "Hello world");
     string text = File.ReadAllText(filePath);
});

However, this approach often requires me to implement four method overloads to support returned results and anonymous functions:

public static void UsingTempFile(Action<string> action) { /* ... */ }
public static TResult UsingTempFile<TResult>(Func<string, TResult> func) { /* ... */ }
public static async Task UsingTempFile(Func<string, Task> asyncAction) { /* ... */ }
public static async Task<TResult> UsingTempFile<TResult>(Func<string, Task<TResult>> asyncFunc) { /* ... */ }

The former three can implemented as simple wrappers that call the last overload. However, in a public API, they still need to be documented and unit-tested, substantially cluttering my codebase.

An alternative approach is to design an instantiatable class that implements IDisposable to represent the operation. This class would perform the initialization in its constructor, and the clean-up in its Dispose method. It could then be consumed like so:

using (var tempFile = new TempFile())
{        
     File.WriteAllText(tempFile.FilePath, "Hello world");
     string text = File.ReadAllText(tempFile.FilePath);
}

The advantage of this approach is that the C# compiler automatically handles all my four cases ‒ the user can specify return and await keywords in the using block.

However, I often need to be able to throw exceptions from my clean-up logic – like IOException being thrown from File.Delete if the temporary file is still in use in the above example. The MSDN documentation for the Dispose Pattern states:

X AVOID throwing an exception from within Dispose(bool) except under critical situations where the containing process has been corrupted (leaks, inconsistent shared state, etc.).

Users expect that a call to Dispose will not raise an exception.

If Dispose could raise an exception, further finally-block cleanup logic will not execute. To work around this, the user would need to wrap every call to Dispose (within the finally block!) in a try block, which leads to very complex cleanup handlers.

A related question makes a stronger case about the downsides to throwing exceptions from finally blocks:

  1. A first exception is thrown
  2. A finally block is executed as a result of the first exception
  3. The finally block calls a Dispose() method
  4. The Dispose() method throws a second exception

[…] You lose information because .NET unceremoneously replaces the first exception with the second one. A catch block somewhere up the call stack will therefore never see the first exception. However, one is usually more interested in the first exception because that normally gives better clues as to why things started to go wrong.

This argument has merit – if the user's custom logic throws an exception, I would not want it to be hidden (and lost) by any exceptions thrown in the clean-up. There are solutions to this, such as Marc Gravell's wrapper for IDisposable objects (which swallows all Dispose exceptions), but this imparts more responsibility on the user. On the other hand, the .NET Framework Class Library itself seems to disregard this rule ‒ FileStream can throw exceptions from its Dispose method if it fails to flush the buffer's contents to disk.

Which of the two approaches (delegate-parameterized methods vs IDisposable) would be recommendable for these general-purpose implementations? Are there any implications apart from the ones mentioned above?

Update: This is an example of how delegate-parameterized methods can prevent main-logic exceptions from being swallowed:

public static void UsingTempFile(Action<string> action)
{
    // initialization
    string tempFilePath = Path.GetTempFileName();
    bool isSuccess = false;

    try
    {
        // main logic
        action(tempFilePath);
        isSuccess = true;
    }
    finally
    {
        try
        {
            // clean-up
            File.Delete(tempFilePath);
        }
        catch
        {
            // only propagate exceptions from clean-up if there were
            // no unhandled exceptions from the main logic
            if (isSuccess)
                throw;
        }
    }
}
Community
  • 1
  • 1
Douglas
  • 53,759
  • 13
  • 140
  • 188
  • 1
    *the .NET Framework Class Library itself seems to discard this rule ‒ FileStream can throw exceptions from its Dispose method if it fails to flush the buffer's contents to disk* I do think you gave yourself the response: if the `Dispose()` do other things other than disposing (for example flushing buffers), then it is ok to throw exceptions. – xanatos Feb 24 '16 at 13:03
  • 1
    *AVOID throwing an exception from within Dispose(bool)* says that don't throw exceptions. I infer that literally. I mean, you don't throw exception; that doesn't mean you shouldn't call any method that throws(simply it isn't possible). It is just a guideline, not a rule; Also there is no reason for you to throw `IOException` when temp file is in use, try to delete and framework will throw for you. Btw my vote is for `IDisposable` and I've used it the same way. Surprisingly my class name is also `TempFile`. – Sriram Sakthivel Feb 24 '16 at 13:05
  • @SriramSakthivel *I mean, you don't throw exception; that doesn't mean you shouldn't call any method that throws(simply it isn't possible)* This would be a very strange interpretation: it is like saying that Microsoft .NET classes are "noble" classes that can throw while your classes are "plebeian" classes that can't. I do prefer to read it more like a "shouldn't", like "you should always `Dispose()`, unless it is too much complex to do it" – xanatos Feb 24 '16 at 13:09
  • @xanatos I mean to say that you don't throw exception in Dispose method but if that happens somewhere inside the callstack, that's fine. – Sriram Sakthivel Feb 24 '16 at 13:11
  • @SriramSakthivel Yes, I read it exactly like that, and it is like saying "if `FileStream.Dispose()` throws an exception it is ok, because Microsoft wrote it, but `TempFile.Dispose()` mustn't generate new exceptions because you wrote it, and you aren't Microsoft. :-)" – xanatos Feb 24 '16 at 13:16
  • @xanatos no. Forget about msft. I mean, you(anyone) don't throw exception literally in Dispose method, but if that happens for some reason deep inside the callstack, that's fine. You can't do anything about that. Hope I'm being clear. – Sriram Sakthivel Feb 24 '16 at 13:31
  • @SriramSakthivel: I see your point, but I've run into cases where I wouldn't have agreed with it. For example, I implemented a pool that could cache and reuse modal form dialogs. In the “clean-up” logic, I needed to check that the caller had *closed* the form before returning it to the pool. This was an exception that needed to be thrown due to the nature of the clean-up, not due to any internal functionality it relied on. – Douglas Feb 24 '16 at 13:55
  • 1
    Handling problems that are detected in main-line code is often easier than handling problems that aren't detected until clean-up code is run. It is often helpful, for example, to have a cleanup method which throws an exception if anything unexpected happens, and a `Dispose` method which stifles most exceptions and does nothing if it is invoked after the former cleanup method. If that is done properly, the only cases where `Dispose` would throw an exception would be those where another exception was already pending. – supercat Feb 24 '16 at 21:21
  • @supercat: That's a popular suggestion (and the one put forward by the WCF team in [Avoiding Problems with the Using Statement](https://msdn.microsoft.com/en-us/library/aa355056(v=vs.110).aspx)). However, I don't agree with it. If I burden the user with the responsibility of calling a `Cleanup` or `Close` method, then the entire dispose pattern becomes useless, leading us to effectively ditch the `using` keyword (like the WCF team recommended). – Douglas Feb 24 '16 at 21:51
  • After attacking this for a day, I'm siding with the `IDisposable` option, but providing callers with a choice of strategy on how to propagate or swallow exceptions from `Dispose`; see [`DisposableExtensions.Using` extension method](http://stackoverflow.com/a/35613744/1149773). – Douglas Feb 24 '16 at 21:58
  • 1
    @Douglas: Semantic correctness requires that cleanup code know whether the guarded clause completed successfully. If Microsoft had defined `Dispose` to take a parameter which would indicate whether it was being run because of an exception, using a separate method for the "success" case wouldn't be necessary, but since MS didn't, I don't see any semantically-correct alternative. – supercat Feb 24 '16 at 22:18
  • @supercat: If you use a delegate-parameterized method, you could internally execute the specified delegate in a try-catch block, and thereby know whether the cleanup is being performed following success or an exception. I believe this meets the requirements for semantic correctness that you mentioned, no? – Douglas Feb 24 '16 at 22:49
  • Oh, the swan-song of IDisposable. You are getting battered by reminders that using it for anything else than what the *contract* states should happen is a very bad idea. Hard enough already to get programmers to use it when they write a program that takes less than a second. Not the mention the unwashed masses that never once used the *using* statement in their code and never noticed a problem. You have to write a manual to tell them that they *must* use it to make your code behave. When you have to write a manual anyway, you might as well do it right. – Hans Passant Feb 24 '16 at 23:34
  • Primarily opinion-based. There are too many outside factors that might affect which approach one might prefer. That said, assuming client code only ever use `using`, as intended, with your temp-file class, there won't ever be _"further finally-block cleanup logic"_ to not execute should an exception occur, so the MSDN guidance is moot. You can "safely" throw exceptions from `Dispose()` without harm (of the sort MSDN is concerned about, anyway). – Peter Duniho Feb 24 '16 at 23:51
  • 1
    @Douglas: The delegate-parameterized method is good in some contexts; unusable for others. Among other things, it requires objects to have strictly nested lifetimes. The `IDisposable` pattern is generally a good one, except for the inability of the cleanup code to know whether the guarded block succeeded. – supercat Feb 24 '16 at 23:54
  • @HansPassant: That was one of my original arguments against using `IDisposable` for these generic initialize–cleanup operations. However, I would consider `IDisposable` to be a sufficiently well-established pattern that its required usage (with the `Dispose` call) becomes self-evident for competent developers. This is similar to the expectation that developers consuming asynchronous methods call `await` on the returned task (not fire and forget). Even productivity tools such as ReSharper enforce these patterns. – Douglas Feb 25 '16 at 09:21
  • On the other hand, the requirement to call a custom `Cleanup` method is not self-evident; *that* is the case where I would need to write a manual for correct usage. This is notoriously the case for WCF clients, with its [Avoiding Problems with the Using Statement](https://msdn.microsoft.com/en-us/library/aa355056(v=vs.110).aspx). – Douglas Feb 25 '16 at 09:24
  • @supercat: I agree about the issue of strictly nested lifetimes; it's one I've faced in the past. Delegate-parameterized methods can be composable, but only by wrapping calls to them in *other* delegate-parameterized methods, exacerbating the four-overloads issue. `IDisposable` is indeed cleaner to pass around – you just return it from your method and relegate the responsibility of disposing it to your caller (like the `File.Open` and `XmlReader.Create` factory methods do). – Douglas Feb 25 '16 at 09:27
  • I find the question a helpful read for new programmers. However, a bit Too Broad for this format. – Drew Feb 25 '16 at 14:27
  • I've narrowed the question down to the specific scenario of temporary files. – Douglas Feb 25 '16 at 15:14
  • 1
    @Douglas: I really wish `using` and `finally` were extended to allow cleanup code to know what's going on, since the proper way to wrap things like transaction scopes would be to say that leaving an uncommitted scope because of an exception should roll it back, but leaving an uncommitted scope without rolling it back when no exception is pending should *throw an improper-usage exception*. Throwing an improper usage exception if the scope is left because of another exception, however, would be extremely rude. – supercat Feb 25 '16 at 16:23

0 Answers0