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

Allow HttpClient instances to be refreshed periodically #4673

Merged
merged 8 commits into from
Apr 27, 2020

Conversation

Mpdreamz
Copy link
Member

@Mpdreamz Mpdreamz commented Apr 24, 2020

This PR fixes a longstanding wish to be able to recycle HttpClient instances periodically. We reuse HttpClient/HttpMessageHandler's to prevent TCP leakage but infamously this does not take DNS changes into account.

IHttpClientFactory from Microsoft.Extensions.Http solves this but the nuget package depends on too many things we don't want to depend on OOTB like Dependency Injection. This copies parts of this code (now MIT licensed as of February) so we can reuse the same strategy in the client.

Starting with netstandard2.1 SocketsHttpHander also supports PooledConnectionLifetime this however has a bug in corrolation with MaxConnectionsPerServer. This is fixed in netcoreapp3.0 and up.

The goal is to ship this new RequestDataHttpClientFactory introduced here in all TFM's.

In a follow up commit we'll introduce netcoreapp3.0 and netcoreapp3.1 TFM's that will only use RequestDataHttpClientFactory to create HttpClient instances but won't use its recycling capabilities since that's now handled by PooledConectionLifetime.

Mpdreamz and others added 6 commits April 24, 2020 18:09
In dotnet/runtime#22366 we found that if
HttpClient is using the curl handler it will lead to TCP connections
bleeding. Our DefaultConnectionLimit is very restrictive if this is
true. Our check however is too lenient and will default to true always
on .NET core since netcoreapp still ships with CurlHandler as recent as
`3.1.x`
This commit updates the implementation of determining if
a conservative default connection limit should be set by also checking
if CurlHandler exists when UseSocketsHttpHandler exists and is
disabled.
@Mpdreamz
Copy link
Member Author

I finally found where to remove travis-ci which was forever pending as we disabled it! It was a required check for master in the branch rules 🎉

Copy link
Contributor

@russcam russcam left a comment

Choose a reason for hiding this comment

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

LGTM, I left some small comments 👍

Mpdreamz and others added 2 commits April 27, 2020 11:10
@Mpdreamz Mpdreamz merged commit 4c340e8 into master Apr 27, 2020
@Mpdreamz Mpdreamz deleted the feature/master/client-factory branch April 27, 2020 09:38
@github-actions
Copy link
Contributor

The backport to 6.x failed:

The process 'git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-6.x 6.x
# Navigate to the new working tree
cd .worktrees/backport-6.x
# Create a new branch
git switch --create backport-4673-to-6.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick 4c340e87d4fb633593783a812ac867ac56313375
# Push it to GitHub
git push --set-upstream origin backport-4673-to-6.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-6.x

Then, create a pull request where the base branch is 6.x and the compare/head branch is backport-4673-to-6.x.

@github-actions

This comment has been minimized.

Mpdreamz added a commit that referenced this pull request Apr 27, 2020
Co-Authored-By: Russ Cam <russ.cam@elastic.co>
(cherry picked from commit 4c340e8)
Mpdreamz added a commit that referenced this pull request Apr 27, 2020
Mpdreamz added a commit that referenced this pull request Apr 27, 2020
6.x does not follow the new layout so a cherry-pick is to involved.

Manually reimplemented the same logic that introduces
`RequestDataHttpClientFactory` to periodically recycle HttpClient
instances.
Mpdreamz added a commit that referenced this pull request Apr 30, 2020
russcam added a commit that referenced this pull request Jun 23, 2020
Relates: #4673

This commit adds the Clients property back to HttpConnection
,for binary compatibility, with an ObsoleteAttribute applied
to indicate that the property is no longer used.
russcam added a commit that referenced this pull request Jun 23, 2020
Relates: #4673

This commit adds the Clients property back to HttpConnection
,for binary compatibility, with an ObsoleteAttribute applied
to indicate that the property is no longer used.
github-actions bot pushed a commit that referenced this pull request Jun 23, 2020
Relates: #4673

This commit adds the Clients property back to HttpConnection
,for binary compatibility, with an ObsoleteAttribute applied
to indicate that the property is no longer used.
russcam added a commit that referenced this pull request Jun 23, 2020
Relates: #4673

This commit adds the Clients property back to HttpConnection
,for binary compatibility, with an ObsoleteAttribute applied
to indicate that the property is no longer used.

Co-authored-by: Russ Cam <russ.cam@elastic.co>
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

Successfully merging this pull request may close these issues.

2 participants