"CA2000: Dispose objects before losing scope" sounds like good advice. Your dad might say that before you head off to college, or maybe you heard it from a Jedi master. Too bad this advice is bullshit.

Note: this article documents a real bug in a real system. The code on display is unrelated to business logic, and the names have been changed solely for my entertainment.

One day, Wally was doing his thing down in dev ops when he came across a set of compiler warnings that had not yet been eradicated. Being a conscientious fellow, he immediately set about causing their downfall. Among others, these two were present in abundance:

The short version is that both of these are bullshit. However, someone else has a blog post on CA2202, so we're only going to discuss the former.

Wally sent a pull request adjusting the patterns in use for IDisposable objects practically throughout the codebase. After some consternation, the developer responsible (we'll call him Handsom Jack) decided the changes were by and large harmless and that enough of them were genuinely positive to justify the merge. He would live to regret that decision.

welcome-to-hell-1

What's the problem?

The problem is that you should only dispose of an object when you're done with it. The problem is that it isn't always possible to know when you're done with an object unless you have a deep understanding of the code. Here's the original method body:

var request = new HttpRequestMessage(HttpMethod.Post, address);
var content = new StringContent(Serializer.ToJson(payload));

request.Content = content;
request.Content.Headers.ContentType = new MediaTypeHeaderValue("application/json");
_authProvider.Authorize(request);

return _client.SendAsync(request);

Clearly, request goes out of scope after the return statement, which means that the following modification should be perfectly safe:

HttpRequestMessage request = null;
Task<HttpResponseMessage> response = null;

try
{
    request = new HttpRequestMessage();

    request.Method = HttpMethod.Post;
    request.RequestUri = new Uri(address);
    request.Content = new StringContent(Serializer.ToJson(payload));
    request.Content.Headers.ContentType = new MediaTypeHeaderValue("application/json");
    _authProvider.Authorize(request);

    response = _client.SendAsync(request);
}
finally
{
    if (request != null)
    {
        request.Dispose();
    }
}
return response;  

...But it's not. Not even close. See, response keeps a reference to request, which means that request will, in fact, survive carbage collection for at least a few more cycles. It also means, once request has been disposed, response becomes only so much worthless garbage. Accessing the response stream will now spew esoteric but vulgar exception messages into your log. Embarrassingly, this is a bug Handsome Jack has fixed before during his long tenure, but Handsome Jack did not notice it during the review—and neither was this code path tested before deployment.

What's the fix?

Basically, the price of freedom is actually eternal laziness. Compiler warnings are just that: warnings that you may be doing something wrong. They are not the incontrovertible word of the Lord, but more the kind of self-righteous advice your neighbor snipes from across the fence every time you bring out the mower.

...Like that son of a bitch even does his own lawn...