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

Update HttpClient.xml #4330

Closed
wants to merge 2 commits into from
Closed

Update HttpClient.xml #4330

wants to merge 2 commits into from

Conversation

feherzsolt
Copy link

Title

Add additional remark about connection limit

Summary

The documentation suggests using one HttpClient instance, which is reused in the application. However in multithreaded applications, doing http requests to the same server will quickly exhaust the maximum allowed concurrent connection limit (2 or 10 for web applications), which can degrade performance. I think developers following the guidance about one HttpClient in the application would benefit having this piece of information.

The documentation suggests using one HttpClient instance, which is reused in the application. However in multithreaded applications, doing http requests to the same server will quickly exhaust the maximum allowed concurrent connection limit (2 or 10 for web applications), which can degrade performance.
@rpetrusha rpetrusha added the ✨ 1st-time docs contributor! Indicates PRs from new contributors to the docs repository label Feb 6, 2018
Copy link
Contributor

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

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

Thanks for contributing to the dotnet/docs repo, @feherzsolt, and for submitting this PR to provide additional information about the HttpClient class. I have a number of suggested changes.
Also, @davidsh, could you review this PR, please?

@@ -66,6 +66,8 @@ public class GoodController : ApiController
}

```

By default <xref:System.Net.ServicePointManager.DefaultConnectionLimit> will define the maximum number of concurrent connections to the same server.
Copy link
Contributor

Choose a reason for hiding this comment

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

comma after "default"
after defaultConnectionLimit, ?displayProperty=nameWithType
will define --> defines

Copy link
Author

Choose a reason for hiding this comment

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

Thank you, i have added the changes.

Suggested changes.
@davidsh
Copy link
Contributor

davidsh commented Feb 7, 2018

I don't understand why adding this line of text about ServicePointManager helps the HttpClient documentation. It seems out of place here since the documentation is all about HttpClient class.

Yes, there is a relationship between HttpClient and ServicePointManager. That is an artifact of the history of how the class was introduced since it uses HttpWebRequest underneath for the networking implementation and thus is subject to limits of the "global" settings of ServicePointManager. However, for other platforms like .NET Core, for example, ServicePointManager is not fully implemented, nor supported and the settings on it don't affect HttpClient in the same way (or at all).

cc: @karelz @stephentoub

@karelz
Copy link
Member

karelz commented Feb 7, 2018

I think it would makes sense to mention top problems / gotchas related to using single instance of HttpClient in your app near the sample code. We should IMO cover limit of connections per server and Dns refresh problem (incl. guidance to periodically replace HttpClient singleton, or using HttpClientFactory once it is released).
The information should be specific for applicable frameworks (.NET Framework vs. .NET Core vs. others) as @davidsh points out.

@feherzsolt
Copy link
Author

Thank you for checking out the PR.
IMO when the documentation provides a best practice using the mentioned class, readers should be aware of the consequences of doing so. When following the documentation this could cause a very subtle behavior which can easily go production without notice. Even if discovered it is quite hard to track down the mentioned relation to ServicePointManager in the documentation.
I think that HttpClient is a very common class, and running into this has a high chance in a production web application or even higher in a desktop application / service.
I agree that this information might not relate to the HttpClient class itself directly, but I would add it to the recommendation / code example about using one HttpClient.
Thank you for pointing out, I totally agree that the information should be made specific for applicable frameworks.

@rpetrusha
Copy link
Contributor

Thanks, @karelz and @davidsh. Your suggestion sounds good, @karelz. Since the issue goes beyond this PR, what I think we should do is open a GitHub issue to address the issues raised by @feherzsolt and @karelz in an implementation-specific way. Once the issue is open, I'll close the PR. Since I'm at an offsite, I'll open it early next week at the latest, unless you'd like to open it earlier, @feherzsolt.
Does that sound good?

@feherzsolt
Copy link
Author

That sounds good. Please open the issue @rpetrusha when you get back. Thank you all for the help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ 1st-time docs contributor! Indicates PRs from new contributors to the docs repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants