Ayende @ Rahien

It's a girl

Avoid object initializers & the using statement

This piece of code has a huge bug in it. Can you see it?

image

I’ll give you a hint, what is going to happen if Key, IV or Mode are going to throw an exception? Well, let us check the actual generated code from Reflector.

image

As you can see, we first set the value to a local variable, we set the properties and only then we enter the using clause. If any of the properties will throw out an exception, we will never enter the using block, and the object will never be disposed.

Personally, I think that this is an issue with the compiler, that it should be able to generate safe code for this scenario. As it is, the code looks fine, but the subtle bug can come and bite you.

Beware.

Comments

Barry Dahlberg
01/15/2009 06:24 AM by
Barry Dahlberg

Nasty. This is also a problem if you are chaining methods like in this contrived example:

using(var thing = new Thing().WithWheels())

{

}

If WithWheels throws the object will never be disposed.

Dave
01/15/2009 08:37 AM by
Dave

I already commit this bug during the VS beta stage at the connect site, but Microsoft developers say it's by design. I still hope they fix it in VS2010. The Mono compiler (mcs, nightly build) doesn't have this issue.

The compiler should first create the object (without the initialiser) because using is the outer statement. And inside the using statement assign the initialisers.

Because I'm aware of this issue, I generally don't use object initializers except when using linq (new object { Foo="a", Bar="b" } )

andrew
01/15/2009 11:48 AM by
andrew

Hi Ayende,

Would the same be true for this code on the outer using clause?

    Using responseStream As Stream = webResponse.GetResponseStream

        Using memStream As New MemoryStream

            Do

                count = responseStream.Read(buffer, 0, buffer.Length)

                memStream.Write(buffer, 0, count)

            Loop While count <> 0


            result = memStream.ToArray

        End Using

    End Using
configurator
01/15/2009 11:50 AM by
configurator

Ouch, this look very much like a compiler bug. I was sure it would compile to something like

using (RijandelManaged <>g__initLocal1 = new RijandelManager())

{

<>g__initLocal1.Key = this.Key;

/// etc.

}

With Barry's example it is more obviously by design - only the return value of WithWheels should be in the using clause. Even if this is a fluent API and WithWheels returns the same object as Thing, how could the compiler know it?

configurator
01/15/2009 11:51 AM by
configurator

@Andrew, this would be disposed of properly as there is no object initializer to fail before the outer using. If GetRespnseStream fails, you really have nothing to do about that whatever you do because you never get the object to dispose of.

Andrew
01/15/2009 12:51 PM by
Andrew

Thanks for the response Configurator.

After a little bit of thought that was the conclusion I had come to.

Eldon Ferran de Pol
01/15/2009 02:01 PM by
Eldon Ferran de Pol

I just mentioned this in the office and someone pointed out that this could be viewed as technically correct as the Initialiser code is prior to the curly brace that starts the using block. Maybe that is why it is "by design"?

However, to me it seems this is still wrong as anything that is a compiler trick, such as object initialisers, should compile down to the code that would have been written if the compiler trick didn't exist.

josh
01/15/2009 02:38 PM by
josh

huh.. i've never run into that situation but I can see the danger in it. I'm definitely going to use this tip.

ZeusTheTrueGod
01/15/2009 03:10 PM by
ZeusTheTrueGod

I guess resharper could detect such assigments like it detects variables in modified closures. I hope someone with licensed version of resharper could send a request to resharper team

And now I will be aware about object initializers

Tool
01/15/2009 03:17 PM by
Tool

Sorry if this is a dumb question, but even though it's a bad thing to create something that's never used and just sits there (I assume this would be classified as a memory leak), won't it just be disposed when the method it's being used in ends?

SomeMethod()

{

using(var thing = new Thing())

{

...

}

}

Once SomeMethod completes, won't the item be popped off the stack and therefore the object lose it's reference? Is this more important with something like memory streams rather than just normal objects?

Rayn
01/15/2009 04:39 PM by
Rayn

@tool:

No, objects aren't GC'd when the method ends, they're GC'd when the the .Net framework is 'good and ready'.

This garbage collection can take place minutes, hours, etc. from when the routine exited. It all depends on the perceived memory pressure and resource usage on the machine.

Tool
01/15/2009 05:04 PM by
Tool

@Rayn

Gotcha, for some reason I had thought it ran fairly often and just rounded up all non referenced objects. That does change things.

Jomit
01/16/2009 04:33 AM by
Jomit

