Is it abusive to use IDisposable and “using” as a

Question

Something I often used back in C++ was letting a class A handle a state entry and exit condition for another class B, via the A constructor and destructor, to make sure that if something in that scope threw an exception, then B would have a known state when the scope was exited. This isn't pure RAII as far as the acronym goes, but it's an established pattern nevertheless.

In C#, I often want to do

class FrobbleManager
{
    ...

    private void FiddleTheFrobble()
    {
        this.Frobble.Unlock();
        Foo();                  // Can throw
        this.Frobble.Fiddle();  // Can throw
        Bar();                  // Can throw
        this.Frobble.Lock();
    }
}

Which needs to be done like this

private void FiddleTheFrobble()
{
    this.Frobble.Unlock();

    try
    {            
        Foo();                  // Can throw
        this.Frobble.Fiddle();  // Can throw
        Bar();                  // Can throw
    }
    finally
    {
        this.Frobble.Lock();
    }
}

if I want to guarantee the Frobble state when FiddleTheFrobble returns. The code would be nicer with

private void FiddleTheFrobble()
{
    using (var janitor = new FrobbleJanitor(this.Frobble))
    {            
        Foo();                  // Can throw
        this.Frobble.Fiddle();  // Can throw
        Bar();                  // Can throw
    }
}

where FrobbleJanitor looks roughly like

class FrobbleJanitor : IDisposable
{
    private Frobble frobble;

    public FrobbleJanitor(Frobble frobble)
    {
        this.frobble = frobble;
        this.frobble.Unlock();
    }

    public void Dispose()
    {
        this.frobble.Lock();
    }
}

And that's how I want to do it. Now reality catches up, since what I want to use requires that the FrobbleJanitor is used with using. I could consider this a code review issue, but something is nagging me.

Question: Would the above be considered as abusive use of using and IDisposable?

Answer 1

If you just want some clean, scoped code, you might also use lambdas, á la

myFribble.SafeExecute(() =>
    {
        myFribble.DangerDanger();
        myFribble.LiveOnTheEdge();
    });

where the .SafeExecute(Action fribbleAction) method wraps the try - catch - finally block.

Answer 2

I consider this to be an abuse of the using statement. I am aware that I'm in the minority on this position.

I consider this to be an abuse for three reasons.

First, because I expect that "using" is used to use a resource and dispose of it when you're done with it. Changing program state is not using a resource and changing it back is not disposing anything. Therefore, "using" to mutate and restore state is an abuse; the code is misleading to the casual reader.

Second, because I expect "using" to be used out of politeness, not necessity. The reason you use "using" to dispose of a file when you're done with it is not because it is necessary to do so, but because it is polite -- someone else might be waiting to use that file, so saying "done now" is the morally correct thing to do. I expect that I should be able to refactor a "using" so that the used resource is held onto for longer, and disposed of later, and that the only impact of doing so is to slightly inconvenience other processes. A "using" block which has semantic impact on program state is abusive because it hides an important, required mutation of program state in a construct that looks like it is there for convenience and politeness, not necessity.

And third, your program's actions are determined by its state; the need for careful manipulation of state is precisely why we're having this conversation in the first place. Let's consider how we might analyze your original program.

Were you to bring this to a code review in my office, the first question I would ask is "is it really correct to lock the frobble if an exception is thrown?" It is blatantly obvious from your program that this thing aggressively re-locks the frobble no matter what happens. Is that right? An exception has been thrown. The program is in an unknown state. We do not know whether Foo, Fiddle or Bar threw, why they threw, or what mutations they performed to other state that were not cleaned up. Can you convince me that in that terrible situation it is always the right thing to do to re-lock?

Maybe it is, maybe it isn't. My point is, that with the code as it was originally written, the code reviewer knows to ask the question. With the code that uses "using", I don't know to ask the question; I assume that the "using" block allocates a resource, uses it for a bit, and politely disposes of it when it is done, not that the closing brace of the "using" block mutates my program state in an exceptional cirumstance when arbitrarily many program state consistency conditions have been violated.

Use of the "using" block to have a semantic effect makes this program fragment:

}

extremely meaningful. When I look at that single close brace I do not immediately think "that brace has side effects which have far-reaching impacts on the global state of my program". But when you abuse "using" like this, suddenly it does.

The second thing I would ask if I saw your original code is "what happens if an exception is thrown after the Unlock but before the try is entered?" If you're running a non-optimized assembly, the compiler might have inserted a no-op instruction before the try, and it is possible for a thread abort exception to happen on the no-op. This is rare, but it does happen in real life, particularly on web servers. In that case, the unlock happens but the lock never happens, because the exception was thrown before the try. It is entirely possible that this code is vulnerable to this problem, and should actually be written

bool needsLock = false;
try
{
    // must be carefully written so that needsLock is set
    // if and only if the unlock happened:

    this.Frobble.AtomicUnlock(ref needsLock);
    blah blah blah
}
finally
{
    if (needsLock) this.Frobble.Lock();
}

Again, maybe it does, maybe it doesn't, but I know to ask the question. With the "using" version, it is susceptible to the same problem: a thread abort exception could be thrown after the Frobble is locked but before the try-protected region associated with the using is entered. But with the "using" version, I assume that this is a "so what?" situation. It's unfortunate if that happens, but I assume that the "using" is only there to be polite, not to mutate vitally important program state. I assume that if some terrible thread abort exception happens at exactly the wrong time then, oh well, the garbage collector will clean up that resource eventually by running the finalizer.

Answer 3

It is a slippery slope. IDisposable has a contract, one that's backed-up by a finalizer. A finalizer is useless in your case. You cannot force the client to use the using statement, only encourage him to do so. You can force it with a method like this:

void UseMeUnlocked(Action callback) {
  Unlock();
  try {
    callback();
  }
  finally {
    Lock();
  }
}

But that tends to get a bit awkward without lamdas. That said, I've used IDisposable like you did.

There is however a detail in your post that makes this dangerously close to an anti-pattern. You mentioned that those methods can throw an exception. This is not something the caller can ignore. He can do three things about that:

• Do nothing, the exception isn't recoverable. The normal case. Calling Unlock doesn't matter.
• Catch and handle the exception
• Restore state in his code and let the exception pass up the call chain.

The latter two requires the caller to explicitly write a try block. Now the using statement gets in the way. It may well induce a client into a coma that makes him believe that your class is taking care of state and no additional work needs to be done. That's almost never accurate.

Answer 4

Its not abusive. You are using them what they are created for. But you might have to consider one upon another according to your needs. For e.g if you are opting for 'artistry' then you can use 'using' but if your piece of code is executed alot of times, then for performance reasons you might use the 'try'..'finally' constructs. Because 'using' usually involves creations of an object.

Answer 5

Chiming in on this: I agree with most in here that this is fragile, but useful. I'd like to point you to the System.Transaction.TransactionScope class, that does something like you want to do.

Generally I like the syntax, it removes a lot of clutter from the real meat. Please consider giving the helper class a good name though - maybe ...Scope, like the example above. The name should give away that it encapsulates a piece of code. *Scope, *Block or something similar should do it.

Answer 6

I think you did it right. Overloading Dispose() would be a problem the same class later had cleanup it actually had to do, and the lifetime of that cleanup changed to be different then the time when you expect to hold a lock. But since you created a separate class (FrobbleJanitor) that is responsible only for locking and unlocking the Frobble, things are decoupled enough you won't hit that issue.

I would rename FrobbleJanitor though, probably to something like FrobbleLockSession.