23

Recently I've seen some code written as follows:

public void Dipose()
{
   using(_myDisposableField) { }
}

This seems pretty strange to me, I'd prefer to see myDisposableField.Dispose();

What reasons are there for using "using" to dispose your objects over explicitly making the call?

Dave Hillier
  • 18,105
  • 9
  • 43
  • 87

4 Answers4

24

No, none at all. It will just compile into an empty try/finally and end up calling Dispose.

Remove it. You'll make the code faster, more readable, and perhaps most importantly (as you continue reading below) more expressive in its intent.

Update: they were being slightly clever, equivalent code needs a null check and as per Jon Skeet's advice, also take a local copy if multi-threading is involved (in the same manner as the standard event invocation pattern to avoid a race between the null check and method call).

IDisposable tmp = _myDisposableField; 

if (tmp != null) 
    tmp.Dispose();

From what I can see in the IL of a sample app I've written, it looks like you also need to treat _myDisposableField as IDisposable directly. This will be important if any type implements the IDisposable interface explicitly and also provides a public void Dispose() method at the same time.

This code also doesn't attempt to replicate the try-finally that exists when using using, but it is sort of assumed that this is deemed unnecessary. As Michael Graczyk points out in the comments, however, the use of the finally offers protection against exceptions, in particular the ThreadAbortException (which could occur at any point). That said, the window for this to actually happen in is very small.

Although, I'd stake a fiver on the fact they did this not truly understanding what subtle "benefits" it gave them.

Adam Houldsworth
  • 63,413
  • 11
  • 150
  • 187
  • Strictly speaking, even that's not quite the same - what if another thread changes the value of `_myDisposableField` after the nullity check? You'd need to extract a local variable first... – Jon Skeet Aug 02 '12 at 13:50
  • 5
    @JonSkeet True. But then again, in both of these cases the commonality is to slap the developer responsible for doing this without comment. – Adam Houldsworth Aug 02 '12 at 13:51
  • 3
    I'm not sure how to parse that. Do you {slap them for doing this} without comment, or do you slap them for {for doing this without comment}? :) – Jon Skeet Aug 02 '12 at 13:53
  • @JonSkeet I'd say "{for doing this without comment} without comment" :) – Tim S. Aug 02 '12 at 13:53
  • 2
    @Jon Both, they deserve two slaps. Forehand and backhand :-P – Adam Houldsworth Aug 02 '12 at 13:53
  • Yes, it was used on single threaded code and seems to be one of the many quirks of the code base. – Dave Hillier Aug 02 '12 at 14:00
  • @DaveHillier Then although not technically equivalent, you can drop the `tmp` variable for semantically equivalent code in your case. – Adam Houldsworth Aug 02 '12 at 14:01
  • Perhaps then `using (var tmp = _myDisposableField) {}` would be the most/least clever approach? – user7116 Aug 02 '12 at 14:04
  • 1
    @JonSkeet Turns out treating the variable as `IDisposable` is also important due to differences in implicit vs explicit interface definitions. – Adam Houldsworth Aug 02 '12 at 14:13
  • 1
    @AdamHouldsworth There is one other important difference. The OP's posted code will run `_myDisposableField.IDisposable.Dispose()` in a *protected section* (inside a finally block). This means that it will not be interrupted by a `ThreadAbortException`, among other things. Seems like this "clever" code is looking more **clever** by the minute. – Michael Graczyk Aug 03 '12 at 07:30
  • @MichaelGraczyk I don't think so personally, if it were that clever I'd expect to have seen it touted in an article somewhere. Also the OP specified that this isn't a multi-threaded app. I think it'd be more clever to wrap those protected sections yourself in order to make it perfectly understandable what is going on with it. Plus the window for that exception to cause any great behavioural difference is very small, the try-finally lives and dies within that one method call. – Adam Houldsworth Aug 03 '12 at 07:37
  • @AdamHouldsworth I agree that it is not a huge deal in most situations, but it is interesting to see yet another subtle difference between this and simply `_myDisposableField.Dispose();`. – Michael Graczyk Aug 03 '12 at 07:56
  • @MichaelGraczyk Indeed, and another good catch - I've amended my answer to provide the information. – Adam Houldsworth Aug 03 '12 at 07:57
