When or if to Dispose HttpResponseMessage when calling ReadAsStreamAsync?

Question

I am using the System.Net.Http.HttpClient to do some client-side HTTP communication. I've got all of the HTTP in one spot, abstracted away from the rest of the code. In one instance I want to read the response content as a stream, but the consumer of the stream is well insulated from where the HTTP communication happens and the stream is opened. In the spot responsible for HTTP communication I am disposing of all of the HttpClient stuff.

This unit test will fail at Assert.IsTrue(stream.CanRead):

[TestMethod]
public async Task DebugStreamedContent()
{
    Stream stream = null; // in real life the consumer of the stream is far away 
    var client = new HttpClient();        
    client.BaseAddress = new Uri("https://www.google.com/", UriKind.Absolute);

    using (var request = new HttpRequestMessage(HttpMethod.Get, "/"))
    using (var response = await client.SendAsync(request))
    {
        response.EnsureSuccessStatusCode();
        //here I would return the stream to the caller
        stream = await response.Content.ReadAsStreamAsync();
    }

    Assert.IsTrue(stream.CanRead); // FAIL if response is disposed so is the stream
}

I typically try to dispose of anything IDisposable at the earliest possible convenience but in this case, disposing the HttpResponseMessage also disposes the Stream returned from ReadAsStreamAsync.

So it seems like the calling code needs to know about and take ownership of the response message as well as the stream, or I leave the response message undisposed and let the finalizer deal with it. Neither option feels right.

This answer talks about not disposing the HttpClient. How about the HttpRequestMessage and/or HttpResponseMessage?

Am I missing something? I am hoping to keep the consuming code ignorant of HTTP but leaving all these undisposed objects around goes against year of habit!

Verger

Just a tip - Not everything IDisposable needs to be disposed – Incommensurable

This doesn't seem to have anything to do with async per se. The rules are the same either way: don't dispose the object until you're done with it. The same thing would apply using the synchronous version. So, use the returned Stream inside the using. If you need to use the Stream outside the context where the request is created, you'll have to set up a different mechanism to dispose of it at the right time. – Skimmer

Also: I don't recommend leaving disposal for the finalizer...but I note that you don't bother to dispose client anyway, so if you're comfortable with that, don't bother with the other stuff either. As for the answer you reference, note that it pertains to the scenario where you reuse the HttpClient object; frankly, it should be obvious that if you want to reuse it, you don't dispose it. The guidance doesn't say anything at all about whether it's legitimate to just let the object disposal happen via finalization (and IMHO it's very poor form to). – Skimmer

Answer 1

So it seems like the calling code needs to know about and take ownership of the response message as well as the stream, or I leave the response message undisposed and let the finalizer deal with it. Neither option feels right.

