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.

Sign up for GitHub

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

[csharp][netcore-httpclient] Reuse HttpClient, Allow use of external HttpClient, Fix Socket Exhaustion, Alternative With Constructor Injection #9085

Merged
merged 13 commits into from
Mar 27, 2021
Merged

[csharp][netcore-httpclient] Reuse HttpClient, Allow use of external HttpClient, Fix Socket Exhaustion, Alternative With Constructor Injection #9085

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
33bcb4a
Do not allow not reusing HttpClient, fix socket exhaustion issues, ad…
Blackclaws Mar 25, 2021
68b5d76
Add Readme change
Blackclaws Mar 25, 2021
dcf3e2d
Alternative version via constructor injection
Blackclaws Mar 25, 2021
5ec0b1d
Update samples
Blackclaws Mar 25, 2021
2a1227e
Update usage in ExecAsync
Blackclaws Mar 25, 2021
402c875
Clean Constructor
Blackclaws Mar 25, 2021
f7b07fa
Add Warning to constructor, Update Samples
Blackclaws Mar 25, 2021
ef8f51c
Add warning, fix last constructor, update samples
Blackclaws Mar 25, 2021
e18dd4b
Change client to private in constructor injection
Blackclaws Mar 26, 2021
c3aed2e
Add disposable to ApiClient
Blackclaws Mar 26, 2021
d08d2b9
Do not dispose handler if we did not create it
Blackclaws Mar 26, 2021
ddcaad8
Update samples
Blackclaws Mar 26, 2021
e1ca890
Add disable switch and update samples and documentation
Blackclaws Mar 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions modules/openapi-generator/src/main/resources/csharp-netcore/README.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,19 @@ System.Net.WebProxy webProxy = new System.Net.WebProxy("http://myProxyUrl:80/");
webProxy.Credentials = System.Net.CredentialCache.DefaultCredentials;
c.Proxy = webProxy;
```
{{#useHttpClient}}

To use your own HttpClient instances just assign the static properties on the ApiClient:

```csharp
HttpClientHandler yourHandler = new HttpClientHandler();
HttpClient yourHttpClient = new HttpClient(yourHandler);
{{packageName}}.Client.ApiClient.HttpClient = yourHttpClient;
```

You can also change properties of the HttpClientHandler on the API client directly.

{{/useHttpClient}}

<a name="getting-started"></a>
## Getting Started
Expand Down
48 changes: 19 additions & 29 deletions ...enapi-generator/src/main/resources/csharp-netcore/libraries/httpclient/ApiClient.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,10 +164,8 @@ namespace {{packageName}}.Client
{{>visibility}} partial class ApiClient : ISynchronousClient{{#supportsAsync}}, IAsynchronousClient{{/supportsAsync}}
{
private readonly String _baseUrl;
{{#reUseHttpClient}}
private readonly HttpClientHandler _httpClientHandler;
private readonly HttpClient _httpClient;
{{/reUseHttpClient}}
public HttpClientHandler HttpClientHandler;
public HttpClient HttpClient;

/// <summary>
/// Specifies the settings on a <see cref="JsonSerializer" /> object.
Expand All @@ -189,30 +187,28 @@ namespace {{packageName}}.Client
/// <summary>
/// Initializes a new instance of the <see cref="ApiClient" />, defaulting to the global configurations' base url.
/// </summary>
public ApiClient()
public ApiClient(HttpClient client = null, HttpClientHandler handler = null) : this({{packageName}}.Client.GlobalConfiguration.Instance.BasePath, client, handler)
{
_baseUrl = {{packageName}}.Client.GlobalConfiguration.Instance.BasePath;
{{#reUseHttpClient}}
_httpClientHandler = new HttpClientHandler();
_httpClient = new HttpClient(_httpClientHandler);
{{/reUseHttpClient}}
}

/// <summary>
/// Initializes a new instance of the <see cref="ApiClient" />
/// Initializes a new instance of the <see cref="ApiClient" />
Blackclaws marked this conversation as resolved.
Show resolved Hide resolved
/// </summary>
/// <param name="basePath">The target service's base path in URL format.</param>
/// <exception cref="ArgumentException"></exception>
public ApiClient(String basePath)
public ApiClient(String basePath, HttpClient client = null, HttpClientHandler handler = null)
{
if (string.IsNullOrEmpty(basePath))
throw new ArgumentException("basePath cannot be empty");

_baseUrl = basePath;
{{#reUseHttpClient}}
_httpClientHandler = new HttpClientHandler();
_httpClient = new HttpClient(_httpClientHandler);
{{/reUseHttpClient}}
/* TODO: Decide how to handle this case
if(client != null && handler == null) {
throw new ArgumentException("if providing HttpClient, you also need to provide its handler");
}*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the code sample in the readme:

HttpClientHandler yourHandler = new HttpClientHandler();
HttpClient yourHttpClient = new HttpClient(yourHandler);

Looks like if the HttpClient is provided, one can obtain its HttpClientHandler, right?

If that's the case, I would suggest simply ask for HttpClient in the constructor to make it easier for the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is unfortunately not the case. You can not acquire the handler from the HttpClient once it is constructed. The handler is used for setting/retrieving cookies as well as configuring the proxy and client certificates. We could say that in case a user passed in their own HttpClient that they would be handling all of that themselves or simply have to live with the fact that it doesn't work. Hence why I was wondering whether we should at least give some kind of warning.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just saw this: https://docs.microsoft.com/en-us/dotnet/api/system.net.http.httpclient?view=net-5.0#httpclient-and-net-core

Shall we simply use HttpClient = new HttpClient(); as it should pick the best available transport based on the platform? In other words, ApiClient will not store the HttpClientHandler.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, we are no longer able to set proxy options and client certificates anymore if we do that. Instantiating HttpClientHandler creates an underlying actual handler which is SocketsHttpHandler.

Copy link
Contributor Author

@Blackclaws Blackclaws Mar 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear, what we are doing by default is exactly what HttpClient and its default constructor are doing:

        public HttpClient()
            : this(new HttpClientHandler())
        {
        }

Copy link
Contributor

@lucamazzanti lucamazzanti Mar 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also thinked about the validation of both parameters as you.
But actually i think that I won't to get a complex DI for these few not so useful parameters.
I test it in a real net core application.
I see more things:

  • The disposable client now must be manage in the API template.
  • The api must have a better ctor to inject the client, the double interface is problematic to manage in the DI
    What do you think will be the usage, the client is prepared that way for example:
 services.AddHttpClient<IProductClient, ProductClient>()
 .SetHandlerLifetime(TimeSpan.FromMinutes(5))
.AddTransientHttpErrorPolicy(p => p.RetryAsync(3))
.AddTransientHttpErrorPolicy(p => p.CircuitBreakerAsync(5, TimeSpan.FromSeconds(30)));

Now i need to register the Api passing some related options like the basePath or configuration and the client or the api client.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you need customize the DI injection by running a lambda function that then sets the correct configuration at that point.

Right now adding just the HttpClient is not a supported UseCase. If you say this is relevant for you as well, and you want to use the client from IHttpClientFactory then we'll figure out a way to do that.

I talked about this also with @wing328 and we actually wanted to add a way to do this later if people request it. It appears you may have a usecase already?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a new version that includes a switch to disable the exception and the features that don't work if handler isn't present.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why in those 3+ years of net core, no one had this issue using a swagger codegen.
Maybe because in the official startup documentation they promote NSwag, effectively the only one actually support the injection of an HttpClient. They also underline very well the issues\tradeoffs in the HttpClient and how they manage it with the factory in the DI.

We moved to net core with a few projects in this years, those services consume others using rest api, with a Swagger codgen. In this days I monitored a strange pressure on the production environment, and I found the socket exsaustion, done by ourself with a chatty client. I compared the different behavior of the vanilla codegen in both framework, monitoring with wireshark:

  • one uses 1 socket, 1 TCP handshake, 1 TLS handshale, N calls
  • the other one open each times the pipeline consuming sockets

The breaking change is inside RestSharp, it has an issue open but the project seems aware to this difficult change to support net core and I think it has no future plans to fix it. The real change is inside the HttpWebRequest that actually in Net Core no more has a static lifecycle and create a new HttpClient.

So RestSharp is not a valid production library for net core, at all.
This means also Swagger is not a valid choice for net core.
Swagger seems no more active, the main author, William, is here now on OpenApi, reviewing you now (William thank you for the projects, they are great).

So the choice for us are:

  • create new contracts with NSwag for the net core projects, leaving Swagger.
  • cooperate on OpenApi to have a net-core client fully integrated in the core DI. OpenApi c# codegen is 99% the same as swagger so moving to OpenApi it seems the right way to continue using Swagger.

Your fix is just enough to avoid the exsaustion now.
But having a client that not fit easily in the DI can move us to choose another more solid way for the future.
But I also see a lot of interest in this PR and on this project to move to a working client for net core so I will await and cooperate with you if I can give some support.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've different suggestions but let's go with what you've so far. We can resume the discussion after the merge.


this.HttpClientHandler = handler ?? new HttpClientHandler();
this.HttpClient = client ?? new HttpClient(this.HttpClientHandler);
}

/// Prepares multipart/form-data content
Expand Down Expand Up @@ -275,6 +271,11 @@ namespace {{packageName}}.Client

HttpRequestMessage request = new HttpRequestMessage(method, builder.GetFullUri());

if (configuration.UserAgent != null)
{
request.Headers.TryAddWithoutValidation("User-Agent", configuration.UserAgent);
}

if (configuration.DefaultHeaders != null)
{
foreach (var headerParam in configuration.DefaultHeaders)
Expand Down Expand Up @@ -400,14 +401,8 @@ namespace {{packageName}}.Client
IReadableConfiguration configuration,
System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken))
{
{{^reUseHttpClient}}
var handler = new HttpClientHandler();
var client = new HttpClient();
{{/reUseHttpClient}}
{{#reUseHttpClient}}
var handler = _httpClientHandler;
var client = _httpClient;
{{/reUseHttpClient}}
var handler = this.HttpClientHandler;
var client = this.HttpClient;
var deserializer = new CustomJsonCodec(SerializerSettings, configuration);

var finalToken = cancellationToken;
Expand All @@ -423,11 +418,6 @@ namespace {{packageName}}.Client
handler.Proxy = configuration.Proxy;
}

if (configuration.UserAgent != null)
{
client.DefaultRequestHeaders.TryAddWithoutValidation("User-Agent", configuration.UserAgent);
}

if (configuration.ClientCertificates != null)
{
handler.ClientCertificates.AddRange(configuration.ClientCertificates);
Expand Down
Loading