Change EnsureSuccessStatusCode to not dispose response content

[thomaslevesque]

Currently the HttpResponseMessage.EnsureSuccessStatusCode method always disposes the content of the response. The reason is explained in a comment:

// Disposing the content should help users: If users call EnsureSuccessStatusCode(), an exception is
// thrown if the response status code is != 2xx. I.e. the behavior is similar to a failed request (e.g.
// connection failure). Users don't expect to dispose the content in this case: If an exception is
// thrown, the object is responsible fore cleaning up its state.

However, this isn't always helpful. It's frequent for web APIs to return details about the error in the response body. If I want to map the error to a specific domain exception, I might write something along these lines:

private async Task<T> GetResponseAsync<T>(HttpRequestMessage request)
{
    using (var response = await _client.SendAsync(request))
    {
        try
        {
            response.EnsureSuccessStatusCode();
            return await response.Content.ReadAsAsync<T>();
        }
        catch (HttpRequestException ex)
        {
            var errorDetails = await response.Content.ReadAsAsync<ErrorDetails>();
            throw CreateDomainExceptionFromDetails(errorDetails, ex);
        }
    }
}

private Exception CreateDomainExceptionFromDetails(ErrorDetails errorDetails, Exception innerException)
{
    // Some logic based on the error details
    // ...
}

However, this doesn't work, because when I try to read the content in the catch block, the content is already disposed. So I need to read the content in the try block instead, before the call to EnsureSuccessStatusCode:

private async Task<T> GetResponseAsync<T>(HttpRequestMessage request)
{
    using (var response = await _client.SendAsync(request))
    {
        string content = null;
        try
        {
            content = await response.Content.ReadAsStringAsync();
            response.EnsureSuccessStatusCode();
            return JsonConvert.DeserializeObject<T>(content);
        }
        catch (HttpRequestException ex) when (content != null)
        {
            var errorDetails = JsonConvert.DeserializeObject<ErrorDetails>(content);
            throw CreateDomainExceptionFromDetails(errorDetails, ex);
        }
    }
}

It's less intuitive and makes the code harder to read

An EnsureSuccessStatusCode(bool disposeContent) overload would be helpful in this case. It would let the user state that they're taking responsibility for disposing the content (in the exemple above, the content would be disposed when the response is disposed at the end of the using statement).

(I'm aware that I could just check the IsSuccessStatusCode property, but I want the "normal" HttpRequestException as the inner exception of my domain exception)

I'm willing to submit a pull request if the proposal is approved.

[davidsh]

cc; @stephentoub @karelz

We should make this change. It only made sense for EnsureSuccessStatusCode to dispose the request content when HttpClient was doing it while sending requests.

However, we changed that behavior in PR dotnet/corefx#19082

So, to be consistent, we need to fix EnsureSuccessStatusCode() as well. And by "fix" this, I mean that we should just change the behavior of the EnsureSuccessStatusCode API. We don't need to add a new overload.

[stephentoub]

We should make this change. It only made sense for EnsureSuccessStatusCode to dispose the request content when HttpClient was doing it while sending requests.

I'm fine with changing EnsureSuccessStatusCode, but isn't it disposing of the response content, not the request content?

[davidsh]

Ugh...you're right. I read this wrong. I thought it was referring to request content. :(

But in reviewing, we probably should still change this here to follow the same principles we did in #21394 and not proactively dispose something.

I would still be in favor of not doing an explicit Dispose() here of the response body content. It would get disposed later anyways when the enclosing HttpResponseMessage gets disposed.

[stephentoub]

SGTM

[karlzf]

I do not see the fix applied to .NET Core 2.2 release. Can anyone confirm? Thanks. https://github.com/dotnet/corefx/blob/release/2.2/src/System.Net.Http/src/System/Net/Http/HttpResponseMessage.cs

[nbarbettini]

@karlzf Confirmed, .NET Core 2.2 still disposes the Content property.

For some reason the activity log above doesn't show that it was re-assigned to the 3.0 milestone, but the "Milestone" field at the top of the page shows 3.0.

[davidsh]

For some reason the activity log above doesn't show that it was re-assigned to the 3.0 milestone, but the "Milestone" field at the top of the page shows 3.0.

This fix is only in the .NET Core 3.0 branch.