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.

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" } )

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

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?

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

Thanks for the response Configurator.

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

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.

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

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

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?

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

@Rayn

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

Wow. Sloppy.

Ahhh !!! that was a good insight.

Hi,

I have seen a similar example earlier here:

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

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

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.

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.

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.

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

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.

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");

}

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/

Dose someone reported this to JetBrains as feature request?

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.

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.

Ive reported it to JetBrains. Feel free to vote.

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