107

C# lets me do the following (example from MSDN):

using (Font font3 = new Font("Arial", 10.0f),
            font4 = new Font("Arial", 10.0f))
{
    // Use font3 and font4.
}

What happens if font4 = new Font throws? From what I understand font3 will leak resources and won't be disposed of.

  • Is this true? (font4 won't be disposed of)
  • Does this mean using(... , ...) should be avoided altogether in favor of nested using?
everton
  • 7,579
  • 2
  • 29
  • 42
Benjamin Gruenbaum
  • 270,886
  • 87
  • 504
  • 504
  • 7
    It won't _leak_ memory; in the worst case, it will still get GC'd. – SLaks Jan 14 '14 at 16:10
  • @SLaks so the underlying resource will always get disposed? `font3.Dispose()` will surely be called? – Benjamin Gruenbaum Jan 14 '14 at 16:11
  • 3
    I wouldn't be surprised if `using(... , ...)` is compiled into nested using blocks regardless, but I don't know that for sure. – Dan J Jan 14 '14 at 16:11
  • 1
    That's not what I meant. Even if you don't use `using` at all, the GC will still eventually collect it. – SLaks Jan 14 '14 at 16:11
  • The documentation precisely suggests that use case. What makes you suspect that Dispose() wouldn't be called on the first object if constructing the second throws? – zneak Jan 14 '14 at 16:15
  • 1
    @zneak: Had it compiled to a single `finally` block, it wouldn't have entered the block until all of the resources were constructed. – SLaks Jan 14 '14 at 16:16
  • 2
    @zneak: Because in the conversion of a `using` to a `try`-`finally`, the initialization expression is evaluated outside the `try`. So it is a reasonable question. – Ben Voigt Jan 14 '14 at 16:16
  • 1
    @SLaks, of course there's a way to screw this up, but there are ways to screw up almost anything. If it's [documented as a best practice](http://msdn.microsoft.com/en-us/library/yh598w02.aspx#CodeSnippetContainerCode_13090b41-9af1-4e17-b74c-0ef451748f02), then it ought to work. – zneak Jan 14 '14 at 16:20
  • Just for curiosity: `font3` and `font4` has the same values just for example purposes, right? – everton Jan 15 '14 at 14:50
  • 1
    @EvertonAgner Yes, of course. In practice they're not even fonts - in practice this is not an issue I'm really even facing - I always used `using` with one resource. The _real_ rationale is explained here: http://stackoverflow.com/questions/21118201/can-using-with-more-than-one-resource-cause-a-resource-leak?noredirect=1#comment31797113_21123756 – Benjamin Gruenbaum Jan 15 '14 at 15:41

5 Answers5

159

No.

The compiler will generate a separate finally block for each variable.

The spec (§8.13) says:

When a resource-acquisition takes the form of a local-variable-declaration, it is possible to acquire multiple resources of a given type. A using statement of the form

using (ResourceType r1 = e1, r2 = e2, ..., rN = eN) statement 

is precisely equivalent to a sequence of nested using statements:

using (ResourceType r1 = e1)
   using (ResourceType r2 = e2)
      ...
         using (ResourceType rN = eN)
            statement
SLaks
  • 868,454
  • 176
  • 1,908
  • 1,964
68

UPDATE: I used this question as the basis for an article which can be found here; see it for additional discussion of this issue. Thanks for the good question!


Though Schabse's answer is of course correct and answers the question that was asked, there is an important variant on your question you did not ask:

What happens if font4 = new Font() throws after the unmanaged resource was allocated by the constructor but before the ctor returns and fills in font4 with the reference?

Let me make that a little bit more clear. Suppose we have:

public sealed class Foo : IDisposable
{
    private int handle = 0;
    private bool disposed = false;
    public Foo()
    {
        Blah1();
        int x = AllocateResource();
        Blah2();
        this.handle = x;
        Blah3();
    }
    ~Foo()
    {
        Dispose(false);
    }
    public void Dispose() 
    { 
        Dispose(true); 
        GC.SuppressFinalize(this);
    }
    private void Dispose(bool disposing)
    {
        if (!this.disposed)
        {
            if (this.handle != 0) 
                DeallocateResource(this.handle);
            this.handle = 0;
            this.disposed = true;
        }
    }
}

Now we have

using(Foo foo = new Foo())
    Whatever(foo);

This is the same as

{
    Foo foo = new Foo();
    try
    {
        Whatever(foo);
    }
    finally
    {
        IDisposable d = foo as IDisposable;
        if (d != null) 
            d.Dispose();
    }
}

OK. Suppose Whatever throws. Then the finally block runs and the resource is deallocated. No problem.

Suppose Blah1() throws. Then the throw happens before the resource is allocated. The object has been allocated but the ctor never returns, so foo is never filled in. We never entered the try so we never enter the finally either. The object reference has been orphaned. Eventually the GC will discover that and put it on the finalizer queue. handle is still zero, so the finalizer does nothing. Notice that the finalizer is required to be robust in the face of an object that is being finalized whose constructor never completed. You are required to write finalizers that are this strong. This is yet another reason why you should leave writing finalizers to experts and not try to do it yourself.

Suppose Blah3() throws. The throw happens after the resource is allocated. But again, foo is never filled in, we never enter the finally, and the object is cleaned up by the finalizer thread. This time the handle is non-zero, and the finalizer cleans it up. Again, the finalizer is running on an object whose constructor never succeeded, but the finalizer runs anyways. Obviously it must because this time, it had work to do.

Now suppose Blah2() throws. The throw happens after the resource is allocated but before handle is filled in! Again, the finalizer will run but now handle is still zero and we leak the handle!

You need to write extremely clever code in order to prevent this leak from happening. Now, in the case of your Font resource, who the heck cares? We leak a font handle, big deal. But if you absolutely positively require that every unmanaged resource be cleaned up no matter what the timing of exceptions is then you have a very difficult problem on your hands.

The CLR has to solve this problem with locks. Since C# 4, locks that use the lock statement have been implemented like this:

bool lockEntered = false;
object lockObject = whatever;
try
{
    Monitor.Enter(lockObject, ref lockEntered);
    lock body here
}
finally
{
    if (lockEntered) Monitor.Exit(lockObject);
}

Enter has been very carefully written so that no matter what exceptions are thrown, lockEntered is set to true if and only if the lock was actually taken. If you have similar requirements then what you need to to is actually write:

    public Foo()
    {
        Blah1();
        AllocateResource(ref handle);
        Blah2();
        Blah3();
    }

and write AllocateResource cleverly like Monitor.Enter so that no matter what happens inside AllocateResource, the handle is filled in if and only if it needs to be deallocated.

Describing the techniques for doing so is beyond the scope of this answer. Consult an expert if you have this requirement.

Community
  • 1
  • 1
Eric Lippert
  • 647,829
  • 179
  • 1,238
  • 2,067
  • 1
    where is _Schabse's answer_ you refer to? – gnat Jan 14 '14 at 21:13
  • 6
    @gnat: The accepted answer. That S has to stand for something. :-) – Eric Lippert Jan 14 '14 at 21:19
  • 3
    *We leak a font handle, big deal* ... I find that if you leak 1 font handle, your code is such that it will leak font handles rather often... to the point where it crashes horribly. Seen it happen :( – gbjbaanb Jan 14 '14 at 22:16
  • 4
    @gbjbaanb: A semi-quote from somewhere: "In software, anything with a one in a million chance happens about once a second." – Zan Lynx Jan 14 '14 at 22:43
  • 4
    Some background, we're working on [`using` for JavaScript promises](https://github.com/petkaantonov/bluebird/issues/65). We had the exact same discussion (constructor allocates resource then throws) yesterday and we've decided to _explicitly_ require that constructors (our analogue is functions returning a promise on a resource (like Tasks if you're unfamiliar with the term)) have a no throw guarantee (no destructors in JS). We're having quite a hard time :), think `using(var resources = await ObtainUsingTaskWhenAll(t1,t2,t3,t4)){` – Benjamin Gruenbaum Jan 15 '14 at 06:02
  • 3
    @BenjaminGruenbaum: Sounds like an interesting project! And yes, I am familiar with promises. – Eric Lippert Jan 15 '14 at 06:11
  • @EricLippert and thanks for the answer :) I feel slightly less crazy now about that other problem. – Benjamin Gruenbaum Jan 15 '14 at 06:23
  • 1
    I think the risks are exaggerated, and the example contrived. And I don't see what you understand by the "extremely clever code" that can only be written by experts and is beyond the scope of this answer. One would normally set the handle field or equivalent directly with the result of a call to an unmanaged API. – Joe Jan 15 '14 at 11:51
  • 1
    I'm finding myself agreeing with Joe here. Wouldn't `this.handle = AllocateResource();` and not running unrelated things between getting the handle and setting it just fix the problem? I may be (and am sure I am) missing a more fundamental part of the problem but this particular example seems at a glance trivial to fix which means if there is more to the problem you are hiding it behind an obvious bug. – Chris Jan 15 '14 at 12:17
  • @Chris: What if `AllocateResources()` throws between allocation & return? – SLaks Jan 15 '14 at 12:32
  • @SLaks: yeah, my point was more on the triviality of the example than anything else. I'm sure what you say is right but my immediate reaction is again don't put things between allocation and return but I suppose at some point somewhere some work might need to be done on a resource after it is created and before it is given away. An example of where you really need to do some work between would make me much more able to see the point. – Chris Jan 15 '14 at 12:48
  • @Slaks, in fact the best practice IMHO would be to always wrap an unmanaged resource in an wrapper class that does nothing but implement IDisposable and a finalizer. The AllocateResource() method is typically a P/Invoke call, so no real scope for a leak. And consumers of the wrapper class only deal with managed IDisposable resources, so don't need to worry about a finalizer. – Joe Jan 15 '14 at 13:49
  • 13
    @Joe: Of course the example is *contrived*. **I just contrived it**. The risks are not *exaggerated* because I haven't stated what the *level* of risk is; rather, I've stated that this pattern is *possible*. The fact that you believe that setting the field directly solves the problem indicates precisely my point: that like the vast majority of programmers who have no experience with this kind of problem, you're not competent to solve this problem; indeed, most people don't even recognize that there *is* a problem, which is *why I wrote this answer in the first place*. – Eric Lippert Jan 15 '14 at 14:15
  • 6
    @Chris: Suppose there is zero work done between the allocation and the return, and between the return and the assignment. We delete all those `Blah` method calls. **What stops a ThreadAbortException from happening at either of those points?** – Eric Lippert Jan 15 '14 at 14:16
  • 1
    @EricLippert - I may or may not be competent to solve the problem, but your suggestion that it's too complicated for any but experts isn't convincing without some attempt to describe (or link to a description of) the problems and techniques to solve them. But I'm looking it from the persepective of a pragmatic developer who wants to produce robust real-world solutions, which perhaps don't necessarily "require that every unmanaged resource be cleaned up no matter what the timing of exceptions". – Joe Jan 15 '14 at 14:24
  • 5
    @Joe: This isn't a debating society; I'm not looking to score points by being more *convincing*. If you're skeptical and don't want to take my word for it that this is a tricky problem that requires consultation with experts to solve correctly then you are welcome to disagree with me. – Eric Lippert Jan 15 '14 at 14:31
  • 1
    @EricLippert: Ah, true. I suspect that in the normal run of events I'd be using resources where experts have already dealt with all these problems (eg framework things) which is why I don't tend to think about them too much. I suspect if i was writing my own classes that dealt with unmanaged resources natively that I'd worry about things a lot more. ;-) – Chris Jan 15 '14 at 14:32
  • 1
    @Joe: Just ask it as a question on here and get answers. :) – Chris Jan 15 '14 at 14:32
  • 2
    @Chris: Correct. That's why we have `SafeHandle`. – Eric Lippert Jan 15 '14 at 14:33
  • 3
    @EricLippert Isn't it completely impossible to defend perfectly against ThreadAbort? No matter what you do, the thread can abort before you do it. (or what http://msdn.microsoft.com/en-us/library/ms228973(v=vs.110).aspx help?) – SLaks Jan 15 '14 at 14:54
  • 3
    @slaks indeed, you can use a CER. – Eric Lippert Jan 15 '14 at 15:10
  • @EricLippert If I were code reviewing your Foo() constructor, I'd ask for it to be modified. I'd wrap everything after int x = AllocateResource() in a try except handler. Inside the except clause I'd put DeallocateResource(x) and then re-throw the original exception. In the case of either Blah2 or Blah3 throwing an exception then the contrustor would not return but the resource would be deallocated. – Giles Roberts Jan 17 '14 at 15:35
  • 7
    @GilesRoberts: How does that solve the problem? Suppose the exception happens *after* the call to `AllocateResource` but *before* the assignment to `x`. A `ThreadAbortException` can happen at that point. Everyone here seems to be missing my point, which is **creation of a resource and assignment of a reference to it to a variable is not an atomic operation**. In order to solve the problem I've identified you must make it an atomic operation. – Eric Lippert Jan 17 '14 at 15:49
  • 4
    @EricLippert Thanks. It took me a while reading your comment to figure this out. You're correct, I'd assumed x = AllocateResource(); was an atomic operation. At least I've hopefully moved out of the set of programmers who wouldn't recognise this as a problem. I'm still in the set of programmers who are not competent to solve this problem. I'd be interested in learning about solutions to this problem. TIL my exception handling code isn't as robust as I thought it was. – Giles Roberts Jan 17 '14 at 17:08
  • 1
    @EricLippert "creation of a resource and assignment of a reference to it to a variable is not an atomic operation" is such a good explanation of the underlying problem that it should be included directly in the answer. – Eldritch Conundrum Mar 20 '14 at 16:44
  • @EricLippert for what it's worth what I was working on for JS has made it to [bluebird](https://github.com/petkaantonov/bluebird) (most of the work wasn't mine) and now has over a million monthly downloads - thanks for the help. On a more interesting note - C# is now discussing adding something similar at https://github.com/dotnet/roslyn/issues/114 – Benjamin Gruenbaum Jul 01 '15 at 12:58
