CODEWITHSUNDEEP
c#.netstreamidisposabledotnet-httpclient
dkackman
dkackman

Reputation: 15579

When or if to Dispose HttpResponseMessage when calling ReadAsStreamAsync?

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!

Upvotes: 71

Views: 44925

Answers (7)

Mark Sowul
Mark Sowul

Reputation: 10610

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:

https://github.com/microsoft/referencesource/blob/master/System/net/System/Net/Http/HttpClient.cs#L293-L299

Upvotes: 0

Steven Liekens
Steven Liekens

Reputation: 14133

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.

Upvotes: 0

SuperJMN
SuperJMN

Reputation: 13992

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.

Upvotes: 4

Craig Brunetti
Craig Brunetti

Reputation: 595

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.

Upvotes: 15

Alper Ebicoglu
Alper Ebicoglu

Reputation: 9634

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

Upvotes: 2

LP13
LP13

Reputation: 34149

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

Upvotes: 13

Yuval Itzchakov
Yuval Itzchakov

Reputation: 149598

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.

Upvotes: 18

Related Questions