Runtime: 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):

```c#
public class CachingRequestsHandler : DelegatingHandler
{
public CachingRequestsHandler()
{
this.Requests = new List();
}

    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):

```c#
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):

```c#
private async Task RequestAsync1(HttpMethod httpMethod, Uri url, HttpContent content, Func 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.

[karelz]

How is HttpContent typically created? Could it be created twice? (retry should be pretty rare after all)

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

Agreed. However, the API exists and behaves as it does, so we have to keep compat in mind as well.

[name1ess0ne]

How is HttpContent typically created?
Could it be created twice?

Does it matter for current problem? External code owns the content and it is external codes responsibility to create/dispose content. Right?

retry should be pretty rare after all

It is not true, you always should remember about retry when you work with remote API.
Also if you work with OAUTH service your request can be declined with 401 response. In this case OAuth token should be refreshed and request should be retried. It is in OAuth specification. How developer can retry request in this case?

[zabulus]

Agreed. However, the API exists and behaves as it does, so we have to keep compat in mind as well.

Maybe it should declared as deprecated, and/or new API is proposed in this case. Something like leaveOpen flag used System.IO streams.

[karelz]

@name1ess0ne

Does it matter for current problem? External code owns the content and it is external codes responsibility to create/dispose content. Right?

It matters, because it helps us understand the impact of current situation. Changing APIs (even bad APIs as you point out) has impact. Adding new APIs also has impact.
We're not purists to change APIs just because they use bad patterns. What matters is impact on developers. And the decisions are always combo of benefit/cost/risk.

It is not true, you always should remember about retry when you work with remote API.

I was not suggesting that retry should not be present. I tried to say that executing the retry code path is rare -- which means that consuming more CPU or memory in such case is ok-ish (not ideal, but also not super-high impact on the app from practical point of view).

@zabulus

Maybe it should declared as deprecated, and/or new API is proposed in this case. Something like leaveOpen flag used System.IO streams.

New API might be an option. If you guys have proposal which does not confuse developers and if there is enough demand for such behavior/API and if it is feasible to implement, we can consider it. (please do not take it as a promise - we will need to consult with area experts)

[aensidhe]

@karelz can I try to submit a PR with proposal? I'm asking here because I should not according guidelines.

[karelz]

@aensidhe don't submit a PR for adding new API - please follow API process.
Key part of API proposal should be deeper analysis of the workaround - justifying the need for new API (e.g. links to StackOverflow showing lots of people look up the workaround or that it is not easy to do always).

[aamirmahmood28]

I stumbled upon this thread because I just went through same problem. And I was scratching my head about what's going on.

I don't understand how can people justify and defend this design. It is fundamentally flawed and against the principal of "don't dispose if you don't own". No matter how may idiots were convenienced with flawed design.

Now the mistake has been made and released, and we don't want to break existing code. Fine.
Release a new class that does not dispose content and let the owner control the lifecycle of their object.

I don't have to explain what use case I have other than testing. But I will just mention it here so people can think out of their small brains. I need to post the same stream to multiple web api's, and in my case I don't even own the stream. But I want to reuse the stream that I have in my hand and send it to multiple web api's. And (of course) I don't want to create copies of my stream.

[davidsh]

@aamirmahmood28
As of PR dotnet/corefx#19082, HttpClient no longer disposes the HttpContent request object. This change was made for .NET Core.

[Gladskih]

So we have opposite behaviour of the same code (or Standard library) in Framework and Core!

[karelz]

@Gladskih you can't maintain 100% compatibility while also fixing bugs and fixing design flaws (like this one). Compatibility and innovation are ancient rivals.