<< Rhino Service Bus: Field Level Security | Home | Rhino Service Bus: Saga and State >>

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.

Print | posted on Thursday, January 15, 2009 6:07 AM

Feedback

Gravatar

# re: Avoid object initializers & the using statement 1/15/2009 8:24 AM 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.

Gravatar

# re: Avoid object initializers & the using statement 1/15/2009 10:37 AM 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" } )

Gravatar

# re: Avoid object initializers & the using statement 1/15/2009 1:48 PM 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

Gravatar

# re: Avoid object initializers & the using statement 1/15/2009 1:50 PM 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?

Gravatar

# re: Avoid object initializers & the using statement 1/15/2009 1:51 PM 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.

Gravatar

# re: Avoid object initializers & the using statement 1/15/2009 2:51 PM Andrew

Thanks for the response Configurator.
After a little bit of thought that was the conclusion I had come to.

Gravatar

# re: Avoid object initializers & the using statement 1/15/2009 4:01 PM 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.

Gravatar

# re: Avoid object initializers & the using statement 1/15/2009 4:38 PM 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.

Gravatar

# re: Avoid object initializers & the using statement 1/15/2009 5:10 PM 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

Gravatar

# re: Avoid object initializers & the using statement 1/15/2009 5:17 PM 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?

Gravatar

# re: Avoid object initializers & the using statement 1/15/2009 6:39 PM 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.

Gravatar

# re: Avoid object initializers & the using statement 1/15/2009 7:04 PM 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.

Gravatar

# re: Avoid object initializers & the using statement 1/16/2009 2:09 AM Mats Helander

Wow. Sloppy.

Gravatar

# re: Avoid object initializers & the using statement 1/16/2009 6:33 AM Jomit


Ahhh !!! that was a good insight.

Gravatar

# re: Avoid object initializers & the using statement 1/16/2009 9:54 AM Tom

Hi,

I have seen a similar example earlier here:

http://www.codeproject.com/KB/cs/CSharp_using.aspx

Gravatar

# re: Avoid object initializers & the using statement 1/16/2009 10:15 AM 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.

Gravatar

# re: Avoid object initializers & the using statement 1/16/2009 10:30 AM 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.

Gravatar

# re: Avoid object initializers & the using statement 1/16/2009 3:03 PM 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.

Gravatar

# re: Avoid object initializers & the using statement 1/16/2009 3:51 PM 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.

Gravatar

# re: Avoid object initializers & the using statement 1/16/2009 4:21 PM Ayende Rahien

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

Gravatar

# re: Avoid object initializers & the using statement 1/16/2009 5:05 PM 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.

Gravatar

# re: Avoid object initializers & the using statement 1/16/2009 11:02 PM 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");
}

Gravatar

# re: Avoid object initializers & the using statement 1/17/2009 1:02 AM 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/

Gravatar

# re: Avoid object initializers & the using statement 1/19/2009 4:44 PM Steve Wagner

Dose someone reported this to JetBrains as feature request?

Gravatar

# re: Avoid object initializers & the using statement 1/22/2009 7:47 PM 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.

Gravatar

# re: Avoid object initializers & the using statement 1/22/2009 9:02 PM 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.

Gravatar

# re: Avoid object initializers & the using statement 1/23/2009 2:00 PM Steve Wagner

Ive reported it to JetBrains. Feel free to vote.

http://jetbrains.net/jira/browse/RSRP-92815

Comments have been closed on this topic.