Is it abusive to use IDisposable and "using" as a means for getting "scoped behavior" for exception safety?

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?

Cincinnati

+1 for the class and method names alone :-). Well, to be fair, and the actual question. – Houchens

@Johannes: Yeah, I can see why - most people just frobble their fiddle, but I just must protest against that. ;-) And, by the way, +1 for your name similarity. – Cincinnati

Perhaps you can use the lock(this) { .. } scope to achieve the same result in this example? – Dolhenty

@oɔɯǝɹ: I assume you're pulling my leg. – Cincinnati

not really, perhaps i dont understand your scenario enough. On a side note, TransactionScope allows a using() { .. } block to be used for cleanup like this. Perhaps a transaction is considered a resource to be cleaned up for this to be considered allowed. – Dolhenty

@oɔɯǝɹ:You use lock() to prevent simultaneous access to something from different threads. Nothing to do with the scenario I describe. – Cincinnati

IScopeable with no Finalizer in the next version of C# :) – Scarlatina

Answer 1

I don't think so, necessarily. IDisposable technically is meant to be used for things that have non-managed resources, but then the using directive is just a neat way of implementing a common pattern of try .. finally { dispose }.

A purist would argue 'yes - it's abusive', and in the purist sense it is; but most of us do not code from a purist perspective, but from a semi-artistic one. Using the 'using' construct in this way is quite artistic indeed, in my opinion.

You should probably stick another interface on top of IDisposable to push it a bit further away, explaining to other developers why that interface implies IDisposable.

There are lots of other alternatives to doing this but, ultimately, I can't think of any that will be as neat as this, so go for it!

Dawes

I believe it is an artistic use of "using" as well. I think that Samual Jack's post linking to a comment from Eric Gunnerson validates this implementation of "using". I can see excellent points made from both Andras and Eric on this. – Sc

I talked to Eric Gunnerson about this a while back and according to him, it was the intention of the C# design team that using would denote scopes. In fact, he postulated that had they designed using before the lock statement, there might not even be a lock statement. It would have been a using block w/ a Monitor call or something. UPDATE: Just realized the next answer is by Eric Lippert who is also on the languages team. ;) Perhaps the C# team itself is not in full agreement about this? The context of my discussion w/ Gunnerson was the TimedLock class: bit.ly/lKugIO – Brunobruns

@Brunobruns - Amusing isn't it; your comment totally proves my point; that you'd spoken to one guy on the team and he was all for it; then pointing out Eric Lippert below, from the same team disagrees. From my point of view; C# provides a keyword that exploits an interface and also happens to yield a nice looking code pattern that works in so many scenarios. If we're not supposed to abuse it, then C# should find another way to enforce that. Either way, the designers probably need to get their ducks in a row here! In the meantime: using(Html.BeginForm()) sir? don't mind if I do sir! :) – Dawes

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.

Transgress

Although I would answer the question differently, I think that's a well-argued post. However, I take issue with your claim that using is a politeness rather than correctness issue. I believe that resource leaks are correctness issues, although often they are minor enough that they are not high-priority bugs. Thus, using blocks already have semantic impact on program state, and what they prevent is not just "inconvenience" to other processes but possibly a broken environment. That's not to say that the different type of semantic impact implied by the question is also appropriate. – Kopp

Resource leaks are correctness issues, yes. But failure to use a "using" does not leak the resource; the resource will be cleaned up eventually when the finalizer runs. The cleanup might not be as aggressive and timely as you'd like, but it will happen. – Transgress

@Eric: That's a really great answer, which puts words on and sums up all the small nagging feelings I mentioned the question. And more! – Cincinnati

Wonderful post, here's my two cents :) I suspect much "abuse" of using is because it's a poor man's aspect oriented weaver in C#. What the developer really wants is a way to guarantee that some common code will run at the end of a block without having to duplicate that code. C# doesn't support AOP today (MarshalByRefObject & PostSharp not withstanding). However, the compiler's transformation of the using statement makes it possible to achieve simple AOP by re-purposing the semantics of deterministic resource disposal. While not a good practice, it's sometimes attractive to pass up..... – Lindalindahl

While I generally agree with your position, there are some cases where the benefit of re-purposing using is too attractive to give up. The best example I can think of is to track and report code timings: using( TimingTrace.Start("MyMethod") ) { /* code */ } Again, this is AOP - Start() captures the start time before the block begins, Dispose() captures the end time and logs the activity. It doesn't change program state and is agnostic to exceptions. It's also attractive because it avoids quite a bit of plumbing and it's frequent use as a pattern mitigates the confusing semantics. – Lindalindahl

I agree very much with Eric. One way to summarise his answer would be: don't hide the magic! Be expicit. – Mail

Magic isn't hidden if comments are made regarding an unexpected use of "using". If the implementation is not for resource clean-up, then comment the usage. That said, I would not opt for an unknown program state, nor to voluntarily allow resource leakage, not matter how insignificant that leakage may appear. – Sc