In this specific case, there are no finalizers. Neither HttpResponseMessage or HttpRequestMessage implement a finalizer (and that's a good thing!). If you don't dispose of either of them, they will get garbage collected once the GC kicks in, and the handle to their underlying streams will be collected once that happens.

As long as you're using these objects, don't dispose. Once done, dispose of them. Instead of wrapping them in a using statement, you can always explicitly call Dispose once you're done. Either way the consuming code doesn't need to have any knowledge underlying http requests.

Rising

That's not a good thing at all! Leaving un-disposed HttpMessageRequest & HttpMessageResponse objects means that they will only be disposed by GC (whenever it may decided to run) and until then all the resources used for the call will be freed. This leads to port exhaustion, timeouts if there is a limit on concurrent HTTP requests, etc... – Axil

@AlonCatz Where in my answer did I say not to dispose the objects? Did you read the second paragraph? – Rising

@AlonCatz - The GC never calls .Dispose() in its own right. It's only if there is a finalizer that calls .Dispose() that the GC can trigger a dispose. – Lauricelaurie

@Yuval, perhaps I misread it, but the impression I got was that it's OK not to dispose those objects because GC will take care of them. In my experience, it's very important to dispose them explicitly. – Axil

@Enigmativity, of course the finalizer calls Dispose() and only the objects which hold the actual native resources (a socket in our case) will have the finalizer. However, it doesn't matter, because when you call Dispose() on the HttpResponseMessage, it will Dispose all the objects it references including the one with the finalizers. – Axil

@AlonCatz - It's not at all guaranteed that the finalizer calls .Dispose() - this will only happen if the finalizer is explicitly written to do so. – Lauricelaurie

This HttpClient is absymal and falls over in cases where RestSharp has no problems. HttpClient... when I call ReadAsStringAsync... if I close the response but don't close the Content, it has tons of different socket errors occuring under high load. If I close the Content explicitly, the errors go away, but I still get "Service Unvailable" responses. Bottom line is, you have to close BOTH the Content and the Response. Disposing the Response alone is not enough. This is nuts. What is RestSharp doing better? HttpClient also uses 8k connections where RestSharp self-limits to around 4k. – Memnon

@YuvalItzchakov Reading your answer, it's not clear to me what's the solution to ensure the response is disposed when the underlying content stream is disposed. For example, I have a scenario in which my method accepts a Stream factory (Func<Stream>). It's supposed to cover any scenarios, because the caller is supposed to be in control of the lifetime. But then you discover HttpResponseMessage , that is itself disponsable and your design blows up... :( – Burson

This is misleading -- if the messages themselves don't have finalizers, but contain streams with handles, that do have finalizers, then it's functionally the same thing. Sigh. With HttpClient it's always a barrel of laughs, isn't it? – R

Answer 2

Dealing with Disposables in .NET is both easy, and hard. For sure.

Streams pull this same nonsense... Does disposing the buffer also automatically dispose the Stream it wrapped? Should it? As a consumer, should I even know whether it does?

When I deal with this stuff I go by some rules:

1. That if I think there are non-native resources at play (like, a network connection!), I don't ever let the GC "get around to it". Resource exhaustion is real, and good code deals with it.
2. If a Disposable takes a Disposable as a parameter, there is never harm in covering my butt and making sure that my code disposes of every object it makes. If my code didn't make it, I can ignore it.
3. GCs call ~Finalize, but nothing ever guarantees that Finalize (i.e. your custom destructor) invokes Dispose. There is no magic, contrary to opinions above, so you have to be responsible for it.

So, you've got an HttpClient, an HttpRequestMessage, and an HttpResponseMessage. The lifetimes of each of them, and any Disposable they make, must be respected. Therefore, your Stream should never be expected to survive outside of the Disposable lifetime of HttpResponseMessage, because you didn't instantiate the Stream.

In your above scenario, my pattern would be to pretend that getting that Stream was really just in a Static.DoGet(uri) method, and the Stream you return would HAVE to be one of our own making. That means a second Stream, with the HttpResponseMessage's stream .CopyTo'd my new Stream (routing through a FileStream or a MemoryStream or whatever best fits your situation)... or something similar, because:

• You have no right to the lifetime of HttpResponseMessage's Stream. That's his, not yours. :)
• Holding up the lifetime of a disposable like HttpClient, while you crunch the contents of that returned stream, is a crazy blocker. That'd be like holding onto a SqlConnection while you parse a DataTable (imagine how quickly we'd starve a connection pool if DataTables got huge)
• Exposing the how of getting that response may work against SOLID... You had a Stream, which is disposable, but it came from an HttpResponseMessage, which is disposable, but that only happened because we used HttpClient and HttpRequestMessage, which are disposable... and all you wanted was a stream from a URI. How confusing do those responsibilities feel?
• Networks are still the slowest lanes in computer systems. Holding them up for "optimization" is still insane. There are always better ways to handle the slowest components.

So use disposables like catch-and-release... make them, snag the results for yourself, release them as quickly as possible. And don't confuse optimization for correctness, especially from classes that you did not yourself author.

Triform

Wouldn't CopyTo enumerate the stream? So instead of holding up HttpResponseMessage, I'm now loading the full response in memory and creating a new stream from that? – Filberte

Answer 3

You can also take stream as input parameter, so the caller has complete control over type of the stream as well as its disposal. And now you can also dispose httpResponse before control leaves the method.
Below is the extension method for HttpClient

    public static async Task HttpDownloadStreamAsync(this HttpClient httpClient, string url, Stream output)
    {
        using (var httpResponse = await httpClient.GetAsync(url).ConfigureAwait(false))
        {
            // Ensures OK status
            response.EnsureSuccessStatusCode();

            // Get response stream
            var result = await httpResponse.Content.ReadAsStreamAsync().ConfigureAwait(false);

            await result.CopyToAsync(output).ConfigureAwait(false);
            output.Seek(0L, SeekOrigin.Begin);                
        }
    }

Evanston

The problem with this approach is that it doesn't behave well with functional / reactive programming. – Burson

Answer 4

After thinking about it for hours, I've come to the conclusion that this approach is the best:

An Adapter that takes the HttpRequestMessage and its content stream as dependencies.

Here it is. Pay close attention to it's static factory method Create. The constructor is private for obvious reasons.

public class HttpResponseMessageStream : Stream
{
    private readonly HttpResponseMessage response;

    private readonly Stream inner;

    private HttpResponseMessageStream(Stream stream, HttpResponseMessage response)
    {
        inner = stream;
        this.response = response;
    }

    public override bool CanRead => inner.CanRead;

    public override bool CanSeek => inner.CanSeek;

    public override bool CanWrite => inner.CanWrite;

    public override long Length => inner.Length;

