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

What is the reason for keeping HttpClientCacheSize to number Environment.ProcessorCount on Linux #1929

Closed
madhub opened this issue Oct 12, 2021 · 10 comments
Labels
bug This issue is a bug. module/sdk-core p2 This is a standard priority issue queued s3

Comments

@madhub
Copy link
Contributor

madhub commented Oct 12, 2021

The Question

Currently we are investigating slowness of S3 access on Linux deployment, upon investigation we found number of cache HttpClient varies between Windows & Linux relevant section of the code is ClientConfig.cs

What is the reason of having different number for cached HttpClients ?
From .NET Core 2.1 onwards HttpClient uses SocketsHttpHandler on both Windows & Linux and their should not be any behavior changes.

Here is the sample scenario to show the difference
On 10 CPU systems make 10 S3 request to same object on the same bucket.

  • On Windows - First request takes time, and all subsequent request are fast
  • On Linux - All request request takes equal amount of time which includes connection establish cost

So should we set the AmazonS3Config.HttpClientCacheSize to 1 on Linux ? , any recommendation ?

Environment

  • SDK Version: 3.7.3.12
  • Package Version: Awssdk.s3 ver: 3.7.3.12, Awssdk.core ver:3.7.3.19
  • OS Info: Windows 10 , Centos 8
  • Build Environment :Terminal dotnet
  • Targeted .NET Platform: .NET 3.1

This is a ❓ general question

@madhub madhub added guidance Question that needs advice or information. needs-triage This issue or PR still needs to be triaged. labels Oct 12, 2021
@ashishdhingra
Copy link
Contributor

Hi @madhub,

Good morning.

Thanks for posting the guidance question. Looking at the documentation comment for HttpClientCacheSize, it mentions that On Windows the default value is 1 since the underlying native implementation does not have throttling constraints like the non Windows Curl based implementation. For non Windows based platforms the default is the value return from System.Environment.ProcessorCount.. Based on the value of the cache size, it creates number of HttpClient instances at HttpRequestMessageFactory.CreateHttpClientCache(). The HttpClient is retrieved from client cache via HttpClientCache.GetNextClient(). If you notice that if the number of HttpClient(s) in cache is 1, it just returns the first client, else it uses Interlocked.Increment() to randomly determine the client to be used from cache. This might cause some lag due to thread safe locking.

While we cannot advise if you should set AmazonS3Config.HttpClientCacheSize to 1 on Linux since as mentioned above, the cache is used to prevent throttling issues. Changing the default behavior could result in throttling issues in non-Windows environments as comment mentions. Hope this helps.

Thanks,
Ashish

@ashishdhingra ashishdhingra added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. s3 and removed needs-triage This issue or PR still needs to be triaged. labels Oct 12, 2021
@ashishdhingra ashishdhingra self-assigned this Oct 12, 2021
@madhub
Copy link
Contributor Author

madhub commented Oct 13, 2021

Thanks for the response,

.NET Core 2.1 on wards implementation is same for Windows & Non Windows platforms .See the documentation here HttpClient Sockets improvements.
Both Windows & non Windows platform uses the same implementation SocketsHttpHandler

.NET Core 2.1 Release
consistent-cross-platform-implementation

More information about how HttpClient internal implemented before .NET Core 2.1 and after 2.x

@normj
Copy link
Member

normj commented Oct 13, 2021

@madhub when this code was written .NET Core was using a different implementation for windows and linux. We do need to revise this code now that both platforms use SocketsHttpHandler.

I suspect you are right and we should just use one cached HttpClient with the modern versions of .NET Core. We will need to do some validation first before making that change.

@ppittle ppittle added B bug This issue is a bug. and removed guidance Question that needs advice or information. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Oct 13, 2021
@ashishdhingra ashishdhingra removed their assignment Oct 13, 2021
@madhub
Copy link
Contributor Author

madhub commented Oct 15, 2021

Note that HttpClient uses SocketsHttpHandler common implementation from .NET Core 2.1 onwards , previous versions ( .NET Core 2.0) still uses different implementation for Windows(WinHttpHandler) & Linux (CurlHandler) , since AWS SDK is .NETStandard 2.0 library same can be used deployed in .NET Core 2.0 , .NET Core 2.1,.NET Core 3.1 and NET 5 runtime hence AmazonS3Config.HttpClientCacheSize should be set to one only if the runtime is >.NET Core 2.1

@ozziepeeps
Copy link

We got bitten by this exact issue, combined with our use of the new DOTNET_PROCESSOR_COUNT environment variable in NET6 which exacerbated things. We can confirm that setting HttpClientCacheSize to 1 improved performance in our setup.

It would be good to get this fixed in the SDK @normj.

@ashishdhingra ashishdhingra added p2 This is a standard priority issue queued and removed B labels Nov 2, 2022
@rsaunderson88-kw
Copy link

rsaunderson88-kw commented Feb 23, 2023

I was also just bit by this when testing on Linux. Requests under 1 MB went from taking almost 200 ms to finish GetObjectAsync and another 200 to execute CopyToAsync to 50ms round trip. This is a pretty large difference even for a simple example.

@danielmarbach
Copy link
Contributor

Why hasn't this value been streamlined? I mean we are now alread in .NET 8 preview mode, which will be the next LTS and all of what has been written here in this issue is valid for multiple years now.

@danielmarbach
Copy link
Contributor

See #2549

@ashishdhingra
Copy link
Contributor

Closing issue since PR #2549 is merged and released per #2549 (comment).

@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. module/sdk-core p2 This is a standard priority issue queued s3
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants