Hidden Pitfalls With Object Initializers

code 0 comments suggest edit

I love automation. I’m pretty lazy by nature and the more I can offload to my little programmatic or robotic helpers the better. I’ll be sad the day they become self-aware and decide that it’s payback time and enslave us all.

But until that day, I’ll take advantage of every bit of automation that I can.

the-matrix-humans

For example, I’m a big fan of the Code Analysis tool built into Visual Studio. It’s more more commonly known as FxCop, though judging by the language I hear from its users I’d guess it’s street name is “YOU BIG PILE OF NAGGING SHIT STOP WASTING MY TIME AND REPORTING THAT VIOLATION!”

Sure, it has its peccadilloes, but with the right set of rules, it’s possible to strike a balance between a total nag and a helpful assistant.

As a developer, it’s important for us to think hard about our code and take care in its crafting. But we’re all fallible. In the end, I’m just not smart enough to remember ALL the possible pitfalls of coding ALL OF THE TIME such as avoiding the Turkish I problem when comparing strings. If you are, more power to you!

I try to keep the number of rules I exclude to a minimum. It’s saved my ass many times, but it’s also strung me out in a harried attempt to make it happy. Nothing pleases it. Sure, when it gets ornery, it’s easy to suppress a rule. I try hard to avoid that because suppressing one violation sometimes hides another.

Here’s an example of a case that confounded me today. The following very straightforward looking code ran afoul of a code analysis rule.

public sealed class Owner : IDisposable
{
    Dependency dependency;

    public Owner()
    {
        // This is going to cause some issues.
        this.dependency = new Dependency { SomeProperty = "Blah" };
    }

    public void Dispose()
    {
        this.dependency.Dispose();
    }
}

public sealed class Dependency : IDisposable
{
    public string SomeProperty { get; set; }
        
    public void Dispose()
    {
    }
}

Code Analysis reported the following violation:

CA2000 Dispose objects before losing scope \ In method ‘Owner.Owner()’, object ‘<>g__initLocal0’ is not disposed along all exception paths. Call System.IDisposable.Dispose on object ‘<>g__initLocal0’ before all references to it are out of scope.

That’s really odd. As you can see, dependency is disposed when its owner is disposed. So what’s the deal?

Can you see the problem?

A Funny Thing about Object Initializers

A2600_PitfallThe big clue here is the name of the variable that’s not disposed, <>g__initLocal0. As Phil Karlton once said, emphasis mine,

There are only two hard things in Computer Science: cache invalidation and naming things.

Naming may be hard, but I can do better than that. Clearly the compiler came up with that name, not me. I fired up Reflector to see the generated code. The following is the constructor for Owner.

public Owner()
{
    Dependency <>g__initLocal0 = new Dependency {
        SomeProperty = "Blah"
    };
    this.dependency = <>g__initLocal0;
}

Aha! So we can see that the compiler generated a temporary local variable to hold the initialized object while its properties are set, before assigning it to the member field.

So what’s the problem? Well if for some reason setting SomeProperty throws an exception, <>g__initiLocal0 will never be disposed. That’s what the Code Analysis is complaining about. Note that if an exception is thrown while setting that property, my member field is also never set to the instance. So it’s a dangling undisposed instance.

So what’s the Plan Stan?

Well the fix to keep code analysis happy is simple in this case.

public Owner()
{
    this.dependency = new Dependency();
    this.dependency.SomeProperty = "Blah";
}

Don’t use the initializer and set the property the old fashioned way.

This shuts up CodeAnalysis, but did it really solve the problem? Not in this specific case because we happen to be inside a constructor. If the Owner constructor throws, nobody is going to dispose of the dependency.

As Greg Beech wrote so long ago,

From this we can ascertain that if the object is not constructed correctly then the reference to the object will not be assigned, which means that no methods can be called on it, so the Dispose method cannot be used to deterministically clean up managed resources. The implication here is that if the constructor creates expensive managed resources which need to be cleaned up at the earliest opportunity then it should do so in an exception handler within the constructor as it will not get another chance.

