search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.44k stars 4.76k forks source link

ServicePoint ConnectionLimit reset when creating a web request #27298

Closed drauch closed 4 years ago

drauch commented 6 years ago

I'm configuring an explicit ConnectionLimit for a given ServicePoint (note, I don't set ServicePointManager.DefaultConnectionLimit, as I want to set the new limit only for one specific service).

However, my ServicePoint's ConnectionLimit seems to be reset when creating a new request - look in https://github.com/dotnet/corefx/blob/release/1.1.0/src/System.Net.Http/src/System/Net/Http/HttpClientHandler.Net46.cs in line 500 -> it is reset to the default! https://github.com/dotnet/corefx/blob/3ac33e4df7acd85c557bc5cecb9a4b4cb4a67007/src/System.Net.Http/src/System/Net/Http/HttpClientHandler.Net46.cs#L500

Why is that the case? Doesn't this make ConnectionLimit on ServicePoint useless? How to workaround this problem?

drauch commented 6 years ago

OK, it looks like this is https://github.com/Microsoft/dotnet/blob/master/releases/net471/KnownIssues/534719-Networking.ServicePoint.ConnectionLimit%20default%20behavior%20changed.md

But why is it working when referencing my GAC 4.7.1 version and it is only defect when I use the System.Net.Http NuGet package 4.3.3 (latest)? Shouldn't both fail?

karelz commented 6 years ago

The latest NuGet uses the same implementation as .NET Framework. In general, we recommend to drop the System.Net.Http nuget dependency as it has no advantage over using the one in .NET Framework. (previous nuget package versions tried to use different 'better' implementation and we failed to do it right, so we reverted all that)

The known issue in 4.7.1 states that it may be fixed in future. I guess that your version of .NET Framework System.Net.Http.dll has been serviced with newer, fixed version (e.g. from 4.7.2). If you look at full FileVersion of that file (the one with branch name), we can confirm.

Closing as answered, let me know if you have further questions.

Context: System.Net.Http nuget package

drauch commented 6 years ago

we recommend to drop the System.Net.Http nuget dependency as it has no advantage over using the one in .NET Framework

Why's that so? We have to use the NuGet package, otherwise we run into exceptions.

I guess that your version of .NET Framework System.Net.Http.dll has been serviced with newer, fixed version (e.g. from 4.7.2)

That would be splendid, but I couldn't find anything in the release notes. Do you know by chance which issue fixed the problem?

drauch commented 6 years ago

Is it possible the problem originates from https://github.com/Microsoft/dotnet/blob/master/releases/net472/KnownIssues/613745%20-%20Single-name%20references%20are%20removed%20by%20the%20SDK%20when%20targeting%204.7.2.md ?

karelz commented 6 years ago

Why's that so? We have to use the NuGet package, otherwise we run into exceptions.

See the description above: https://github.com/dotnet/corefx/issues/32081#issuecomment-418432928 - the NuGet package doesn't bring anything useful. You should not run into exceptions, unless some of your dependencies brings in the NuGet package into your dependency graph - in which case you may get version mismatch.

That would be splendid, but I couldn't find anything in the release notes. Do you know by chance which issue fixed the problem?

Check out this entry in 4.7.2 release notes:

Is it possible the problem originates from https://github.com/Microsoft/dotnet/blob/master/releases/net472/KnownIssues/613745%20-%20Single-name%20references%20are%20removed%20by%20the%20SDK%20when%20targeting%204.7.2.md ?

That bug itself won't cause the original issue you reported. However, it may play role in exposing it by changing which nuget package you reference (or not).

drauch commented 6 years ago

You should not run into exceptions, unless some of your dependencies brings in the NuGet package into your dependency graph - in which case you may get version mismatch.

Yeah, and what should I do in case I do get exceptions?! :)

Check out this entry in 4.7.2 release notes:

This is a different bug which only solves the problem for the localhost requests. So this is not fixing the bug.

karelz commented 6 years ago

Yeah, and what should I do in case I do get exceptions?! :)

I'd start with root-causing them - are those version mismatch exceptions? Who brings in the NuGet package dependency? Is it something you can change? You may find yourself in the rare case where you cannot drop the dependency. That, however, does not change our general guidance.

This is a different bug which only solves the problem for the localhost requests. So this is not fixing the bug.

In that case I don't know what and where addressed the problem. I am not quite sure anymore what "the problem" actually is.

Stepping back a bit ... Looking at the code: https://github.com/dotnet/corefx/blob/3ac33e4df7acd85c557bc5cecb9a4b4cb4a67007/src/System.Net.Http/src/System/Net/Http/HttpClientHandler.Net46.cs#L500 Note that the HttpWebRequest uses special _connectionGroupName: https://github.com/dotnet/corefx/blob/3ac33e4df7acd85c557bc5cecb9a4b4cb4a67007/src/System.Net.Http/src/System/Net/Http/HttpClientHandler.Net46.cs#L481 https://github.com/dotnet/corefx/blob/3ac33e4df7acd85c557bc5cecb9a4b4cb4a67007/src/System.Net.Http/src/System/Net/Http/HttpClientHandler.Net46.cs#L426 Which uses special ServicePoint, associated ONLY with HttpClient requests. The value overriding ServicePoint.ConnectionLimit is actually coming from HttpClientHandler.MaxConnectionPerServer API, which is the HttpClient-way of setting ConnectionLimit: https://github.com/dotnet/corefx/blob/3ac33e4df7acd85c557bc5cecb9a4b4cb4a67007/src/System.Net.Http/src/System/Net/Http/HttpClientHandler.Net46.cs#L213-L218

All that code sounds very reasonable to me - HttpWebRequest and ServicePoint are just internal details when you use HttpClientHandler and HttpClient on .NET Framework. You should not combine old APIs (HttpWebRequest/ServicePoint) and new APIs (HttpClientHandler/HttpClient) yourself. If you're using HttpClientHandler, stick to that API surface and let the underlying implementation do what it needs.

karelz commented 6 years ago

If you believe there is still a bug somewhere, I'd suggest to file issue with minimal repro showcasing what exactly is wrong (and on which version).

drauch commented 6 years ago

Now it's confirmed, the August 2018 security & quality rollup fixed the problem. That is why it looks fine in the source code you posted - it's the fixed version ;)

See https://github.com/Microsoft/dotnet/blob/master/releases/net471/KnownIssues/534719-Networking.ServicePoint.ConnectionLimit%20default%20behavior%20changed.md on the very bottom.

Now it's time to get rid of our NuGet reference to System.Net.Http, but that's another story.

Thanks for engaging!

kumarongithub commented 5 years ago

Does this also impact dotnetstandard specifically 2.0