2

There is a very subtle but evil bug in the example you posted.

While it "compiles" down to:

try {}
finally
{
    if (_myDisposableField != null) 
        ((IDisposable)_myDisposableField).Dispose();
}

objects should be instantiated within the using clause and not outside:

You can instantiate the resource object and then pass the variable to the using statement, but this isn't a best practice. In this case, after control leaves the using block, the object remains in scope but probably has no access to its unmanaged resources. In other words, it's not fully initialized anymore. If you try to use the object outside the using block, you risk causing an exception to be thrown. For this reason, it's better to instantiate the object in the using statement and limit its scope to the using block.

using statement (C# Reference)

In other words, it's dirty and hacky.

The clean version is extremely clearly spelled out on MSDN:

  • if you can limit the use of an instance to a method, then use a using block with the constructor call on its border. Do not use Dispose directly.
  • if you need (but really need) to keep an instance alive until the parent is disposed, then dispose explicitly using the Disposable pattern and nothing else. There are different ways of implementing a dispose cascade, however they need to be all done similarly to avoid very subtle and hard to catch bugs. There's a very good resource on MSDN in the Framework Design Guidelines.

Finally, please note the following you should only use the IDisposable pattern if you use unmanaged resources. Make sure it's really needed :-)

Pang
  • 9,564
  • 146
  • 81
  • 122
Sklivvz
  • 30,601
  • 24
  • 116
  • 172
2

As already discussed in this answer, this is a cheeky way of avoiding a null test, but: there can be more to it than that. In modern C#, in many cases you could achieve similar with a null-conditional operator:

public void Dipose()
    => _myDisposableField?.Dispose();

However, it is not required that the type (of _myDisposableField) has Dispose() on the public API; it could be:

public class Foo : IDisposable {
    void IDisposable.Dispose() {...}
}

or even worse:

public class Bar : IDisposable {
    void IDisposable.Dispose() {...}
    public void Dispose() {...} // some completely different meaning! DO NOT DO THIS!
}

In the first case, Dispose() will fail to find the method, and in the second case, Dispose() will invoke the wrong method. In either of these cases, the using trick will work, as will a cast (although this will do slightly different things again if it is a value-type):

public void Dipose()
    => ((IDisposable)_myDisposableField)?.Dispose();

If you aren't sure whether the type is disposable (which happens in some polymorphism scenarios), you could also use either:

public void Dipose()
    => (_myDisposableField as IDisposable)?.Dispose();

or:

public void Dipose()
{
    using (_myDisposableField as IDisposable) {}
}
Marc Gravell
  • 1,026,079
  • 266
  • 2,566
  • 2,900
  • I'd argue that when you have an explicitly implemented interface and another method of the same name, then it doesn't matter what you do, it's calling the wrong thing. – Dave Hillier Sep 11 '20 at 08:20
  • @DaveHillier for added fun, you could have `A : IDisposable { public void Dispose(); void IDisposable.Dispose(); }` and `B : A, IDisposable { public new void Dispose(); void IDisposable.Dispose(); }` :) – Marc Gravell Sep 11 '20 at 12:28
-2

The using statement defines the span of code after which the referenced object should be disposed.

Yes, you could just call a .dispose once it was done with but it would be less clear (IMHO) what the scope of the object was. YMMV.

Robbie Dee
  • 1,939
  • 16
  • 43
  • I didn't downvote but my guess would be because your answer is about the using statement in general and not this specific situation where the OP is trying to replace an empty using with equivalent code. – Adam Houldsworth Aug 03 '12 at 07:16
  • The last line reads (and I quote): 'What reasons are there for using "using" to dispose your objects over explicitly making the call?'. Whoever downvoted clearly didn't read the question! – Robbie Dee Aug 03 '12 at 08:15
  • 1
    Yes but the scope plays no part in the reason for using a `using` here. The scope is a class member, the use of `using` doesn't make anything any clearer. Calling `Dispose` would actually make it clearer as `using` tends to be used for stuff that is declared, used and disposed in the same scope. – Adam Houldsworth Aug 03 '12 at 08:17