So a more robust approach would be the following.

public Owner()
{
    this.dependency = new Dependency();
    try
    {
        this.dependency.SomeProperty = "Blah";
    }
    catch (Exception)
    {
        dependency.Dispose();
        throw;
    }       
}

This way, if setting the properties of Dependency throws an exception, we can dispose of it properly.

Why isn’t the compiler smarter?

I’m not the first to run into this pitfall with object initializers and disposable instances. Ayende wrote about a related issue with using blocks and object initializers back in 2009. In that post, he suggests the compiler should generate safe code for this scenario.

It’s an interesting question. Whenever I think of such questions, I put on my Eric Lippert hat and hear his proverbial voice (I’ve never heard his actual voice but I imagine it to be sonorous and profound) in my head saying:

I’m often asked why the compiler does not implement this feature or that feature, and of course the answer is always the same: because no one implemented it. Features start off as unimplemented and only become implemented when people spend effort implementing them: no effort, no feature. This is an unsatisfying answer of course, because usually the person asking the question has made the assumption that the feature is so obviously good that we need to have had a reason tonot implement it. I assure you that no, we actually don’t need a reason to not implement any feature no matter how obviously good. But that said, it might be interesting to consider what sort of pros and cons we’d consider if asked to implement the “silently put inferred constraints on class type parameters” feature.

The current implementation of object initializers seems correct for most cases. The only time it breaks down is in the case of disposable types. So let’s think about some possible solutions.

Why the intermediate variable?

First, let’s look at why the intermediate local variable. My initial knee-jerk reaction (ever notice how often your knee-jerk reaction makes you sound like jerk?) was that the intermediate variable is unecessary. But I thought about it some more and came up with the scenario. Suppose we’re setting a property to the value of an object created via an initializer.

this.SomePropertyWithSideEffects = new Dependency { Prop = 42 };

The way to do this without an intermediate local variable is the following.

this.SomePropertyWithSideEffects = new Dependency();
this.SomePropertyWithSideEffects.Prop = 42;

The first code block only calls the setter of SomePropertyWithSideEffects. But the second code block calls both the getter and setter. That’s pretty different behavior.

Now imagine we’re setting multiple properties or using a collection initializer with multiple items instead. We’d be calling that property getter multiple times. Who knows what awful side-effects that might produce. Sure, side effects in property getters are bad, but as I’ll point out later, there’s another reason this approach is fraught with error.

The intermediate local variable is necessary to ensure the object is only assigned after it’s fully constructed.

Dispose it for me?

So given that, let’s try implementing the the Owner constructor of my previous example the way a compiler might do it.

public Owner()
{
    var <>g__initLocal0 = new Dependency();
    try
    {
        <>g__initLocal0.SomeProperty = "Blah";
    }
    catch (Exception)
    {
        <>g__initLocal0.Dispose();
        throw;
    }
    this.dependency = <>g__initLocal0;
}

That’s certainly seems much safer, but there’s still a potential flaw. It’s optimistically calling dispose on the object when the exception is thrown. What if I didn’t want to call dispose on it even though it’s disposable? Maybe the Dispose method of this specific object deletes your hard-drive and plays Justin Bieber music when invoked. In 99.9 times out of 100, you would want Dispose called in this case. But this is still a change in behavior and I can understand why the compiler might not risk it.

Perhaps the compiler could attempt to figure out if that instance eventually gets disposed and do the right thing. All you have to do find a flaw in Turing’s proof of the Halting Problem. No problem, right?

Perhaps we could be satisfied with good enough. Dispose it always and just say that’s the behavior of object initializers. It’s probably too late for that change as that’d be a breaking change. It’d be one I could live with honestly.

Let me dispose it

Perhaps the problem isn’t that we want the compiler to automatically dispose of the intermediate object in the case of an exception. What we really want is the assignment to  happen no matter what so we can dispose of it in our code if an exception is thrown. Perhaps the compiler can generate code that would allow us to do this in our code.