Funny, I always thought of using as being a means of supporting RAII. It seems reasonably intuitive to me to consider a lock to be a type of resource. – Democracy

@Brian: A lock might be a resource. A mutex is an operating system handle, and those are a scarce global resource. But there is a big difference between a resource that should be released in a timely manner and a semantically important mutation that must happen at a particular time to maintain a logical program invariant. – Transgress

...and the battle continues!! #2725949 – Gratuitous

"I expect "using" to be used out of politeness, not necessity" - causing a Web application to exhaust the DB connection pool under load by failing to deterministically close connections is not just a lack of politeness. – Ombudsman

Answer 3

Eric Gunnerson, who was on the C# language design team, gave this answer to pretty much the same question:

Doug asks:

re: A lock statement with timeout...

I've done this trick before to deal with common patterns in numerous methods. Usually lock acquisition, but there are some others. Problem is it always feels like a hack since the object isn't really disposable so much as "call-back-at-the-end-of-a-scope-able".

Doug,

When we decided[sic] the using statement, we decided to name it “using” rather than something more specific to disposing objects so that it could be used for exactly this scenario.

Telegonus

Well that quote should say what he was referring to when he said "exactly this scenario", as I doubt he was referring to this future question. – Digression

@Frank Schwieterman : I completed the quote. Apparently, the people from the C# team did think the using feature was not to be limited to resource disposal. – Lens

Answer 4

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.

Mehala

In that example, you missed the entry state. Furthermore, sure you wrap try/finally, but you don't call anything in finally, so if something in the lambda throws you have no idea what state you're in. – Cincinnati

This was just an idea, I thought maybe you can adapt it to your needs. The method can have additional parameters, it could take an additional delegate for the finally block, you could use a fancier delegate than a simple Action too, etc etc... – Mehala

Ah! Ok, I guess you mean that SafeExecute is a Fribble method which calls Unlock() and Lock() in the wrapped try/finally. My apologies then. I immediately saw SafeExecute as a generic extension method and therefore mentioned the lack of entry and exit state. My bad. – Cincinnati

It could be modeled as an instance method or as a general utility extension method, just as it fits your needs. – Mehala

Be very careful with this apporach. There could be some dangerous subtle unintended lifetime extensions in the case of captured locals! – Gardiner

Yuk. Why hide try/catch/finally? Makes your code hide to read for others. – Digression

@Jason: The method could be myFribble.SafeExecute(Action<TDangerousFrobble> frobbleAction) instead. The DangerousFrobble instance could be private to the Fribble class, or DangerousFrobble could be declared as a private inner class of Fribble altogether. The DangerousFrobble instance then would be exposed to the caller in the scope of this one method alone. – Mehala

@Yukky Frank: It was not my idea to "hide" anything, it was the questioner's request. :-P ... That said, the request is at last a matter of "Don't Repeat Yourself". You might have a lot of methods that require the same boilerplate "frame" of cleanly acquiring/releasing something and you don't want to impose that on the caller (think of encapsulation). Also one could name methods like .SafeExecute(...) more clearly to communicate well enough what they do. – Mehala

@Mehala der welten: That doesn't escape the issue. The point is that with anonymous delegates or lambda expressions (just a very special case of anonymous delegates) outer variables are captured into an anonymous class that encapsulates the outer variables and the anonymous delegate. Depending on what happens with instances of that anonymous class the lifetime of the outer variables could be extended in unintended ways. – Gardiner

@Jason: As far as I remember, only the outer variables you actually use are captured. We could check that by reflector by switching such a piece of code from .NET 3.5 back to 2.0. Hence my proposal of not using the myFribble instance inside the anonymous delegate, but declaring a DangerousFrobble that encapsulates the dangerous method calls, so that the caller would have to do myFribble.SafeExecute(dangerousFrobble => dangerousFrobble.PushRedButton()); – Mehala

Answer 5

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.

Bechuana

The forced case was given by "herzmeister der welten" above, I think - even if the OP didn't seem to like the example. – Mirthless

Yes, I interpreted his SafeExecute as a generic extension method. I see now that it was likely meant as a Fribble method which calls Unlock() and Lock() in the wrapped try/finally. My bad. – Cincinnati

Ignoring an exception doesn't mean it is not recoverable. It means it will be handled above in the stack. For example, exceptions can be used to exit a thread gracefully. In this case, you must correctly free your resources, including locked locks. – Lens

Answer 6

A real world example is the BeginForm of ASP.net MVC. Basically you can write:

Html.BeginForm(...);
Html.TextBox(...);
Html.EndForm();

or

using(Html.BeginForm(...)){
    Html.TextBox(...);
}

Html.EndForm calls Dispose, and Dispose just outputs the </form> tag. The good thing about this is that the { } brackets create a visible "scope" which makes it easier to see what's within the form and what not.

I wouldn't overuse it, but essentially IDisposable is just a way to say "You HAVE to call this function when you're done with it". MvcForm uses it to make sure the form is closed, Stream uses it to make sure the stream is closed, you could use it to make sure the object is unlocked.

