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

Increase HttpClientHandler.MaxConnectionsPerServer to 64 to improve PM UI performance in Visual Studio #4798

Merged
merged 11 commits into from
Sep 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,13 @@ private static HttpHandlerResourceV3 CreateResource(PackageSource packageSource)
AutomaticDecompression = (DecompressionMethods.GZip | DecompressionMethods.Deflate)
};

#if IS_DESKTOP
Copy link
Member

Choose a reason for hiding this comment

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

If people set the MaxHTTPRequestPerSource explicitly we should always respect that.

Doesn't this break that in dotnet.exe?

Copy link
Contributor Author

@kartheekp-ms kartheekp-ms Sep 16, 2022

Choose a reason for hiding this comment

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

If customers have configured MaxHttpRequestsPerSource then throttling requests is controlled by following code in both .NET and .NET Framework code paths. we don't change the value to 64 (which is new default).

else if (source.PackageSource.MaxHttpRequestsPerSource > 0)
{
throttle = SemaphoreSlimThrottle.CreateSemaphoreThrottle(source.PackageSource.MaxHttpRequestsPerSource);
}

The only difference is, in .NET Framework code path if customers configure MaxHttpRequestsPerSource then above logic controls throttling requests and we also set HttpClientHandler.MaxConnectionsPerServer to MaxHttpRequestsPerSource value. The reason is customers may expect NuGet to send that many requests to a source in parallel but at the end because of the default MaxConnectionsPerServer only 2 requests are sent. Whereas in .NET code paths above logic controls throttling requests and we don't MaxConnectionsPerServer because its default value is Int32.MaxValue.

if (packageSource.MaxHttpRequestsPerSource > 0)
{
clientHandler.MaxConnectionsPerServer = packageSource.MaxHttpRequestsPerSource;
}
#endif

