-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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 connection pool size limit automatically #15263
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT License. | ||
|
||
using System.Net; | ||
using System.Net.Http; | ||
|
||
namespace Azure.Core.Pipeline | ||
{ | ||
internal static class ServicePointHelpers | ||
{ | ||
private const int RuntimeDefaultConnectionLimit = 2; | ||
private const int IncreasedConnectionLimit = 50; | ||
|
||
public static void SetLimits(ServicePoint requestServicePoint) | ||
{ | ||
// Only change when the default runtime limit is used | ||
if (requestServicePoint.ConnectionLimit == RuntimeDefaultConnectionLimit) | ||
{ | ||
requestServicePoint.ConnectionLimit = IncreasedConnectionLimit; | ||
} | ||
} | ||
|
||
public static void SetLimits(HttpClientHandler requestServicePoint) | ||
{ | ||
// Only change when the default runtime limit is used | ||
if (requestServicePoint.MaxConnectionsPerServer == RuntimeDefaultConnectionLimit) | ||
{ | ||
requestServicePoint.MaxConnectionsPerServer = IncreasedConnectionLimit; | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,12 +8,12 @@ | |
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Azure.Core.Pipeline; | ||
using Azure.Core.TestFramework; | ||
using Microsoft.AspNetCore.Http; | ||
using NUnit.Framework; | ||
|
||
namespace Azure.Core.Tests | ||
{ | ||
|
||
[TestFixture(typeof(HttpClientTransport), true)] | ||
[TestFixture(typeof(HttpClientTransport), false)] | ||
#if NETFRAMEWORK | ||
|
@@ -158,6 +158,82 @@ public async Task NonBufferedFailedResponsesAreDisposedOf() | |
Assert.Greater(reqNum, requestCount); | ||
} | ||
|
||
[Test] | ||
public async Task Opens50ParallelConnections() | ||
{ | ||
// Running 50 sync requests on the threadpool would cause starvation | ||
// and the test would take 20 sec to finish otherwise | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we doing anything to validate that we're not hitting starvation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really, this is just to speedup tests. Starvation is expected here because threadpool is smaller than 50 by-default and we are completely blocking threads. |
||
ThreadPool.SetMinThreads(100, 100); | ||
|
||
HttpPipeline httpPipeline = HttpPipelineBuilder.Build(GetOptions()); | ||
int reqNum = 0; | ||
|
||
TaskCompletionSource<object> requestsTcs = new TaskCompletionSource<object>(TaskCreationOptions.RunContinuationsAsynchronously); | ||
|
||
using TestServer testServer = new TestServer( | ||
async context => | ||
{ | ||
if (Interlocked.Increment(ref reqNum) == 50) | ||
{ | ||
requestsTcs.SetResult(true); | ||
} | ||
|
||
await requestsTcs.Task; | ||
}); | ||
|
||
var requestCount = 50; | ||
List<Task> requests = new List<Task>(); | ||
for (int i = 0; i < requestCount; i++) | ||
{ | ||
HttpMessage message = httpPipeline.CreateMessage(); | ||
message.Request.Uri.Reset(testServer.Address); | ||
|
||
requests.Add(Task.Run(() => ExecuteRequest(message, httpPipeline))); | ||
} | ||
|
||
await Task.WhenAll(requests); | ||
} | ||
|
||
[Test] | ||
[Category("Live")] | ||
public async Task Opens50ParallelConnectionsLive() | ||
{ | ||
// Running 50 sync requests on the threadpool would cause starvation | ||
// and the test would take 20 sec to finish otherwise | ||
ThreadPool.SetMinThreads(100, 100); | ||
|
||
HttpPipeline httpPipeline = HttpPipelineBuilder.Build(GetOptions()); | ||
int reqNum = 0; | ||
|
||
TaskCompletionSource<object> requestsTcs = new TaskCompletionSource<object>(TaskCreationOptions.RunContinuationsAsynchronously); | ||
|
||
async Task Connect() | ||
{ | ||
using HttpMessage message = httpPipeline.CreateMessage(); | ||
message.Request.Uri.Reset(new Uri("https://www.microsoft.com/")); | ||
message.BufferResponse = false; | ||
|
||
await ExecuteRequest(message, httpPipeline); | ||
|
||
if (Interlocked.Increment(ref reqNum) == 50) | ||
{ | ||
requestsTcs.SetResult(true); | ||
} | ||
|
||
await requestsTcs.Task; | ||
} | ||
|
||
var requestCount = 50; | ||
List<Task> requests = new List<Task>(); | ||
for (int i = 0; i < requestCount; i++) | ||
{ | ||
|
||
requests.Add(Task.Run(() => Connect())); | ||
} | ||
|
||
await Task.WhenAll(requests); | ||
} | ||
|
||
[Test] | ||
public async Task BufferedResponsesReadableAfterMessageDisposed() | ||
{ | ||
|
@@ -176,7 +252,6 @@ public async Task BufferedResponsesReadableAfterMessageDisposed() | |
} | ||
}); | ||
|
||
// Make sure we dispose things correctly and not exhaust the connection pool | ||
var requestCount = 100; | ||
for (int i = 0; i < requestCount; i++) | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would there be any value in allowing this to be overridden by an environment variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or should we let it override through
Azure.Core.ClientOptions
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would start without it and see if anyone cares.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To add some context, there are already global ways to set this value via ServicePointManager or app config.
And I'm not sure we want to add it to options because it's hard to make it work with custom-provided transports.