Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Content is null when ApiResponse followed by EnsureSucessStatusCodeAsync() #558

Open
Dilsy99 opened this issue Oct 1, 2018 · 23 comments
Open

Comments

@Dilsy99
Copy link

Dilsy99 commented Oct 1, 2018

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

@clairernovotny
Copy link
Member

Can you try the latest version from the MyGet feed? There is now an Error property on the ApiResponse that contains the ApiException. From there, you can get the string content or have it deserialize to a strong type.

@Dilsy99
Copy link
Author

Dilsy99 commented Oct 1, 2018

Will do

@Dilsy99
Copy link
Author

Dilsy99 commented Oct 1, 2018

Sorry, what is the url for the refit MyGet feed?

Search at https://docs.myget.org for refit but could not find anything.

Apologies if a dumb question but never used MyGet before!

MyGet Docs
Documentation - Hosting your NuGet, Npm, Bower, Maven and Vsix packages

@clairernovotny
Copy link
Member

Sorry, I thought it was in the readme...but it's not. I'll file an issue for that.

The myget feed is here: https://www.myget.org/F/refit/api/v3/index.json

@lukepuplett
Copy link

lukepuplett commented Oct 12, 2018

Hey Oren,

Super little library!! Well done.

I was looking at the code for this and it does appear that Refit calls dispose on objects it didn't create so I think that's the root cause.

Could you consider whether you think this is the best design?

It's quite well-known not to dispose of objects you didn't expressly create, else you're inviting trouble.

https://blogs.msdn.microsoft.com/pfxteam/2009/06/24/dont-dispose-of-objects-that-you-dont-own/

https://stackoverflow.com/questions/4085939/who-should-call-dispose-on-idisposable-objects-when-passed-into-another-object

Moreover, from what I can tell, if the response is successful then the "request stack" isn't disposed, its left to the caller to have wrapped the call in a using or explicitly call Dispose on the ApiResponse<T> instance.

If this is the case, my analysis could be wrong, but why worry about disposal if there's an unsuccessful status code but not if successful?

My thinking is that the request still needs to live on for as long as needed to pull further information "off the wire" and potentially drain the bytes from the server. We must remember, afterall, a successful HTTP/TCP conversation took place, only that there was a number >= 400 in some message text.

Cheers!

All about Async/Await, System.Threading.Tasks, System.Collections.Concurrent, System.Linq, and more…
Stack Overflow
Is there any guidance or best practices around who should call Dispose() on disposable objects when they have been passed into another object's methods or constuctor?

Here's a couple of examples a...

@clairernovotny
Copy link
Member

Hi,

I'm not sure where you see that we are disposing of objects we didn't create? We create and own the lifetime of the HttpClient pipeline except in a few narrow cases -- returning HttpResponseMessage or Stream types. As ApiResponse is disposable, I would expect a caller to always dispose it.

@lukepuplett
Copy link

Hey thanks for prompt reply

EnsureSuccessStatusCodeAsync

It disposes of itself rather than letting the consumer of the class do it. It's like it's saying, well, I've thrown an exception I better kill myself, but that doesn't make sense to me, the decision to clear down is upon the holder of that ApiResponse instance.

@clairernovotny
Copy link
Member

I'm not completely sure why that is there without digging further, but in that code path, there's nothing more you can do with the response. The ApiException reads the response content as a string and makes it available either as strongly typed property or as a string.

I'm not sure what behavior you're seeing that's not working as expected?

@lukepuplett
Copy link

"there's nothing more you can do with the response"

So we need to pull out the error detail from the payload but its disposed. Do you see? There is potentially an entire content stream to suck down the pipe once we have an exception :)

@lukepuplett
Copy link

Thanks for you time, btw. We were discussing at work what I nightmare it must be to have a really successful OSS project.

@clairernovotny
Copy link
Member

In that error case, the error stream is already read in as the content of the ApiException, that's why there's nothing further to read?

@lukepuplett
Copy link

lukepuplett commented 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.

@lukepuplett
Copy link

Shall we reopen this and see if we can get more eyes on it?

@clairernovotny
Copy link
Member

Re-opening as that doesn't look right. We probably need a few more tests around EnsureSuccessStatusCodeAsync.

That said, this isn't about disposable. Refit is designed as primarily a wrapper around the underlying httpclient types; the consumer should not be worrying about them.

@dev-thinks
Copy link

@lukepuplett, Is there any workaround you are doing for this, facing the same case as you.

@dev-thinks
Copy link

This works for me though.

if (apiResponse.Error != null) { throw apiResponse.Error; } var response = apiResponse.Content; return response;

@lukepuplett
Copy link

@acclaimuser Unfortunately I'm no longer with that team, so I can't look-up any workaround code cc @Dilsy99

@natelaff
Copy link

Just ran into this myself

@clairernovotny
Copy link
Member

Closing due to age. Please try Refit v6 and reopen if still an issue.

@donnytian
Copy link

donnytian commented 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;
}`

@donnytian
Copy link

#1189 is also related to this issue.

@ZackStone
Copy link

Hi everyone!

I'm experiencing something that I think is related to this issue.

When I call an API using Refit that responds a non-200 status code, and call EnsureSuccessStatusCodeAync method, the ApiException thrown doesn't have anything in the content (ApiException.Content).

image

string responseContent = null;
string responseContentFromApiException = null;

try
{
    var apiResponse = await ... ; // RefitCall

    responseContent = apiResponse.Error?.Content; // has value

    await apiResponse.EnsureSuccessStatusCodeAsync();
}
catch (ApiException ex)
{
    responseContentFromApiException = ex.Content; // is null
}

Windows 10
Refit 5.2.1 and 6.3.2 (tried in both versions)
Visual Studio 17.3.3
.Net Core 6

@dvirsegal
Copy link

The issue still happens on Refit 6.3.2

Indeed the solution is to check if Error is null.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants