CODEWITHSUNDEEP
c#
noseratio
noseratio

Reputation: 61744

Dealing with nested "using" statements in C#

I've noticed that the level of nested using statements has lately increased in my code. The reason is probably because I use more and more of async/await pattern, which often adds at least one more using for CancellationTokenSource or CancellationTokenRegistration.

So, how to reduce the nesting of using, so the code doesn't look like Christmas tree? Similar questions have been asked on SO before, and I'd like to sum up what I've learnt from the answers.

Use adjacent using without indentation. A fake example:

using (var a = new FileStream())
using (var b = new MemoryStream())
using (var c = new CancellationTokenSource())
{
    // ... 
}

This may work, but often there's some code between using (e.g. it may be too early to create another object):

// ... 
using (var a = new FileStream())
{
    // ... 
    using (var b = new MemoryStream())
    {
        // ... 
        using (var c = new CancellationTokenSource())
        {
            // ... 
        }
    }
}

Combine objects of the same type (or cast to IDisposable) into single using, e.g.:

// ... 
FileStream a = null;
MemoryStream b = null;
CancellationTokenSource c = null;
// ...
using (IDisposable a1 = (a = new FileStream()), 
    b1 = (b = new MemoryStream()), 
    c1 = (c = new CancellationTokenSource()))
{
    // ... 
}

This has the same limitation as above, plus is more wordy and less readable, IMO.

Refactor the method into a few methods.

This is a preferred way, as far as I understand. Yet, I'm curious, why would the following be considered a bad practice?

public class DisposableList : List<IDisposable>, IDisposable
{
    public void Dispose()
    {
        base.ForEach((a) => a.Dispose());
        base.Clear();
    }
}

// ...

using (var disposables = new DisposableList())
{
    var a = new FileStream();
    disposables.Add(a);
    // ...
    var b = new MemoryStream();
    disposables.Add(b);
    // ...
    var c = new CancellationTokenSource();
    disposables.Add(c);
    // ... 
}

[UPDATE] There are quite a few valid points in the comments that nesting using statements makes sure Dispose will get called on each object, even if some inner Dispose calls throw. However, there is a somewhat obscure issue: all nested exceptions possibly thrown by disposing of nested 'using' frames will be lost, besides the most outer one. More on this here.

Upvotes: 30

Views: 11533

Answers (5)

Mike Zboray
Mike Zboray

Reputation: 40838

In a single method, the first option would be my choice. However in some circumstances the DisposableList is useful. Particularly, if you have many disposable fields that all need to be disposed of (in which case you cannot use using). The implementation given is good start but it has a few problems (pointed out in comments by Alexei):

  1. Requires you to remember to add the item to the list. (Although you could also say you have to remember to use using.)
  2. Aborts the disposal process if one of the dispose methods throws, leaving the remaining items un-disposed.

Let's fix those problems: 

public class DisposableList : List<IDisposable>, IDisposable
{
    public void Dispose()
    {
        if (this.Count > 0)
        {
            List<Exception> exceptions = new List<Exception>();

            foreach(var disposable in this)
            {
                try
                {
                    disposable.Dispose();
                }
                catch (Exception e)
                {
                    exceptions.Add(e);
                }
            }
            base.Clear();

            if (exceptions.Count > 0)
                throw new AggregateException(exceptions);
        }
    }

    public T Add<T>(Func<T> factory) where T : IDisposable
    {
        var item = factory();
        base.Add(item);
        return item;
    }
}

Now we catch any exceptions from the Dispose calls and will throw a new AggregateException after going through all the items. I've added a helper Add method that allows a simpler usage:

using (var disposables = new DisposableList())
{
    var file = disposables.Add(() => File.Create("test"));
    // ...
    var memory = disposables.Add(() => new MemoryStream());
    // ...
    var cts = disposables.Add(() => new CancellationTokenSource());
    // ... 
}

Upvotes: 16

Bal&#225;zs Sz&#225;nt&#243;
Bal&#225;zs Sz&#225;nt&#243;

Reputation: 1500

I would stick to the using blocks. Why?

  • It clearly shows your intentions with these objects
  • You don't have to mess around with try-finally blocks. It's error prone and your code gets less readable.
  • You can refactor embedded using statements later (extract them to methods)
  • You don't confuse your fellow programmers including a new layer of abstractions by creating your own logic

Upvotes: 6

codemonkeh
codemonkeh

Reputation: 2152

Another option is to simply use a try-finally block. This might seem a bit verbose, but it does cut down unnecessary nesting.

FileStream a = null;
MemoryStream b = null;
CancellationTokenSource c = null;

try
{
   a = new FileStream();
   // ... 
   b = new MemoryStream();
   // ... 
   c = new CancellationTokenSource();
}
finally 
{
   if (a != null) a.Dispose();
   if (b != null) b.Dispose();
   if (c != null) c.Dispose();
}

Upvotes: 1

user2674389
user2674389

Reputation: 1143

You should always refer to your fake example. When this is not possible, like you mentioned, then it is very likely that you can refactor the inner content into a separate method. If this also does not make sense you should just stick to your second example. Everything else just seems like less readable, less obvious and also less common code.

Upvotes: 6

alzaimar
alzaimar

Reputation: 4622

Your last suggestion hides the fact that a, b and c should be disposed explicitly. That`s why it's ugly.

As mentioned in my comment, if you'd use clean code principles you wouldn't run into these problems (usually).

Upvotes: 1

Related Questions