32

As a complement to @SLaks answer, here's the IL for your code:

.method private hidebysig static 
    void Main (
        string[] args
    ) cil managed 
{
    // Method begins at RVA 0x2050
    // Code size 74 (0x4a)
    .maxstack 2
    .entrypoint
    .locals init (
        [0] class [System.Drawing]System.Drawing.Font font3,
        [1] class [System.Drawing]System.Drawing.Font font4,
        [2] bool CS$4$0000
    )

    IL_0000: nop
    IL_0001: ldstr "Arial"
    IL_0006: ldc.r4 10
    IL_000b: newobj instance void [System.Drawing]System.Drawing.Font::.ctor(string, float32)
    IL_0010: stloc.0
    .try
    {
        IL_0011: ldstr "Arial"
        IL_0016: ldc.r4 10
        IL_001b: newobj instance void [System.Drawing]System.Drawing.Font::.ctor(string, float32)
        IL_0020: stloc.1
        .try
        {
            IL_0021: nop
            IL_0022: nop
            IL_0023: leave.s IL_0035
        } // end .try
        finally
        {
            IL_0025: ldloc.1
            IL_0026: ldnull
            IL_0027: ceq
            IL_0029: stloc.2
            IL_002a: ldloc.2
            IL_002b: brtrue.s IL_0034

            IL_002d: ldloc.1
            IL_002e: callvirt instance void [mscorlib]System.IDisposable::Dispose()
            IL_0033: nop

            IL_0034: endfinally
        } // end handler

        IL_0035: nop
        IL_0036: leave.s IL_0048
    } // end .try
    finally
    {
        IL_0038: ldloc.0
        IL_0039: ldnull
        IL_003a: ceq
        IL_003c: stloc.2
        IL_003d: ldloc.2
        IL_003e: brtrue.s IL_0047

        IL_0040: ldloc.0
        IL_0041: callvirt instance void [mscorlib]System.IDisposable::Dispose()
        IL_0046: nop

        IL_0047: endfinally
    } // end handler

    IL_0048: nop
    IL_0049: ret
} // end of method Program::Main

