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

Drop ConnectionLeaseTimeout #703

Closed
tmenier opened this issue Jun 10, 2022 · 9 comments
Closed

Drop ConnectionLeaseTimeout #703

tmenier opened this issue Jun 10, 2022 · 9 comments

Comments

@tmenier
Copy link
Owner

tmenier commented Jun 10, 2022

Flurl.Http 2.0 introduced the ConnectionLeaseTimeout setting for platforms that don't support ServicePoint, in order to deal with this infamous DNS problem. A lot changed since them, most notably the introduction of SocketsHttpHandler in .NET Core 2.1, which more elegantly solved the problem, is far superior to Flurl's solution, and is now the default handler out of the box with HttpClient. Flurl kept its setting around and improved upon it, primarily to cover gaps in platforms that supported neither ServicePoint nor SocketsHttpHandler.

Those platforms that support neither? .NET Core 1.x, which has been out of support for 3 years and isn't usable with Flurl anymore anyway.

So Flurl's ConnectionLeaseTimeout setting will be dropped in the next 4.0 prerelease. I don't expect much protest, but it's possible those on platforms that don't support SocketsHttpHandler (full framework and Core 2.0) might find Flurl's ConnectionLeaseTimeout setting nicer to use than ServicePoint. I'm open to hearing that argument if it's out there, but I just have a hunch not many are using it, and major version upgrades are always a good opportunity to trim some fat.

tmenier added a commit that referenced this issue Jun 10, 2022
@tmenier tmenier changed the title Drop ConnectionLeaseTimeout (open to debate) Drop ConnectionLeaseTimeout Jun 11, 2022
@tmenier
Copy link
Owner Author

tmenier commented Jun 20, 2022

Specific objects from which ConnectionLeaseTimeout is removed as of Flurl.Http 4.0-pre2:

  • FlurlHttp.GlobalSettings
  • IFlurlClient.Settings
  • HttpTest.Settings

@KorsG
Copy link

KorsG commented Jun 27, 2022

Sorry for being a little late here, but we actually use this pretty extensively in alot of projects that needs to multi-target .NET Framework + .NET Standard 2.0 + .NET 5+ (API clients/SDKs mostly), and found that Flurl handles this very conveniently without having to figure out/remember all the gritty details with Servicepoint, SocketHttpHandler and lack of built-in solution for some targets.

So I guess we actually use it for the intended reasons and like the fact that it just works across all targets, since we need to support these "older" platforms for the foreseeable future (maybe not netstandard, but it's going to take along time to get rid of .NET Framework in the enterprise segment).

So I hope you will reconsider and thereby not remove it 🙏😀

@tmenier
Copy link
Owner Author

tmenier commented Jul 8, 2022

@KorsG You won't like this answer but I've only become more convinced that 4.0 is the right time to drop this. Understand that Flurl's ConnectionLeaseTimeout completely reinvents the wheel in solving this, and for those on a platform supporting SocketsHttpHandler, it doesn't replace the default behavior - it supplements it, meaning the same problem is being solved in 2 different ways at the same time. That may not create significant overhead from a practical perspective, but it's definitely not ideal. And I have to think we're past the tipping point where most new .NET code is running on one of the newer platforms.

I don't like to force anyone to make the same decisions with their libraries that I've made with mine, but have you considered taking a similar position? That is, upgrade Flurl in the next major version(s) of your libraries and recommend that your users still on full framework either not upgrade or add the necessary ServicePoint one-liner to their solutions? Or, you could even add it to your library with the appropriate #if directive. Let me know if a gist would be helpful, I could put something together.

@tmenier tmenier closed this as completed Dec 14, 2023
@KorsG
Copy link

KorsG commented Dec 14, 2023

Totally forgot to respond - sorry about that!

But I understand you reasoning and we will basically just skip supporting netstandard 2.0 (as we really dont use it anymore) and just support .NET Framework and .NET 6+, by using #if directives as you suggested with Servicepoint for .NET Framework and SocketHttpHandler for .NET 6+.

Looking forward to try out the new version and thanks for all your hard work 👍

@cremor
Copy link

cremor commented Oct 15, 2024

I'm in the process of updating a .NET Framework 4.x solution to Flurl v4. One advantage of Flurl's ConnectionLeaseTimeout property was that you could set it globally. Now I have to set it via ServicePoint for each host.

Is there a way to globally run some code when a new Flurl client is created and where I also can access the base URL? I tried it in IFlurlClientCache.WithDefaults but the IFlurlClientBuilder that is used in there doesn't expose the base URL.

@tmenier
Copy link
Owner Author

tmenier commented Oct 15, 2024

@cremor I think you now have 2 reasons to not upgrade Flurl in your .NET Framework app. ;)

@cremor
Copy link

cremor commented Oct 15, 2024

I've created a WithConnectionLeaseTimeout extension method on IFlurlClient for now, so it's not that bad. But I have to remember to call this in all my clients after calling IFlurlClientCache.GetOrAdd (I use the DI pattern). It would be a bit better if it could be in one central place, that's why I asked for such an extension point.

@tmenier
Copy link
Owner Author

tmenier commented Oct 15, 2024

@cremor How about an extension method on IFlurlClientCache instead. It can take care of both the GetOrAdd and the ConnectionLeaseTimeout.

@cremor
Copy link

cremor commented Oct 15, 2024

Good idea, I'll do that (once #845 is solved), thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Released
Development

No branches or pull requests

3 participants