Ayende @ Rahien

Hi!
My name is Oren Eini
Founder of Hibernating Rhinos LTD and RavenDB.
You can reach me by phone or email:

ayende@ayende.com

+972 52-548-6969

, @ Q c

Posts: 18 | Comments: 76

filter by tags archive

Avoid object initializers & the using statement

time to read 1 min | 180 words

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

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

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

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

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

@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

Thanks for the response Configurator.

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

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

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

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

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

@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

@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

Ahhh !!! that was a good insight.

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
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

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

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

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

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

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
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

Dose someone reported this to JetBrains as feature request?

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

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.

Comment preview

Comments have been closed on this topic.

FUTURE POSTS

  1. Production postmortem: The industry at large - one day from now
  2. The insidious cost of allocations - about one day from now
  3. Buffer allocation strategies: A possible solution - 5 days from now
  4. Buffer allocation strategies: Explaining the solution - 6 days from now
  5. Buffer allocation strategies: Bad usage patterns - 7 days from now

And 2 more posts are pending...

There are posts all the way to Sep 11, 2015

RECENT SERIES

  1. Find the bug (5):
    20 Apr 2011 - Why do I get a Null Reference Exception?
  2. Production postmortem (10):
    01 Sep 2015 - The case of the lying configuration file
  3. What is new in RavenDB 3.5 (7):
    12 Aug 2015 - Monitoring support
  4. Career planning (6):
    24 Jul 2015 - The immortal choices aren't
View all series

Syndication

Main feed Feed Stats
Comments feed   Comments Feed Stats