Ahhh !!! that was a good insight.

Fabian Schmied
01/16/2009 08:15 AM by
Fabian Schmied

@Tom: The example on CodeProject is quite different. With an exception thrown from the constructor, using cannot call Dispose (because therere is no object reference to call it on). Therefore, constructors of disposable objects should make sure they clean up themeselves when they throw.

Ayende's problem is different: In his code, the constructor has already finished when the object initializers are executed. The property setters cannot be required to clean up when they throw (this would usually not be what you want), so the using block should do it.

@Dave: Do you have a Connect URL? I searched for your report, but didn't find it.

Tom
01/16/2009 08:30 AM by
Tom

Yes, you are right, that is a different problem. However I think it is also something to consider together with this. It is because the other example shows that even if you put the Key, IV or Mode to the constructor of RijndaelManaged there will be no Dispose if they throw an exception.

Tomas
01/16/2009 01:03 PM by
Tomas

If the construction of the object throws, is the object still in a well-defined state? If it is not, perhaps it is unsafe to dispose of it for that reason.

John Chapman
01/16/2009 01:51 PM by
John Chapman

I disagree that the compiler should be doing something different here. You want to change what the object initializer means. An object initializer is really meant to mimic the behavior of a constructor.

If you had those properties as arguments to a constructor instead of through an object initializer you surely wouldn't have expected the using to handle disposing of the object would you?

Many people use object initializers in their method parameters. There would be many issues if the instance was returned for usage before assigning properties. That's all we're really doing here. Create object, set properties and then return the IDisposable instance. Many scenarios would not work with object initializers if they behaved as you say.

Ayende Rahien
01/16/2009 02:21 PM by
Ayende Rahien

If the ctor has finished, yes, it is in valid state by definition

Lee Campbell
01/16/2009 03:05 PM by
Lee Campbell

Ahhh all the more to have immutable value types. (Yeah I know you cant change the RijndaelManaged class).

While I suppose it would be nice if the code you posted worked, my assumption would have been that it would work they way it does work. However this doesn't live up to the design goals of "falling into the pit of success" I suppose.

Erhan Hosca
01/16/2009 09:02 PM by
Erhan Hosca

i don't see the "bug" here ...

how is this different to what happens to all the assigned procedure level variables when an exception is thrown.. case in point

void SubA()

{

try{

SubB();

}

catch{

// what happened to variable rm in SubB?

}

}

void SubB()

{

RijndaelManaged rm = new RijndaelManaged();

rm.SomeProperty = "SomeTextValue";

throw new ApplicationException("error");

}

Fj
01/16/2009 11:02 PM by
Fj

Dave, can you please post a link to a Connect bug report?

From my own experience, there is quite a big variance in reviewer's attitudes, so by "bumping" the report we can make the fix appear earlier/

Steve Wagner
01/19/2009 02:44 PM by
Steve Wagner

Dose someone reported this to JetBrains as feature request?

Eric Lippert
01/22/2009 05:47 PM by
Eric Lippert

The behaviour is correct and desirable. The spec is extremely clear on this point and if an implementation of C# does not have this behaviour, they have a bug.

The spec clearly states that "using(T x = y) s;" means exactly the same thing as "{ T x = y; try { s; } finally { ... code to dispose x ... }". That is, the initialization is done outside the try-protected region, precisely so that if it fails, we do NOT attempt to clean it up. That would be dangerous.

The spec also clearly states that "t = new T() { X = x };" means exactly the same thing as "T temp = new T(); temp.X = x; t = temp;". That is, the assignment to the variable happens after the object is known to be in its fully initialized state.

These two designs work together to ensure that you never dispose a partially initialized resource. If you have a situation where you need to handle disposing of partially constructed resources then you need to write the code yourself to do that; there's no way we can generate the code for you and have any confidence that it is right. Any operation on a partially initialized resource is potentially hazardous.

Ayende Rahien
01/22/2009 07:02 PM by
Ayende Rahien

Eric,

I am not disputing that this is not the correct behavior as far as the spec goes.

I am disputing that this is the expected thing to happen as far as the user thinks about things.

From my perspective, this is the same as a memory leak, it is very easy to get it wrong in a non obvious way.

Yes, it is a bug, but it is a very easy one.

As far as partially constructed objects are concerned, if I successfully created an instance, it is constructed. A class should be ready for someone doing new Foo().Dispose();

The problem is that since the property assignment is so much nicer than the other way, people will use that, and get this issue.

Comments have been closed on this topic.