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

Change generated clients to be noob-safe in how they use HttpClient #1097

Open
ishepherd opened this Issue Dec 11, 2017 · 17 comments

Comments

Projects
None yet
6 participants
@ishepherd
Copy link

ishepherd commented Dec 11, 2017

Edit So I had some noob misunderstanding in HttpClient best practice.

  • Some Microsoft sources say you should keep a static HttpClient forever, to avoid socket exhaustion
  • Others point out that, no, holding sockets forever is bad (for instance if you are using load balancing) so you should recycle your HttpClient now and then, (just not on every request)

I appreciate you have added stuff for control of HttpClient lifecycle, but perhaps you can make this safe out of the box? Noobs like me might also learn something by reviewing the generated code.

Here is one approach: dotnet/corefx#11224 (comment)


Original comments...

According to https://aspnetmonsters.com/2016/08/2016-08-27-httpclientwrong/

After each call, even though you've disposed the HttpClient, Windows holds the socket(s) it was using open for 240 seconds. There are only thousands of sockets available, so you can achieve socket exhaustion quite easily.

Although [HttpClient] implements the IDisposable interface it is actually a shared object. This means that under the covers it is reentrant) and thread safe. Instead of creating a new instance of HttpClient for each execution you should share a single instance of HttpClient for the entire lifetime of the application.
...

  1. Make your HttpClient static.
  2. Do not dispose of or wrap your HttpClient in a using unless you explicitly are looking for a particular behaviour (such as causing your services to fail).
@ishepherd

This comment has been minimized.

Copy link
Author

ishepherd commented Dec 11, 2017

Reading further...

Although some MSDN agrees...

HttpClient is intended to be instantiated once and reused throughout the life of an application. The following conditions can result in SocketException errors:

  • Creating a new HttpClient instance per request.
  • Server under heavy load.

Creating a new HttpClient instance per request can exhaust the available sockets.

...it can create another issue: You don't want to keep a socket open forever, for example if there is a DNS flip to a different deployment, you will still be talking to the old one.

Some people have custom logic so that both issues are solved. http://disq.us/p/1d4okj9 dotnet/corefx#11224 (comment)

@ishepherd ishepherd changed the title Change the way generated clients use HttpClient - Potential socket exhaustion Change generated clients to be noob-safe in how they use HttpClient Dec 11, 2017

@RSuter

This comment has been minimized.

Copy link
Owner

RSuter commented Dec 11, 2017

My problem with HttpClient is that there is no single or commonly used best practice... This is why I it is currently up to the user to correctly use it :-). The question: What is the best approach to solve this problem?

As you may know there are currently three ways to handle HttpClient in the generated code:

  • Create an HttpClient per request (no injection, default)
  • Inject HttpClient, handled by client
  • Change the HttpClient type, to e.g. "MyNamespace.IHttpClient" (the interface just needs the required methods which are generated).

Maybe we should implement a separate library which provides which provides HttpClient proxy implementations which internally manage HttpClient based on time or whatever - the library could provide different implementation which can be chosen depending on the use case/scenario?

@ishepherd

This comment has been minimized.

Copy link
Author

ishepherd commented Dec 11, 2017

I'll bet the commonly used 'best' practice is

  • Create an HttpClient per request (no injection, default)

Devs think "I Disposed it, I did the right thing!", not realizing that creating too many HttpClients within 240 seconds will break their system...

My conclusion is that HttpClients should always be static (per my MSDN link) but, because this might lead to it reusing a single socket forever, kept in a pool with round-robin and expiry time.

The pool would need a couple of parameters

  • Size (Reasonable default - 8?)
  • Expiry (Reasonable default - 10 seconds?)

I think this would work for everyone?

Obviously I will be looking for a prebuilt pool class... will have a dig around

Apparently this is faster as well as safer: https://github.com/mspnp/performance-optimization/blob/master/ImproperInstantiation/docs/LoadTesting.md

Edit collection of info on this issue https://softwareengineering.stackexchange.com/a/330379/199040

@RSuter

This comment has been minimized.

Copy link
Owner

RSuter commented Dec 19, 2017

So is there a widely used NuGet package with a good HttpClient wrapper which we could use or propose to use?

@ishepherd

This comment has been minimized.

Copy link
Author

ishepherd commented Dec 20, 2017

@RSuter I haven't had time to look into this any further. Hopefully next month.

@bulfonjf

This comment has been minimized.

@ishepherd

This comment has been minimized.

Copy link
Author

ishepherd commented Jan 18, 2018

You could code a basic version directly in the generator to avoid sprouting another dependency. What we're now doing is

private static readonly ObjectWithLifespan<HttpClient> OurHttpClient = new ObjectWithLifespan<HttpClient>(
    lifespan: TimeSpan.FromSeconds(30),
    creator: () => new HttpClient());