public Owner()
{
    try
    {
        this.dependency = new Dependency { SomeProperty = "blah" };
    }
    catch (Exception)
    {
        if (this.dependency != null)
            this.dependency.Dispose();
    }
}

What might the generated code look like in this case?

public Owner()
{
    var <>g__initLocal0 = new Dependency();
    this.dependency = <>g__initLocal0;
    <>g__initLocal0.SomeProperty = "Blah";
}

That’s not too shabby. We got rid of the try/catch block that we had to introduce previously, and if an exception is thrown in the property setter, we can clean up after it. I’m a genius!

Not so fast Synkowski. There’s a potential problem here. Suppose we’re not inside a constructor, but rather are in a method that’s setting a shared member.

public void DoStuff()
{
    var <>g__initLocal0 = new Dependency();
    this.dependency = <>g__initLocal0;
    <>g__initLocal0.SomeProperty = "Blah";
}

We’ve now introduced a possible race condition if this method is used in an async or multithreaded environment.

Notice that after this.dependency is set to the local incomplete instance, but before the local property is set, there’s room for another thread to modify this.dependency in some way right in that gap leading to indeterminate results. That’s definitely a change you wouldn’t want the compiler doing.

In fact, this same issue affects my earlier proposal of not using an intermediate variable.

So about that Code Analysis

Note that I didn’t specifically address Ayende’s case. In his case, the initializer is in a using block. That seems like a more tractable problem for the compiler to solve, but this post is getting long as it is and it’s time to wrap up. Maybe someone else can analyze that case for shits and giggles.

In my case, we’re setting a member that we plan to dispose later. That’s a much harder (if not impossible) nut to crack.

And here we get to the moral of the story. I get a lot more work done when I don’t stop every hour to write a blog post about some interesting bug I found in my code.

No wait, that’s not it.

The point here is that code analysis is a very helpful tool for writing more robust and correct code. But it’s just an assistant. It’s not a safety net. It’s more like an air bag. It’ll keep you from splattering your brains on the dashboard, but you can still totally wreck your car and break that nose if you’re not careful at the wheel.

Here’s an example where automated tools can both lead you into an accident, but save your butt at the last second.

If you use Resharper (another tool with its own automated analysis) like I do and you write some code in a constructor that doesn’t use an object initializer, you’re very likely to see this (with the default settings).

resharper-nag

See that green squiggly under the new keyword just inviting, no begging, you to hit ALT+ENTER and convert that bad boy into an object initializer? Go ahead, it seems to suggest. What could go wrong? Oh it could cause you to now leak a resource as pointed out earlier.

I often like to hit CTRL E + CTRL C in Resharper to reformat my entire source file to be consistent with my coding standards. Depending on how you set up the reformatting, such an automatic action could easily change this code from working code to subtly broken code.

I still have to pay careful attention to what it’s doing. It’s easy to get lulled into a sense of safety when performing automatic refactorings. But you can’t trust it one hundred percent. You are the one who is responsible, not the tools. You are the one in control.

Fortunately in this case, Code Analysis brought this issue to my attention. And in doing so, pointed out an interesting topic for a blog post. Yay automation!

Found a typo or error? Suggest an edit! If accepted, your contribution is listed automatically here.

Comments

avatar

