Content is null when ApiResponse followed by EnsureSucessStatusCodeAsync()

Hi,

I have noticed an issue with refit when getting a response wrapped in a refit ApiResponse which them calls EnsureSuccessStatusCodeAsync()

The ApiException thrown by the EnsureSuccessStatusCodeAsync() has null content even when content exists in the httpResponseMessage.

This sort of code:

Task<ApiResponse<InvestmentsResponseDto>> InsertInvestments(
......

// ApiResponse<t>
var investmentsResponse = await this.investmentsHttp.InsertInvestments(
                                                  commandContext.Command.PrivateQuoteId,
                                                  request,
                                                  InvestmentConstants.MediaTypes.InvestmentAccountStrategyV100,
                                                  etagValue);

await investmentsResponse.EnsureSuccessStatusCodeAsync();

The issue seems to be caused because ApiException disposes of the response content

try
            {
                exception.ContentHeaders = response.Content.Headers;
                exception.Content = await response.Content.ReadAsStringAsync().ConfigureAwait(false);
                response.Content.Dispose();
            }

In RequestBuilderImplementation, on error for an ApiResponse response, it created an ApiException which is passed in to the ApiResponse

var exception = await ApiException.Create(rq, restMethod.HttpMethod, resp, restMethod.RefitSettings).ConfigureAwait(false);

                        if (isApiResponse)
                        {
                            return ApiResponse.Create<T>(resp, default(T), exception);
                        }

The call to EnsureSuccessStatusCodeAsync() then creates another ApiException (if not successful). However because the response content has been disposed by the previous ApiException creation, the content is now null.

There seems to be a couple of ways of resolving this.

Remove the dispose calls from ApiException (and ApiResponse?)
Change EnsureSucessStatusCodeAsync to use the exception passed in via the contructor (if it exists). This sort of code:

if (!IsSuccessStatusCode)
            {
                if (this.Error == null)
                {
                    this.Error = await ApiException.Create(response.RequestMessage, response.RequestMessage.Method, response).ConfigureAwait(false);
                }

                Dispose();

                throw this.Error;
            }

            return this;

I am happy to make the changes but will be guided by your suggestions for the best way.

Many thanks.

Core 2.1

Refit 4.6.30

Visual Studio 2017 v4.7.03056

Windows 10

Issue Analytics

State:
Created 5 years ago
Reactions:1
Comments:23 (7 by maintainers)

Top GitHub Comments

2reactions

lukepuplettcommented, Oct 12, 2018

Ah. Well that’s where the code conspires with the other code to create the bug. See the comment by the OP.

However because the response content has been disposed by the previous ApiException creation, the content is now null.

The root cause is the self-disposal. I think its really odd to self-dispose. To my mind, the Dispose pattern is there for consumers.

If you implement IDisposable, you’re handing the disposal control over to the API consumer.

Else if you’re going to encapsulate clearing down nicely, then don’t implement IDisposable and don’t expose the Dispose method.

At present you have this situation where you’re saying “hey I implement IDisposable, you might wanna dispose of me… err, unless you call this method in which case I secretly dispose for you.”

Another way to think of it is that classes should throw ObjectDisposedException if touched after disposal. But if you did this, then this code would break.

try
{
   await apiResponse.EnsureSuccessStatusCodeAsync();
   return "Everything's good, man.";
}
catch (ApiException apiex)
{
    return "Oh no, I got a " + apiResponse.ReasonPhrase; // ObjectDisposedException
}

Does that make sense?

I’m not stubborn, I could be mad. I’d love someone else’s opinion.

1reaction

donnytiancommented, Sep 27, 2021

v6 has the same error, Luke is right, Refit should not dispose the response content as a consumer. Can we re-open this issue? To fix this issue , we can just add a simple if check or remove the Dispose() call.

public async Task<ApiResponse<T>> EnsureSuccessStatusCodeAsync()
{
    if (!IsSuccessStatusCode)
    {
        // Add this "if" is a simple fix
        if (Error is not null)
        {
            throw Error;
        }
        var exception = await ApiException.Create(response.RequestMessage!, response.RequestMessage!.Method, response, Settings).ConfigureAwait(false);

        Dispose();

        throw exception;
    }

    return this;
}`