    public override long Position
    {
        get => inner.Position;
        set => inner.Position = value;
    }

    public static async Task<HttpResponseMessageStream> Create(HttpResponseMessage response)
    {
        return new HttpResponseMessageStream(await response.Content.ReadAsStreamAsync(), response);
    }

    public override ValueTask DisposeAsync()
    {
        response.Dispose();
        return base.DisposeAsync();
    }

    public override void Flush()
    {
        inner.Flush();
    }

    public override int Read(byte[] buffer, int offset, int count)
    {
        return inner.Read(buffer, offset, count);
    }

    public override long Seek(long offset, SeekOrigin origin)
    {
        return inner.Seek(offset, origin);
    }

    public override void SetLength(long value)
    {
        inner.SetLength(value);
    }

    public override void Write(byte[] buffer, int offset, int count)
    {
        inner.Write(buffer, offset, count);
    }

    protected override void Dispose(bool disposing)
    {
        response.Dispose();
        base.Dispose(disposing);
    }
}

Check this sample usage:

HttpRequestMessage response = // Obtain the message somewhere, like HttpClient.GetAsync()
var wrapperStream = await HttpResponseMessageStream.Create(response);

The important thing is that disposing this wrapper will dispose the response as well, being able to control the lifecycle effectively.

This way you can safely create generic consumers like this method, that don't care about anything about the underlying implementation:

public async Task DoSomething(Func<Task<Stream>> streamFactory) 
{
    using (var stream = await streamFactory())
    {
       ...
    }
}

and use it like this:

async Task<Stream> GetFromUri(Uri uri)
{
    var response = ...
    return await HttpResponseMessageStream.Create(response);
}

await DoSomething(() => GetFromUri("http://...");

The DoSomething method completely ignores the grievances related with the disposal. It just disposes the stream like any other and the disposal is handled internally.

Burson

This should be the answer. This one does not load the entire response body into memory (not to mention duplicated buffers). I thought of a worse solution of returning a 3rd class that wrap both (cleaner and faster to code) but adapter is much better if you cannot modify the API. – Lachrymatory

Why you do not dispose inner stream in dispose method? – Godthaab

@Vlad disposing the HttpResponseMessage will ensure the response stream is also disposed. It's still unclear to me if this is all really needed, or if you just need to dispose the stream. /edit: it's not needed, just dispose the stream and you're golden. github.com/dotnet/runtime/issues/28578#issuecomment-459769784 – Incongruent

Answer 5

Do not dispose the HttpResponseMessage because it's the duty of the party calling this method.

Method:

public async Task<Stream> GetStreamContentOrNullAsync()
{
    // The response will be disposed when the returned content stream is disposed.
    const string url = "https://myservice.com/file.zip";
    var client = new HttpClient(); //better use => var httpClient = _cliHttpClientFactory.CreateClient();
    var response = await client.GetAsync(url, HttpCompletionOption.ResponseHeadersRead);
    if (response.StatusCode == System.Net.HttpStatusCode.NotFound)
    {
        return null;
    }

    return await response.Content.ReadAsStreamAsync();
}

---

Usage:

  public async Task<IActionResult> DownloadPackageAsync()
  {
      var stream = await GetStreamContentOrNullAsync();
      if (stream == null)
      {
            return NotFound();
      }

      return File(stream, "application/octet-stream");
  }

Keane

Your examples are using ReadAsStringAsync() instead of ReadAsStreamAsync(), which is what the question asks about. – Shivery

@Shivery you're right! I updated my code sample – Keane

You say that the response will be disposed when the content stream is disposed, but I'd swear this won't happen. Please, review it. – Burson

Answer 6

From the master of all things related to C# performance, Stephen Toub:

Do you mean HttpResponseMessage and HttpContent? If so, it's disposable mainly to dispose of the stream. If you have access to the stream, then you can just dispose it, and it appropriately cleans up after the whole operation, returning the connection to the connection pool, etc. It won't end up calling HttpContent.Dispose, but that would only matter if you had some custom HttpContent-derived type that did something custom in its Dispose method beyond disposing of the response stream.

https://github.com/dotnet/runtime/issues/28578#issuecomment-459769784

So, don't worry about disposing the HttpResponseMessage as long as the Stream is eventually disposed. Only if you use custom HttpContent class with additional things to dispose, maybe it's time for a redesign, so you don't need the custom class.

Incongruent

Answer 7

Speaking for HttpRequestMessage, we can see in HttpClient.cs, if you call PostAsync/etc., it just creates an HttpRequestMessage and sends it off without disposing anything:

R

If you take a closer look... SendAsync disposes the request content. HttpRequestMessage.Dispose only disposes the request content. So it is in effect PostAsync is disposing of the request. – Verger