Note the nested try/finally blocks.

David Heffernan
  • 601,492
  • 42
  • 1,072
  • 1,490
17

This code (based on the original sample):

using System.Drawing;

public class Class1
{
    public Class1()
    {
        using (Font font3 = new Font("Arial", 10.0f),
                    font4 = new Font("Arial", 10.0f))
        {
            // Use font3 and font4.
        }
    }
}

It produces the following CIL (in Visual Studio 2013, targeting .NET 4.5.1):

.method public hidebysig specialname rtspecialname
        instance void  .ctor() cil managed
{
    // Code size       82 (0x52)
    .maxstack  2
    .locals init ([0] class [System.Drawing]System.Drawing.Font font3,
                  [1] class [System.Drawing]System.Drawing.Font font4,
                  [2] bool CS$4$0000)
    IL_0000:  ldarg.0
    IL_0001:  call       instance void [mscorlib]System.Object::.ctor()
    IL_0006:  nop
    IL_0007:  nop
    IL_0008:  ldstr      "Arial"
    IL_000d:  ldc.r4     10.
    IL_0012:  newobj     instance void [System.Drawing]System.Drawing.Font::.ctor(string,
                                                                                  float32)
    IL_0017:  stloc.0
    .try
    {
        IL_0018:  ldstr      "Arial"
        IL_001d:  ldc.r4     10.
        IL_0022:  newobj     instance void [System.Drawing]System.Drawing.Font::.ctor(string,
                                                                                      float32)
        IL_0027:  stloc.1
        .try
        {
            IL_0028:  nop
            IL_0029:  nop
            IL_002a:  leave.s    IL_003c
        }  // end .try
        finally
        {
            IL_002c:  ldloc.1
            IL_002d:  ldnull
            IL_002e:  ceq
            IL_0030:  stloc.2
            IL_0031:  ldloc.2
            IL_0032:  brtrue.s   IL_003b
            IL_0034:  ldloc.1
            IL_0035:  callvirt   instance void [mscorlib]System.IDisposable::Dispose()
            IL_003a:  nop
            IL_003b:  endfinally
        }  // end handler
        IL_003c:  nop
        IL_003d:  leave.s    IL_004f
    }  // end .try
    finally
    {
        IL_003f:  ldloc.0
        IL_0040:  ldnull
        IL_0041:  ceq
        IL_0043:  stloc.2
        IL_0044:  ldloc.2
        IL_0045:  brtrue.s   IL_004e
        IL_0047:  ldloc.0
        IL_0048:  callvirt   instance void [mscorlib]System.IDisposable::Dispose()
        IL_004d:  nop
        IL_004e:  endfinally
    }  // end handler
    IL_004f:  nop
    IL_0050:  nop
    IL_0051:  ret
} // end of method Class1::.ctor

