limit http request number per source through NuGet.Config #2382
Conversation
zhili1208
requested review from
joelverhagen,
rohit21agrawal,
PatoBeltran,
nkolev92,
mishra14 and
jainaashish
| @@ -35,5 +35,10 @@ public static SemaphoreSlimThrottle CreateBinarySemaphore() | |||
| { | |||
| return new SemaphoreSlimThrottle(new SemaphoreSlim(1)); | |||
There was a problem hiding this comment.
This can use CreateSemaphoreThrottle(initialCount: 1)
| @@ -2813,6 +2821,75 @@ public void LoadPackageSources_DoesNotLoadClearedSource() | |||
| } | |||
| } | |||
|
|
|||
| [Fact] | |||
| public void LoadPackageSources_SetMaxHttpRequest() | |||
There was a problem hiding this comment.
Could we have a functional test against Mock server that actually verifies the expected behavior?
| @@ -19,6 +19,7 @@ public static class SettingsUtility | |||
| private const string HttpCacheEnvironmentKey = "NUGET_HTTP_CACHE_PATH"; | |||
| private const string PluginsCacheEnvironmentKey = "NUGET_PLUGINS_CACHE_PATH"; | |||
| private const string RepositoryPathKey = "repositoryPath"; | |||
| private const string MaxHttpRequest = "maxHttpRequest"; | |||
There was a problem hiding this comment.
This name is a little ambiguous... could this somehow reflect concurrency?
There was a problem hiding this comment.
because this not really impact concurrency, we still have same operation level concurrency, but limit the http request number per source, NPM is using "maxSockets" for this feature, so I think "maxHttpRequest" may be more accurate for this flag.
There was a problem hiding this comment.
This still doesn't give the right picture, since this config is per source so I'd expect the name reflect that meaning. It should clearly state that this is max http requests per source
|
Is there a corresponding work item for documentation update? |
| var throttle = Throttle ?? NullThrottle.Instance; | ||
| if (Throttle != null) | ||
| { | ||
| throttle = Throttle; |
There was a problem hiding this comment.
our docs should clearly says that --disable-parallel takes precedent over MaxHttpRequests
| { | ||
| throttle = Throttle; | ||
| } | ||
| else if (source.PackageSource.MaxHttpRequest > 0) |
There was a problem hiding this comment.
you can move it under IsHttp check
| await Task.WhenAll(tasks); | ||
|
|
||
| // Assert | ||
| Assert.True(tc.MessageHandler.MaxConcurrencyRequest <= 4); |
There was a problem hiding this comment.
I'm not following this test, by this line, all tasks have been awaited so wouldn't it always be 0?
Ideally you should check for MaxConcurrencyRequest before await Task.WhenAll(tasks);
There was a problem hiding this comment.
discussed offline, we never decrement MaxConcurrencyRequest so once it reaches 4 it stays as is. So it should be good.
|
NuGet/Home#7190 for doc change |
| return result; | ||
| } | ||
|
|
||
| return 0; |
There was a problem hiding this comment.
should the default be 1 instead?
There was a problem hiding this comment.
no, if this not set, we will not set throttle, unlimited is the default.
zhili1208
force-pushed
the
dev-zhi-maxrequest
branch
2 times, most recently
from
6bc0ee6
to
6e384e0
Compare
zhili1208
force-pushed
the
dev-zhi-maxrequest
branch
from
cb4103d
to
31f29d0
Compare
|
I think this merge with only 1 approval was a bit premature because of how tricky this topic is. |
|
With which version has this been released? I tried to add ... <config>
<add key='maxHttpRequestsPerSource' value='2' />
</config>... to my user NuGet.config and have still hundreds of connections during |
|
looks like these 2 commits are interesting. The tags on those commits say that they were in nuget 5.0 and later. Release Notes for NuGet 5.0 seem to indicate that sdk 2.1.602 and later should have this change. |
|
@rrelyea Many thanks for your reply! I have the VS 2017 version of the 2.1 SDK (2.1.514) which seems to bind NuGet 4.9.x :( Any chance to get this backported to NuGet 4.9? |
|
We have not backported any non security fixes to 4.9. So it is unlikely. |
use maxHttpRequest from nuget.config to throttle http resource
NuGet/Home#4538