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

When you want to keep HTTPClient alive FlurlClient keeps disposing him #66

Closed
Toine-db opened this issue Mar 31, 2016 · 9 comments
Closed

Comments

@Toine-db
Copy link

When creating a custom HttpClientFactory that manages a HttpClient the FlurlClient instances keep disposing the used HTTPClient when collected by the GC.

I already made a solution for it, virtual dispose
#65

@tmenier
Copy link
Owner

tmenier commented Apr 1, 2016

I'd like to understand your use case a little better. Keeping the lifespan of the HttpClient bound to that of the FlurlClient was an intentional design decision; I think it makes it more clear and intuitive to the user. See here for details on patterns for re-using FlurlClient, thus re-using the underlying HttpClient, and let me know if this suits your needs.

@Toine-db
Copy link
Author

I'm working on a multithreaded application, where I want to reuse the HttpClient for things like cookies.
The HttpClients SendAsync method is thread save, but I'm not sure FlurlHttp client is when bluiding the URL.

Different FlurlClients will guarantee thread issues, I think.
But then the HttpClient gets disposed again and again.

My solution would be an easy fix, but only needed when FlurlClient isn't 100% thread safe.

@tmenier
Copy link
Owner

tmenier commented Apr 13, 2016

I see. FlurlClient does hold a reference to a URL, so you're right, you can't safely call different URLs on different threads using the same FlurlClient instance. I'll give some thought to a better solution to that, but in the mean time I will pull your change. Look for a Flurl.Http 0.8.1 soon.

@Toine-db
Copy link
Author

OK, good luck with searching for a better solution.

THanks for looking at the request, and thanks for Flurl solution itself.

@tmenier
Copy link
Owner

tmenier commented Apr 17, 2016

Ok, I believe I have a solution that solves this issue in a different way. Before I package it for NuGet, let me know if you think this will work.

The WithUrl method, which previously simply allowed you to set FlurlClient.Url fluently, now does a little more. It creates and returns a new FlurlClient with the Url set to whatever you passed, but with a shared instance of HttpClient and HttpMessageHandler. This effectively allows you to share those objects with different URLs in a thread-safe way. Your change would be:

  1. Rather than creating a global instance of HttpClient and returning it in a custom HttpClientFactory, just create a global instance of FlurlClient. Set your cookies, etc. on that FlurlClient. (You probably don't need the factory at all.)
  2. Every time you want to make an HTTP call, start by calling WithUrl(url) on the shared FlurlClient.

If you follow this pattern, you should get shared cookies etc, thread-safe URLs, and no unexpected disposal of HttpClient. Let me know you think this will work. I'd be happy to create a prerelease NuGet package if you want to try it out.

@tmenier
Copy link
Owner

tmenier commented Apr 17, 2016

These 2 new tests demonstrate the new functionality:
https://github.com/tmenier/Flurl/blob/master/Test/Flurl.Test.Shared/Http/ClientConfigTests.cs#L130-L159

tmenier added a commit that referenced this issue Apr 21, 2016
…ew FlurlClient instance with shared internals for better thread-safe multi-URL use (#66)
@tmenier
Copy link
Owner

tmenier commented Apr 21, 2016

Flurl.Http 0.9.0-pre is released. I will probably need to put this into full release soon because others are waiting on a bug fix affecting .NET 4.6.1, so if you have feedback please get back to me as soon as you can.

@tmenier tmenier mentioned this issue Apr 21, 2016
@tmenier
Copy link
Owner

tmenier commented Apr 21, 2016

Flurl 0.9.0 is released.

@tmenier tmenier closed this as completed Apr 21, 2016
@Toine-db
Copy link
Author

Good solution, this should work!

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

2 participants