Why does HttpClient dispose HttpContent?

[aensidhe]

Hi.

I'm trying to write some tests on our custom protocol over http. So, I passed a fake message handler to client, like this (we don't use C#6 yet, so):

    public class CachingRequestsHandler : DelegatingHandler
    {
        public CachingRequestsHandler()
        {
            this.Requests = new List<HttpRequestMessage>();
        }

        public List<HttpRequestMessage> Requests { get; private set; }

        protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
        {
            this.Requests.Add(request);
            return Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK));
        }
    }

When in test I'm trying to inspect request's Content, I'm getting an ObjectDisposedException. As I found HttpClient unconditionally disposes Content.

I have two questions:

• will this behavior change in future to give user some control over Content?
• how should I work around this issue, because I want to assert and test all parts of request: headers and content? Is there a fast and easy way to clone HttpRequestMessage?

[SidharthNabar]

Hi @aensidhe ,

Based on your description, I am guessing the flow of an HTTP request in your case is: client -> your caching Handler -> HttpClientHandler. Am I right? In that case, you should be able to access the HttpContent before it is sent out by the underlying HttpClientHandler. Is that not working? Or is there a reason you need to access the HttpContent after it has already been sent out?

Yes, HttpClient does dispose HttpContent after the request is sent. This is a convenience feature so that the caller of the API doesn't have to worry about disposing it after sending out the request. This behavior is definitely up for discussion though, and we can think of changing it if needed for developer scenarios.

As for cloning HttpRequestMessage - no, that class is not intended to be derived from.

Thanks!
Sid

[aensidhe]

My flow is simplier: HttpClient -> my handler -> back to caller, but general idea is correct. And yes, I refactored code, so I could place asserts in my handler via callback.

I think this is bad behavior (even if it's convenient), because of caller of HttpClient is more aware about lifetime and scope of content object. It would be great if user of HttpClient could control this disposal feature. And it would be ok, if it's enabled by default.

Thanks.

[SidharthNabar]

I am still not clear on my previous question - Are you getting an error trying to access the Content before it is sent out? Is there a requirement/scenario why you are trying to access it after already sending it out?

Thanks
Sid

[aensidhe]

Nope, I got an error after. It is very convenient to write tests on code which uses HttpClient like this (given handler from starting message):

public void SomeTest()
{
    var handler = new CachingHandler();
    var v = new SomeClass(handler);
    // SomeMethod does work and sends request via HTTP using HttpClient and supplied Handler.
    v.SomeMethod();

    Assert.AreEqual(1, handler.Requests.Length);
    // some assertions on handler.Requests[0]:
    // we could assert url, headers, method and content.
}

And, beside of that it may be convenient to reuse Content object in some cases.

And I want to remind: a user created a content object, not a HttpClient. So user should have a possibility to control lifetime of content object and when it should or should not be disposed.

[sharwell]

We created the DelegatingStream class to address this exact problem. Set ownsStream to false in the constructor and it will prevent calls to Dispose from releasing the underlying stream.

[aensidhe]

I think, main issue here is that HttpClient unconditionally dispose objects which it does not own, not that it's impossible to write tests without workarounds.

[darrelmiller]

The fact that HttpContent is disposed makes a number of other scenarios difficult. Implementing a Redirecting message handler is tricky because you need to clone the Request/Content in order to perform the redirect. This is specifically a problem for 307,308 redirects.

It is also harder to write message handlers that do the 401 authorization dance. If a 401 is received, I want to be able to add appropriate credentials to the request and then re-issue the request.

Stopping the HttpContent from being disposed would only partially solve this problem though because there is still the scenario that someone might use a forward only stream. Although I have no idea how the existing HttpWebRequest infrastructure deals with this.

Ideally we need some way of identifying a HttpContent as "re-readable so don't buffer it", "ok to buffer it to enable re-reading" or "don't even try re-reading this".

[ShenZZ]

I have twe problems with this issue:
request retry
partly upload

[karelz]

This would be a breaking change - BTW: the code likely has comments which explain why it was done this way.
If there is strong interest in this new behavior, we would likely need a new API.
Closing for now, feel free to reopen with more info or with scenario which would justify the new API.

[aensidhe]

@karelz at the time of writing there was no explanation, I checked it. Current explanation is

// When a request completes, dispose the request content so the user doesn't have to. This also
// helps ensure that a HttpContent object is only sent once using HttpClient (similar to HttpRequestMessages
// that can also be sent only once).

The explanation is not very good on my opinion, because it is not always true that HttpClient owns Content object.

Maybe I missing something, but adding an ability to control lifetime of Content object will not break existing clients with default settings to "own" content objects.

[karelz]

Speaking very general: Adding multiple ways of doing things is an option, however it typically pollutes the API surface and confuses developers which one to choose when. So there should be a strong reason to do something like that.
Are there other use cases beside testing? Is testing such strong scenario we need to expose this? Are there alternatives?

[name1ess0ne]

There are bunch scenarios where current approach is very harmful and complicated the code.
For example, if you are calling to REST API and using retry policy. If it happens and you need to retry you have to recreate HttpContent from the scratch. It allocates memory, spends CPU and also complicates development. For example it is extremely complicated to create utility method that sends any content to server and retries if needed.
For example code below wouldn't work):

    private async Task<HttpResponseMessage> RequestAsync1(HttpMethod httpMethod, Uri url, HttpContent content, Func<HttpResponseMessage, bool> isTransientFunc)
    {
      while (true)//for demonstration only, I know about RetryPolicy
      {
        var request = new HttpRequestMessage(httpMethod, url);

        if (content != null)
          request.Content = content;

        var response = await _httpClient.SendAsync(request);

        if (!response.IsSuccessStatusCode && isTransientFunc(response))
        {
          continue;//retry
        }

        return response;
      }
    }

BTW it is very bad practice to free resources that you do not own.