public class ObjectWithLifespan<T>
{
    private class Box
    {
        public T Instance;
        public DateTime Expiry;
    }

    private Box Instance;
    private readonly TimeSpan Lifespan;
    private readonly Func<T> Creator;

    /// <summary>
    /// Function provided must be safe. It might be invoked multiple times needlessly or concurrently,
    /// the value you get may not be from the latest invocation
    /// </summary>
    public ObjectWithLifespan(TimeSpan lifespan, Func<T> creator)
    {
        Lifespan = lifespan;
        Creator = creator ?? throw new ArgumentNullException(nameof(creator));
    }

    public T Value
    {
        get
        {
            var current = Instance;
            if (current == null || current.Expiry <= DateTime.UtcNow)
            {
                current = new Box { Instance = Creator(), Expiry = DateTime.UtcNow + Lifespan };
                Instance = current;
            }
            return current.Instance;
        }
    }

}
@RSuter

This comment has been minimized.

Copy link
Owner

RSuter commented Jan 30, 2018

I think we should provide an implementation for IHttpClient in a NuGet package so that you can specify IHttpClient instead of HttpClient in the NSwag settings and use this NuGet package with the special implementation...

@ishepherd

This comment has been minimized.

Copy link
Author

ishepherd commented Jan 30, 2018

Whatever approach, I think it should be the default. I don't see that all the confusion around this is really justified, it oughta be completely possible to have a 'right' solution

@bulfonjf How about this wrapper? https://github.com/NimaAra/Easy.Common/blob/master/Easy.Common/RestClient.cs

depends on ServicePointManager - looks like it's problematic on .NET Core... dotnet/corefx#11224

@cremor

This comment has been minimized.

Copy link

cremor commented Apr 27, 2018

Microsoft will provide a HttpClientFactory in ASP.NET Core 2.1. See https://github.com/aspnet/HttpClientFactory for the code and https://blogs.msdn.microsoft.com/webdev/2018/02/02/asp-net-core-2-1-roadmap/#httpclientfactory for some documentation.

@reberinformatik

This comment has been minimized.

Copy link

reberinformatik commented Jun 7, 2018

Probably the implementation of Flurl could help. It uses HttpClient under the hood and a factory to instantiate it.

@Eneuman

This comment has been minimized.

Copy link

Eneuman commented Feb 10, 2019

Now that we have services.AddHttpClient in .Net Core 2.1+, I would suggest changing the C# client template in the following way:

The public constructor:

        public ProcessmotorClient(System.Net.Http.HttpClient httpClient)
        {
            BaseUrl = baseUrl; 
            _httpClient = httpClient; 
            _settings = new System.Lazy<Newtonsoft.Json.JsonSerializerSettings>(() => 
            {
                var settings = new Newtonsoft.Json.JsonSerializerSettings();
                UpdateJsonSerializerSettings(settings);
                return settings;
            });
        }

is changed to

        public ProcessmotorClient(string baseUrl, System.Net.Http.HttpClient httpClient)
        {
            _httpClient = httpClient; 
            _settings = new System.Lazy<Newtonsoft.Json.JsonSerializerSettings>(() => 
            {
                var settings = new Newtonsoft.Json.JsonSerializerSettings();
                UpdateJsonSerializerSettings(settings);
                return settings;
            });
        }

BaseUrl is removed in favor for httpClient.BaseAddress

This change will make it possible to do this (in startup.cs) without adding any extra code:

    services.AddHttpClient<MyClient>(client =>
    {
        client.BaseAddress = new Uri("https://api.myclient.com/");
    });

Using AddHttpClient this way also solves problems with port starvation and DNS lookup issues in Micro Services.

@RSuter

This comment has been minimized.

Copy link
Owner

RSuter commented Feb 14, 2019

Have you tried to disable the UseBaseUrl setting?

image

Produces:

image

@Eneuman

This comment has been minimized.

Copy link

Eneuman commented Feb 14, 2019

Nice :)
I didn't know this already existed.
Can this also be set as a parameter in .projcs when using the preview of NSwag.MSBuild.CodeGeneration?
(I'm trying to automaticly create a Nuget Package of the client each time I check in code to Azure DevOps)

@RSuter

This comment has been minimized.

Copy link
Owner

RSuter commented Feb 14, 2019

I think with the NSwagOptions attribute you can pass /UseBaseUrl:false - but not sure if the attribute is called like this...

@Eneuman

This comment has been minimized.

Copy link

Eneuman commented Feb 14, 2019

It worked great! Thanks!
I added the following parameter: GenerateNSwagCSharpOptions="/UseBaseUrl:false"

@RSuter

This comment has been minimized.

Copy link
Owner

RSuter commented Feb 14, 2019

BTW: NSwag.MSBuild.CodeGeneration has been unlisted/renamed to NSwag.ApiDescription.Design

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.