34 responses

  1. Avatar for Pius Ngugi
    Pius Ngugi January 10th, 2013

    Great post!

  2. Avatar for Daniel Marbach
    Daniel Marbach January 10th, 2013

    Hy Phil,
    Thanks for the post. Think this is especially important when you are using third party code. When I program against my own code or some of my team mates we rely on the fact that we are TDDing our code. So we avoid complex object construction and properties with business logic in it. That way we can assume that a constructor or a property does not throw.

    How about this one: ;)

                using (var stream = new MemoryStream())            using (var writer = new StreamWriter(stream) { AutoFlush = true })            {                writer.Write(message.Xml);                stream.Seek(0, SeekOrigin.Begin);                this.documentSession.StoreAttachement(                    "someId", null, stream, new RavenJObject { { "Content-Type", "application/xml" }, });            }

    Try to get rid of the following violation:

    "Microsoft.Usage", "CA2202:Do not dispose objects multiple times"

    Have fun

  3. Avatar for Pierre Chalamet
    Pierre Chalamet January 10th, 2013

    But if you stick to RAII principle this should be no brain. Seems you are blaming compiler/resharper but this kind of code is lame anyway.

  4. Avatar for Daniel Marbach
    Daniel Marbach January 10th, 2013

    I put up a public gist because disqus destroyed the code in my comment. Here it is: https://gist.github.com/450...

  5. Avatar for Sean Kearon
    Sean Kearon January 11th, 2013

    Awesome!  Maybe you should put in a R# feature request to not show the object initialiser suggestion in a ctor?

  6. Avatar for Johnno Nolan
    Johnno Nolan January 11th, 2013

    I'm a little bit wary of object initializers for OO reasons too. It's so easy to throw up an  object & initialize it this way but what this does is that it encourages more of your internals to be exposed. Soon your code base is full of classes with properties that you can get & set at will, which makes me feel a little bit queasy. I've droned on about it in the past. http://johnnosnose.blogspot...

  7. Avatar for WickyNilliams
    WickyNilliams January 11th, 2013

    Another, slightly related, problem with object initialisers: If you are initialising an object with a lot of properties and something goes wrong with with one of the set operations, the stack trace doesn't tell you which set failed. Instead it points to the first line of the initialiser, meaning it can be hard to diagnose issues from logs.

  8. Avatar for haacked
    haacked January 11th, 2013

    Hi Daniel, I fixed up your code comment. But a gist is always welcome. I wish Disqus supported markdown.

  9. Avatar for Aaron
    Aaron January 12th, 2013

    Why would you bother to dispose a memory stream?

  10. Avatar for Damian Hickey
    Damian Hickey January 13th, 2013

    I've previously blogged on this particular resharper annoyance http://dhickey.ie/post/2011...

    The corresponding JetBrains issue if you want to vote for it http://youtrack.jetbrains.c...

  11. Avatar for Daniel Marbach
    Daniel Marbach January 13th, 2013

    It's a stream. You have to dispose them. Even if it is a memory stream. FxCop also enforces this. Without you would leak memory.

  12. Avatar for Vick Mukherjee
    Vick Mukherjee January 14th, 2013

    This was great!

  13. Avatar for James
    James January 15th, 2013

    If a property assignment throws an exception, you're in violation of the object property contract? IE, a property get/set should never fail. If it does, it's because of your codebase, not a failure of the language or compiler.

    Run the same test code, but have it so that the setter for SomeProperty throws an exception (as you assumed at the outset). Then run the analysis again. You'll see what I mean.

  14. Avatar for Jerkimball
    Jerkimball January 15th, 2013

    This had me crying:

    FxCop, though judging by the language I hear from its users I’d guess it’s street name is “YOU BIG PILE OF NAGGING SHIT STOP WASTING MY TIME AND REPORTING THAT VIOLATION!”

    Well stated, sir...well stated.

  15. Avatar for maciek
    maciek January 15th, 2013

    In my opinion object construction is a responsibility of DI container, not your object. It solves the problem.

  16. Avatar for Tobias Zürcher
    Tobias Zürcher January 16th, 2013

    nice article! but remember: fxcop will complain about catch (Exception) {} 

  17. Avatar for Rich
    Rich January 18th, 2013

    Phil,

    The R# suggestion is fine in your example - you are in no worse a position. Consider the case where the property setter throws. Your Owner constructor will never complete and as such no instance of Owner will ever be created that can be disposed.

    Rich

  18. Avatar for Jahmal Lewis
    Jahmal Lewis January 18th, 2013

    Very insightful.  But Phil, shouldn't we be injecting our dependencies rather than initializing them in the owner anyway??

  19. Avatar for haacked
    haacked January 20th, 2013

    Not in every possible case. Sometimes you need to create a temporary stream and dispose of it. Or you're implementing a wrapper class+interface for a type that doesn't implement an interface. So you inject your wrapper but keep the instance being wrapped from bleeding into the public API.

  20. Avatar for haacked
    haacked January 20th, 2013

    Rich, maybe it wasn't clear but the issue is not disposing the Owner instance. It's the Dependency instance that was just created. THAT doesn't get disposed after the R# change. Make sense.

  21. Avatar for Rich
    Rich January 21st, 2013

    Hi Phil, my point was that between the first version of the constructor:

        this.dependency = new Dependency();
    this.dependency.SomeProperty = "Blah";

    and the R# refactored version:

    this.dependency = new Dependency { SomeProperty = "Blah" };

    you are in no worse a position if the property setter throws. The Dependency instance will never be disposed in either case.

    Cheers,
    Rich

  22. Avatar for haacked
    haacked January 21st, 2013

    Ah, good point. The dependency member is assigned, but since the Owner wasn't fully constructed, its Dispose method is never called. :)

  23. Avatar for doekman
    doekman January 21st, 2013

    question answered.

  24. Avatar for Stilgar
    Stilgar January 26th, 2013

    Great article!

    Still I don't understand why you think Resharper is wrong to suggest using object initializer. As you pointed out you can make FxCop happy if you do not use object initializer but you did not solve the problem. If SomeProperty throws your reference is not disposed anyway so why not use object initializer?

  25. Avatar for Knocte
    Knocte February 2nd, 2013

     Just FYI: Gendarme is a good static analysis tool (like FxCop) which is open source (so you can write rules for it) and which already includes rules such as:
    - Not disposing any IDisposable fields in the Dispose method of a class.
    - Not disposing any IDisposable var created within a method.

    And the latter, I think, is prepared to deal with cases in which you assign to a field the value of the IDisposable var, in which it doesn't complain about not disposing it (because it assumes that it's going to be used later by the class, and it should be the class the responsible one to dispose it).

  26. Avatar for Isantipov
    Isantipov February 23rd, 2013

    Hi Phil,

    Thanks for the post. Do you use any analysis tools other than fxCop and resharper at Github? (Maybe StyleCop/Gendarme)

  27. Avatar for haacked
    haacked February 23rd, 2013

    We only use R# and FxCop for analysis. I'm not to keen on StyleCop as I understand you can't customize the styles.

    I am interested in trying out other analysis tools if I think they'd help.

  28. Avatar for Isantipov
    Isantipov February 24th, 2013

    >>  I'm not to keen on StyleCop as I understand you can't customize the styles.

    I think you can implement your own rules for StyleCop (see this reference on StyleCop Plus wiki http://stylecopplus.codeple... ) and you can also use the StyleCop+ plugin which extends original rules and gives more flexibility (as the website claims). Although, I never tried - we are just happy with the core rules at the moment.

  29. Avatar for JJ
    JJ May 8th, 2013

    You all need to be aware of the difference in the object initializer code which is produced in release / debug builds. This whole post relates to debug builds. Look at the code procued in relases mode, you will not get the temporary named object assignment.

  30. Avatar for Ludmila
    Ludmila March 4th, 2014

    Really good post!

  31. Avatar for Brian
    Brian October 21st, 2014

    Your trading off performance for an error that is extremely unlikely to happen. Object Initializers optimize the initialization of the object over straight property setters. Thumbs Down.

  32. Avatar for haacked
    haacked October 21st, 2014

    There is no performance benefit of using object initializers. Try it yourself, you'll see that the IL that's generated is _exactly_ the same.

  33. Avatar for Sam Bronson
    Sam Bronson October 15th, 2015

    But the garbage collector reclaims memory in a timely fashion, doesn't it? I suppose defusing the finalizer would help it do so one collection earlier, but still, it's not like it's a file handle or anything.

  34. Avatar for Latika Shetty
    Latika Shetty June 8th, 2016

    Thanks for the post. Helped solve the CA instantly