Beware of undisposed or unconsumed HttpResponseMessage

[Level T2] This is just a short post to bring something important to the reader's attention. I guess if you are well into ASP.NET Web API, you are bound to see it - especially if you are consuming APIs using HttpClient.

Let's look at this innocent looking delegating handler (useful on the client side):

public class InnocentLookingDelegatingHandler : DelegatingHandler
{

    protected override Task SendAsync(HttpRequestMessage request, 
        CancellationToken cancellationToken)
    {
        return base.SendAsync(request, cancellationToken)
            .ContinueWith(t =>
                {
                    var response = t.Result;
                    if (response.StatusCode == HttpStatusCode.NotModified)
                    {
                        var cachedResponse = request.CreateResponse(HttpStatusCode.OK);
                        cachedResponse.Content = new StringContent("Just for display purposes");
                        return cachedResponse;
                    }
                    else
                    return response;
                });
    }   
}

There isn't really a lot happening. This delegating handler sniffs the incoming responses and if it intercepts a 304 response, it replaces this with a cached response - of course the implementation is a minimal one just for display purposes.

Now let's use it with a resource which returns 304:

class Program
{
    static void Main(string[] args)
    {
        var client = new HttpClient(new InnocentLookingDelegatingHandler()
                        {
                            InnerHandler = new HttpClientHandler()
                        });
        for (int i = 0; i < 1000 * 1000; i++)
        {
            var request = new HttpRequestMessage(HttpMethod.Get, 
                "http://ajax.googleapis.com/ajax/libs/angularjs/1.0.7/angular.min.js");
            request.Headers.IfModifiedSince = DateTimeOffset.Now;
            var response = client.SendAsync(request).Result;
            response.Dispose();
            Console.Write("\r" + i);
        }
            
    }
}

You can find a ready project to test this on GitHub. Please note that System.Net's maxConnection needs to be set up in the app.config.

So what do we see?

1. Application starts to leak memory. 
2. After a while you get an error telling you have run out of sockets.

We can see the effect using SciTech tool:

Well, reason is the server response in the Task continuation is not getting consumed. What does it mean? Well, adding a line to consume the content is enough:

var cachedResponse = request.CreateResponse(HttpStatusCode.OK);
cachedResponse.Content = new StringContent("Just for display purposes");
response.Content.ReadAsByteArrayAsync().Result; // just read the content which is empty!!
return cachedResponse;

Or simply dispose the response:

var cachedResponse = request.CreateResponse(HttpStatusCode.OK);
cachedResponse.Content = new StringContent("Just for display purposes");
response.Dispose(); // Dispose the response
return cachedResponse;

This should be enough to solve the problem. Now one would think that we have got the response and 304 response actually would never have a content so it is silly to read the content - but well, until you hit this bug.

Thanks to Carl Duguay who reported the issue on CacheCow, the resultant memory leak on 304 responses is fixed now. I think it is very likely that you might run into similar problem so beware when getting a response - always consume or dispose it.

This brings the question that should we always dispose HttpRequestMessage and HttpResponseMessage? Recently, there has many classes that implement IDisposable but they do not require use of dispose pattern. Examples include HttpClient, Task or MemoryStream. On the other hand, you cannot find a single sample that either request or response is used in a Dispose pattern - and the fact that the API is fully async makes use of Dispose pattern very difficult.

In any case, ideas are welcome. But as for the bug we had in CacheCow, it is fixed now.