Flurl: HttpResponseMessages are not disposed

[cremor]

Summary

Right now HttpResponseMessage objects are never disposed by Flurl. I'd like to propose those changes:

1. All methods in HttpResponseMessageExtensions except ReceiveStream and all methods in DownloadExtensions should dispose the HttpResponseMessage that they use.
2. Maybe improve the disposing situation of ReceiveStream and the exception case if an improvement is possible. Otherwise document that the caller needs to dispose the Stream and the Response of exceptions.
3. New extension methods should be created directly on HttpResponseMessage (instead of Task<HttpResponseMessage> like they exist now). Those should not dispose the HttpResponseMessage that they use.

Detailed explanation

1. Main problem

Unless I'm missing something here, it seems like all of the convenient methods like GetJsonAsync, GetJsonListAsync, GetStringAsync, GetBytesAsync and DownloadFileAsync do not dispose the HttpResponseMessage. And since they never expose the HttpResponseMessage the caller can't dispose it either. This should be an easy fix.

And since GetXyz only calls ReceiveXyz a change in ReceiveXyz would fix it for code like url.PostJsonAsync(data).ReceiveJson() too.

2. Special cases

Of course GetStreamAsync should not dispose the HttpResponseMessage, otherwise the stream is disposed too. I don't know if disposing the Stream is enough or if the HttpResponseMessage should be disposed too. But I can't think of an good solution right now if the message itself should also be disposed.

If an exception is thrown by the FlurlRequest the HttpResponseMessage isn't disposed either. But since you provide access to it via FlurlHttpException I can't think of a good place to dispose it. Maybe document that the caller should call ex.Call?.Response?.Dispose()?

3. API improvement

There is also a problem with all Flurl.Http methods that return a HttpResponseMessage directly. In theory the caller would be responsible to dispose those. But the current design of the HttpResponseMessageExtensions class makes this harder than it should be. Since those extensions are on Task<HttpResponseMessage> instead of just HttpResponseMessage you can't simply write something like this:

using (HttpResponseMessage responseMessage = await request.PostJsonAsync(data)) {
    var response = await responseMessage.ReceiveJson<Type>(); // Error: 'ReceiveJson' not available here.
    // Not shown: Do something custom with responseMessage
}

The best solution I could find right now is this:

Task<HttpResponseMessage> responseTask = request.PostJsonAsync(data);
using (HttpResponseMessage responseMessage = await responseTask) {
    var response = await responseTask.ReceiveJson<Type>();
    // Not shown: Do something custom with responseMessage
}

That works, but is not very nice. Also, if the ReceiveXyz methods are changed like suggested in the "Main problem" section this would have a weird effect: The HttpResponseMessage would be disposed after ReceiveXyz, although the using block was not left yet.

Therefore I suggest extension methods directly on HttpResponseMessage that do not dispose the HttpResponseMessage. With those, the caller would have a nice API and full control over the dispose time of the HttpResponseMessage.

Btw, I don't know if the methods currently existing in HttpResponseMessageExtensions don't have an Async suffix by design because they are extensions on Task, but for the new ones directly on HttpResponseMessage I would definitely include the Async suffix.

[tmenier]

Keep in mind that the only thing that HttpResponseMessage.Dispose does is call Dispose on its Content property, and that in turn calls Dispose on whatever disposable resources it might be holding. In short, to the best of my knowledge, the only thing you need to be concerned about disposing here is Stream content.

The ReceiveJson implementation deserializes directly from a stream (rather than buffering to a string first) for better memory efficiency. And it gets the stream in a using statement, so it gets disposed properly.

ReceiveStream just returns the stream, and the onus is on the caller dispose it when they're done with it. This is a very common and well understood pattern, and I don't think Flurl needs to do anything here.

Given that HttpResponseMessage holds no other disposable resource other than the underlying HttpContent, I think the API changes you're suggesting are moot.

[cremor]

While it might be true that disposing the HttpResponseMessage is rarely needed for the current implementation, should a library really rely on it? Especially one that targets .NET Standard and therefore runs on multiple runtimes. I'd say unless it is documented that calling Dispose is not needed (like it is for Task and MemoryStream), the dispose pattern should be honored. At least that's my opinion.

I've also found this very recent commit in .NET Core that says the following:

The developer pattern we advise when getting an HttpResponseMessage is to wrap it in a 'using' statement or use an explicit .Dispose() call.

[tmenier]

While I don't anticipate a future where anything other than the contained stream would need to be disposed, I do agree that it can't hurt to follow the correct pattern. I experimented with this and found cases where FlurlHttpException.GetResponseStringAsync and GetResponseJsonAsync broke, so the fix was a little more involved than anticipated. I have it working but going to push it out to the 2.4 release, which will have a prerelease so hopefully others can help test it.

[tmenier]

@cremor Feel free to review that commit if you want, it should now dispose HttpResponseMessage everywhere except when returning a stream. I'll get this out in a 2.4 prerelease fairly soon so it can be tested.

[tmenier]

By the way, I've heard the suggestion for ReceiveXXX extensions on HttpResponseMessage from others as well, so I created a new issue for that: #354

I agree about the naming, the new methods will at least have an Async suffix, and will probably be entirely different. (You've already "received" the response.) I'll post those details in that issue once I settle on the specifics.

[cremor]

@tmenier Great to hear, thanks! I've looked over the commit and the disposing seems good. I'll test it with the prerelease when I find some time.

But I didn't get the use of the new FlurlHttpException constructor overload since it's never actually called. Or is this not finished yet?

[tmenier]

It's called indirectly by the FlurlParsingException sub-type here.

I Could have made the constructor protected but I don't think that's important. Could be some future case where it's useful.

[arieradle]

Guys, any updates on this?

Handling requests and responses with a custom MediaType formatter is a very important feature.

[tmenier]

@arieradle Are you referring to #349? No updates at this time.

[tmenier]

@cremor Proper disposing of HttpResponseMessage is now out in prerelease. Please test and let me know your findings. Thanks.

https://www.nuget.org/packages/Flurl.Http/2.4.0-pre

[cremor]

Looks good to me, thanks.

The only remaining place that I found that requires manual disposal that isn't obvious (like ReceiveStream) is when a call returns a fail HTTP code and therefore Flurl throws a FlurlHttpException before ReceiveJson or a similar method is even called. For that I still have to call exception.Call?.Response?.Dispose(); But I can't think of any way how Flurl could help here, so that's ok.

Point 3 of my initial message in this issue is still open, right? Do you plan to do anything for it? If not, I'll just stay at the workaround that I've shown.

[tmenier]

@cremor Thanks for taking a look. I noticed about those exception cases too, and I agree. I think the alternative is to materialize the stream eagerly, even if the user doesn't need it, which is what it used to do before awaiting in a catch block was allowed. I still think the risk associated with not disposing there is low enough that it's not worth the performance hit.

I moved your point 3 to #354. I still plan on doing it, just haven't gotten to it yet. I plan to do the "official" 2.4 release this early this week, probably with no changes.

[tmenier]

2.4 is in full release.