Personally I would only use it if the following two rules are true, but thet are set arbitarily by me:

• Dispose should be a function that always has to run, so there shouldn't be any conditions except Null-Checks
• After Dispose(), the object should not be re-usable. If I wanted a reusable object, I'd rather give it open/close methods rather than dispose. So I throw an InvalidOperationException when trying to use a disposed object.

At the end, it's all about expectations. If an object implements IDisposable, I assume it needs to do some cleanup, so I call it. I think it usually beats having a "Shutdown" function.

That being said, I don't like this line:

this.Frobble.Fiddle();

As the FrobbleJanitor "owns" the Frobble now, I wonder if it wouldn't be better to instead call Fiddle on he Frobble in the Janitor?

Barm

About your "I don't like this line" - I actually briefly thought of doing it that way when formulating the question, but I didn't want to clutter the semantics of my question. But I kind of agree with you. Kind of. :) – Cincinnati

Upvoted for providing an example from Microsoft itself. Note that there is a specific exception to be thrown in the case you mention: ObjectDisposedException. – Waft

Answer 7

We have plenty of use of this pattern in our code base, and I've seen it all over the place before - I'm sure it must have been discussed here as well. In general I don't see what's wrong with doing this, it provides a useful pattern and causes no real harm.

Shepley

Answer 8

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.

Mirthless

The "Janitor" comes from an old C++ article by Andrei Alexandrescu about scope guards, even before ScopeGuard made its way to Loki. – Cincinnati

@Benjamin: I like the TransactionScope class, hadn't heard of it before. Maybe because I'm in .Net CF land, where it's not included... :-( – Cincinnati

Answer 9

Note: My viewpoint is probably biased from my C++ background, so my answer's value should be evaluated against that possible bias...

What Says the C# Language Specification?

Quoting C# Language Specification:

8.13 The using statement

[...]

A resource is a class or struct that implements System.IDisposable, which includes a single parameterless method named Dispose. Code that is using a resource can call Dispose to indicate that the resource is no longer needed. If Dispose is not called, then automatic disposal eventually occurs as a consequence of garbage collection.

The Code that is using a resource is, of course, the code starting by the using keyword and going until the scope attached to the using.

So I guess this is Ok because the Lock is a resource.

Perhaps the keyword using was badly chosen. Perhaps it should have been called scoped.

Then, we can consider almost anything as a resource. A file handle. A network connection... A thread?

A thread???

Using (or abusing) the using keyword?

Would it be shiny to (ab)use the using keyword to make sure the thread's work is ended before exiting the scope?

Herb Sutter seems to think it's shiny, as he offers an interesting use of the IDispose pattern to wait for a thread's work to end:

http://www.drdobbs.com/go-parallel/article/showArticle.jhtml?articleID=225700095

Here is the code, copy-pasted from the article:

// C# example
using( Active a = new Active() ) {    // creates private thread
       …
       a.SomeWork();                  // enqueues work
       …
       a.MoreWork();                   // enqueues work
       …
} // waits for work to complete and joins with private thread

While the C# code for the Active object is not provided, it is written the C# uses the IDispose pattern for the code whose C++ version is contained in the destructor. And by looking at the C++ version, we see a destructor which waits for the internal thread to end before exiting, as shown in this other extract of the article:

~Active() {
    // etc.
    thd->join();
 }

So, as far as Herb is concerned, it's shiny.

Lens

Answer 10

I believe the answer to your question is no, this would not be an abuse of IDisposable.

The way I understand the IDisposable interface is that you are not supposed to work with an object once it has been disposed (except that you're allowed to call its Dispose method as often as you want).

Since you create a new FrobbleJanitor explicitly each time you get to the using statement, you never use the same FrobbeJanitor object twice. And since it's purpose is to manage another object, Dispose seems appropriate to the task of freeing this ("managed") resource.

(Btw., the standard sample code demonstrating the proper implementation of Dispose almost always suggests that managed resources should be freed, too, not just unmanaged resources such as file system handles.)

The only thing where I'd personally worry is that it's less clear what's happening with using (var janitor = new FrobbleJanitor()) than with a more explicit try..finally block where the Lock & Unlock operations are directly visible. But which approach is taken probably comes down to a matter of personal preferences.

Edlyn

Answer 11

It's 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 example, if you are opting for 'artistry', then you can use 'using', but if your piece of code is executed a lot of times, then for performance reasons, you might use the 'try'..'finally' constructs, because 'using' usually involves creations of an object.

Diantha

Answer 12

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.

Digression

About renaming - I used "Janitor" of the primary reason that I dealt with Lock/Unlock. I thought it rhymed with the other name choices. ;-) If I were to use this method as a pattern throughout my code base, I would definitely include something more generically descriptive, as you implied with "Session". If this had been the old C++ days, I'd probably use FrobbleUnlockScope. – Cincinnati