[csharp][netcore-httpclient] Reuse HttpClient, Allow use of external HttpClient. Patch to previous PR.

[lucamazzanti]

Fixed an existing issue that made the HttpClient open new sockets for each connection.
Also makes HttpClient settable from external.
Fixes: #9060
Patches: #9085

Updated constructors to be more explicit in the usage.
Updated documentation to underline the use cases and the issue.

PR checklist

• Read the contribution guidelines.
• Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
• Run the following to build the project and update samples:

  ./mvnw clean package 
  ./bin/generate-samples.sh
  ./bin/utils/export_docs_generators.sh
  

  Commit all changed files.
  This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
  These must match the expectations made by your contribution.
  You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
  For Windows users, please run the script in Git BASH.
• File the PR against the correct branch: master, 5.1.x, 6.0.x
• If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@mandrean (2017/08) @frankyjuang (2019/09) @shibayan (2020/02) @Blackclaws (2021/03)

[auto-labeler]

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

[Blackclaws]

Looked over it real quick, looks fine from a code standpoint on first glance. We should discuss whether we really want to just assume that having these features missing is going to be noticed by users and having the info in the readme itself is going to be fine.

I'm not even sure how many people know about the handler in the first place. Though I do get it makes for much easier usage and maybe proxy, cookies and client certificates are special use cases anyway.

We could throw exceptions when those features are used and no handler is set. That would be another way to do things.

[lucamazzanti]

Looked over it real quick, looks fine from a code standpoint on first glance. We should discuss whether we really want to just assume that having these features missing is going to be noticed by users and having the info in the readme itself is going to be fine.

You are right, I removed the disable feature parameter because it was 1:1 related with the handler, but I understood your idea to ask the permission to disable some features.

I'm not even sure how many people know about the handler in the first place. Though I do get it makes for much easier usage and maybe proxy, cookies and client certificates are special use cases anyway.

Seeing that nobody has view the issue I could say not a lot of people. An issue that you can see in a real production scenario, nothing you can easily test here. Thinking about my usage in the Net Core, the base documentation is clear and cover a few but detailed points, one is the DI and its features. One of that is the HttpClient and its handlers lifcecyle, plus a resilient integration with Polly for retries, caching. The handlers are pooled, and you recieve a configured, working HttpClient, not the handler.
So I think that someone that embrace the current features will not use them, just the client.
If you don't relies on the IHttpClientFactory you need to do some not so simple work: a thread safe pool with time evictions.
So probably the handler will be used only for this remaining features, not a basic scenario.

It could throw exceptions when those features are used and no handler is set. That would be another way to do things.

Its a good idea, they depends a lot on the configuration so probably they will not comes out easily at coding time, but in some customer settings during the production setup. I still prefer to have a clear error probaly than a silent behavior.

[lucamazzanti]

Think about to remove the handler\clients from local variables and from method signature and use them as class fields as they are now.

[Blackclaws]

Think about to remove the handler\clients from local variables and from method signature and use them as class fields as they are now.

Yeah, do that change, its a leftover from when they were created locally.

[Blackclaws]

Its a good idea, they depends a lot on the configuration so probably they will not comes out easily at coding time, but in some customer settings during the production setup. I still prefer to have a clear error probaly than a silent behavior.

I'd say do that then. Introduce an exception with "Operation not supported" or something like that, including a useful error message. That of course doesn't work for the extraction of cookies in the ToApiResponse part, but I think we can leave that one.

[lucamazzanti]

Sorry I closed for error

[lucamazzanti]

I don't know maybe the check can be made at constructor time when you have both handler valued and the configuration with something not supported. I have too many doubt on the configuration, I see is passed\merged at ctor but is also used in the call passing it from the outside so I cannot do that.
I must put the check where now there is the null check, at call time.

What type of exception is more adapt to the client?
I actually see some preconditions on arguments, a platform exception and the ApiException at call time (I think related to the rest call not the setup). I will not peek them, I see an InvalidOperationException in the Configuration class, maybe is usable.
Otherwise: NotSupportedException, see the difference.

[Blackclaws]

I think InvalidOperationException or NotSupportedException both kind of fit. In general yeah I think the csharp surface needs a bit of a rework, as its used by the generated code I think the generation never is used after the initial client setup. So maybe I'll rework that afterwards as well.

[Blackclaws]

Looks good to me. I like having exceptions in just these specific cases where it actually matters. So good to be merged from my points of view.
Thanks for the pull request!

[wing328]

Circle CI failure (not related to this change) already fixed in the master

[wing328]

@lucamazzanti when you've time, can you please PM me via https://join.slack.com/t/openapi-generator/shared_invite/enQtNzAyNDMyOTU0OTE1LTY5ZDBiNDI5NzI5ZjQ1Y2E5OWVjMjZkYzY1ZGM2MWQ4YWFjMzcyNDY5MGI4NjQxNDBiMTlmZTc5NjY2ZTQ5MGM to discuss a little bit more about your use case? Thank you.

[lucamazzanti]

Thanks you all for the project and the support

[lucamazzanti]

@lucamazzanti when you've time, can you please PM me via https://join.slack.com/t/openapi-generator/shared_invite/enQtNzAyNDMyOTU0OTE1LTY5ZDBiNDI5NzI5ZjQ1Y2E5OWVjMjZkYzY1ZGM2MWQ4YWFjMzcyNDY5MGI4NjQxNDBiMTlmZTc5NjY2ZTQ5MGM to discuss a little bit more about your use case? Thank you.

Done

[lucamazzanti]

So this merge will be released in milestone 5.1.1, I'll test it and move in production after the release.

[Blackclaws]

So this merge will be released in milestone 5.1.1, I'll test it and move in production after the release.

I might be doing another pull request to clean up the huge amount of constructors we have right now. I think we can boil those down a bit.

[lucamazzanti]

I understand there are so many constructors.
It was just to manage better the parameters and intellisense in code completion hints.
I had a code smell seeing a unique ctor with all nullable multiple parameters and some strong preconditions inside to validate their coupled states. I preferred to be more explicit on their different use cases.
But you are right, they are a LOT, maybe too much.

[lucamazzanti]

Created PR #9145 to reduce the number of constructors.