As you can see, the try {} block doesn't start until after the first allocation, which takes place at IL_0012. At first glance, this does appear to allocate the first item in unprotected code. However, notice that the result is stored in location 0. If the second allocation then fails, the outer finally {} block executes, and this fetches the object from location 0, i.e. the first allocation of font3, and calls its Dispose() method.

Interestingly, decompiling this assembly with dotPeek produces the following reconstituted source:

using System.Drawing;

public class Class1
{
    public Class1()
    {
        using (new Font("Arial", 10f))
        {
            using (new Font("Arial", 10f))
                ;
        }
    }
}

The decompiled code confirms that everything is correct and that the using is essentially expanded into nested usings. The CIL code is a bit confusing to look at, and I had to stare at it for a good few minutes before I properly understood what was happening, so I'm not surprised that some 'old wives tales' have started to sprout up about this. However, the generated code is the unassailable truth.

Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
Tim Long
  • 13,508
  • 19
  • 79
  • 147
  • @Peter Mortensen your edit removed chunks of the IL code (between IL_0012 and IL_0017) rendering the explanation both invalid and confusing. That code was intended to be a _verbatim_ copy of the results I obtained and editing invalidates that. Can you please review your edit and confirm this is what you intended? – Tim Long Jul 27 '14 at 23:15
7

Here is a sample code to prove @SLaks answer:

void Main()
{
    try
    {
        using (TestUsing t1 = new TestUsing("t1"), t2 = new TestUsing("t2"))
        {
        }
    }
    catch(Exception ex)
    {
        Console.WriteLine("catch");
    }
    finally
    {
        Console.WriteLine("done");
    }

    /* outputs

        Construct: t1
        Construct: t2
        Dispose: t1
        catch
        done

    */
}

public class TestUsing : IDisposable
{
    public string Name {get; set;}

    public TestUsing(string name)
    {
        Name = name;

        Console.WriteLine("Construct: " + Name);

        if (Name == "t2") throw new Exception();
    }

    public void Dispose()
    {
        Console.WriteLine("Dispose: " + Name);
    }
}
Wagner DosAnjos
  • 6,304
  • 1
  • 15
  • 29
  • 1
    That does not prove it. Where is Dispose: t2? :) – Piotr Perak Jan 15 '14 at 07:52
  • 1
    The question is about the dispose of the first resource on the using list not the second. _"What happens if `font4 = new Font` throws? From what I understand font3 will leak resources and won't be disposed of."_ – Wagner DosAnjos Jan 15 '14 at 13:06