// Setup http client handler client certificates
if (packageSource.ClientCertificates != null)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Collections.Concurrent;
using System.Diagnostics;
using System.Net;
using System.Threading;
using System.Threading.Tasks;
using NuGet.Configuration;
Expand All @@ -13,6 +14,9 @@ namespace NuGet.Protocol
{
public class HttpSourceResourceProvider : ResourceProvider
{
#if IS_DESKTOP
private const int DefaultMaxHttpRequestsPerSource = 64;
#endif
// Only one HttpSource per source should exist. This is to reduce the number of TCP connections.
private readonly ConcurrentDictionary<PackageSource, HttpSourceResource> _cache
= new ConcurrentDictionary<PackageSource, HttpSourceResource>();
Expand Down Expand Up @@ -47,6 +51,12 @@ public override Task<Tuple<bool, INuGetResource>> TryCreate(SourceRepository sou
{
throttle = SemaphoreSlimThrottle.CreateSemaphoreThrottle(source.PackageSource.MaxHttpRequestsPerSource);
}
#if IS_DESKTOP
else if (ServicePointManager.DefaultConnectionLimit == ServicePointManager.DefaultPersistentConnectionLimit)
{
source.PackageSource.MaxHttpRequestsPerSource = DefaultMaxHttpRequestsPerSource;
}
#endif

curResource = _cache.GetOrAdd(
source.PackageSource,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Collections.Generic;
using System.Threading.Tasks;
using NuGet.Configuration;
using NuGet.Protocol.Core.Types;
using NuGet.Test.Utility;
using Xunit;

namespace NuGet.Protocol.Tests
{
public class HttpHandlerResourceV3ProviderTests
{
private readonly string _testPackageSourceURL = "https://contoso.test/v3/index.json";

#if IS_DESKTOP
[PlatformFact(Platform.Windows)]
public async Task DefaultMaxHttpRequestsPerSourceIsForwardedToV3HttpClientHandler_SuccessAsync()
{
// Arrange
var packageSource = new PackageSource(_testPackageSourceURL);
var sourceRepository = new SourceRepository(packageSource, new List<INuGetResourceProvider>() { new HttpSourceResourceProvider(), new HttpHandlerResourceV3Provider() });

// HttpSourceResourceProvider updates PackageSource.MaxHttpRequestsPerSource value for .NET Framework code paths
// HttpSource constructor accepts a delegate that creates HttpHandlerResource and it stores the delegate in a private variable.
// Hence used discard to ignore the return value and fetched HttpHandlerResource from the source repository to verify behavior.
_ = await sourceRepository.GetResourceAsync<HttpSourceResource>();

// Act
HttpHandlerResource httpHandlerResource = await sourceRepository.GetResourceAsync<HttpHandlerResource>();

// Assert
Assert.NotNull(httpHandlerResource);
Assert.Equal(64, httpHandlerResource.ClientHandler.MaxConnectionsPerServer);
}

[PlatformTheory(Platform.Windows)]
[InlineData(128)]
[InlineData(256)]
public async Task PackageSourceMaxHttpRequestsPerSourceIsForwardedToV3HttpClientHandler_SuccessAsync(int maxHttpRequestsPerSource)
{
// Arrange
var packageSource = new PackageSource(_testPackageSourceURL) { MaxHttpRequestsPerSource = maxHttpRequestsPerSource };
var sourceRepository = new SourceRepository(packageSource, new List<INuGetResourceProvider>() { new HttpSourceResourceProvider(), new HttpHandlerResourceV3Provider() });

// HttpSourceResourceProvider updates PackageSource.MaxHttpRequestsPerSource value for .NET Framework code paths
// HttpSource constructor accepts a delegate that creates HttpHandlerResource and it stores the delegate in a private variable.
// Hence used discard to ignore the return value and fetched HttpHandlerResource from the source repository to verify behavior.
_ = await sourceRepository.GetResourceAsync<HttpSourceResource>();

// Act
HttpHandlerResource httpHandlerResource = await sourceRepository.GetResourceAsync<HttpHandlerResource>();

// Assert
Assert.NotNull(httpHandlerResource);
Assert.Equal(maxHttpRequestsPerSource, httpHandlerResource.ClientHandler.MaxConnectionsPerServer);
}
#elif IS_CORECLR

[Theory]
[InlineData(64)]
[InlineData(128)]
[InlineData(2)]
public async Task PackageSourceMaxHttpRequestsPerSourceIsNotForwardedToV3HttpClientHandler_SuccessAsync(int maxHttpRequestsPerSource)
{
// Arrange
var packageSource = new PackageSource(_testPackageSourceURL) { MaxHttpRequestsPerSource = maxHttpRequestsPerSource };
var sourceRepository = new SourceRepository(packageSource, new[] { new HttpHandlerResourceV3Provider() });

// HttpSourceResourceProvider updates PackageSource.MaxHttpRequestsPerSource value for .NET Framework code paths
// HttpSource constructor accepts a delegate that creates HttpHandlerResource and it stores the delegate in a private variable.
// Hence used discard to ignore the return value and fetched HttpHandlerResource from the source repository to verify behavior.
_ = await sourceRepository.GetResourceAsync<HttpSourceResource>();

// Act
HttpHandlerResource httpHandlerResource = await sourceRepository.GetResourceAsync<HttpHandlerResource>();

// Assert
Assert.NotNull(httpHandlerResource);
Assert.NotEqual(maxHttpRequestsPerSource, httpHandlerResource.ClientHandler.MaxConnectionsPerServer);
}
#endif
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Threading.Tasks;
using NuGet.Configuration;
using NuGet.Protocol.Core.Types;
using NuGet.Test.Utility;
using Xunit;

namespace NuGet.Protocol.Tests
{
public class HttpSourceResourceProviderTests
{
private readonly string _testPackageSourceURL = "https://contoso.test/v3/index.json";

#if IS_DESKTOP
[PlatformFact(Platform.Windows)]
public async Task WhenMaxHttpRequestsPerSourceIsNotConfiguredThenItsValueIsSetToDefault_SuccessAsync()
{
// Arrange
var packageSource = new PackageSource(_testPackageSourceURL);
var sourceRepository = new SourceRepository(packageSource, new[] { new HttpSourceResourceProvider() });

// Act
HttpSourceResource httpSourceResource = await sourceRepository.GetResourceAsync<HttpSourceResource>();

// Assert
Assert.NotNull(httpSourceResource);
Assert.Equal(64, sourceRepository.PackageSource.MaxHttpRequestsPerSource);
}
#elif IS_CORECLR
[Fact]
public async Task WhenMaxHttpRequestsPerSourceIsNotConfiguredThenItsValueWillNotBeUpdated_SuccessAsync()
{
// Arrange
var packageSource = new PackageSource(_testPackageSourceURL);
var sourceRepository = new SourceRepository(packageSource, new[] { new HttpSourceResourceProvider() });

// Act
HttpSourceResource httpSourceResource = await sourceRepository.GetResourceAsync<HttpSourceResource>();

// Assert
Assert.NotNull(httpSourceResource);
Assert.Equal(0, sourceRepository.PackageSource.MaxHttpRequestsPerSource);
}
#endif

[PlatformTheory]
[InlineData(128)]
[InlineData(256)]
public async Task WhenMaxHttpRequestsPerSourceIsConfiguredThenItsValueWillNotBeUpdated_SuccessAsync(int maxHttpRequestsPerSource)
{
// Arrange
var packageSource = new PackageSource(_testPackageSourceURL) { MaxHttpRequestsPerSource = maxHttpRequestsPerSource };
var sourceRepository = new SourceRepository(packageSource, new[] { new HttpSourceResourceProvider() });

// Act
HttpSourceResource httpSourceResource = await sourceRepository.GetResourceAsync<HttpSourceResource>();

// Assert
Assert.NotNull(httpSourceResource);
Assert.Equal(maxHttpRequestsPerSource, sourceRepository.PackageSource.MaxHttpRequestsPerSource);
}
}
}