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

Tear down pending HTTP connection when the originating request completes #71785

Merged
merged 35 commits into from
Jul 26, 2022
Merged
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
f0e3e7a
cancel pending connections when request is cancelled
antonfirsov Jul 7, 2022
c4a6919
delete old methods
antonfirsov Jul 7, 2022
b11e3b1
move CancelPendingRequest_DropsStalledConnectionAttempt_WithGlobalTim…
antonfirsov Jul 7, 2022
ae278a7
comments
antonfirsov Jul 7, 2022
e746278
language
antonfirsov Jul 7, 2022
f4b7765
language
antonfirsov Jul 7, 2022
95a446d
minor simplification
antonfirsov Jul 7, 2022
10b3203
attempt to fix CA1852 on wasm targets
antonfirsov Jul 7, 2022
5db3229
better logging in CheckForHttp*ConnectionInjection
antonfirsov Jul 8, 2022
b792264
cleanup
antonfirsov Jul 8, 2022
cc3a274
Merge branch 'main' into cancel-stalled-connections
antonfirsov Jul 11, 2022
a8d4ad2
nits
antonfirsov Jul 11, 2022
49b1811
synchronize access to ConnectionCTS
antonfirsov Jul 11, 2022
b70b722
TryGetPooledHttp2Connection out params
antonfirsov Jul 12, 2022
8fe1df9
add comment on CancelAfter
antonfirsov Jul 12, 2022
4887411
skip CancelPendingRequest_DropsStalledConnectionAttempt on HTTP/3 for…
antonfirsov Jul 12, 2022
dafbde5
address feedback
antonfirsov Jul 12, 2022
c190074
compact catch block
antonfirsov Jul 12, 2022
7323f83
Merge branch 'main' into cancel-stalled-connections
antonfirsov Jul 12, 2022
6388b7d
SkipTestException doesn't seem to work on CI
antonfirsov Jul 12, 2022
bda66af
log cancellation reason
antonfirsov Jul 13, 2022
218c82b
default PendingConnectionTimeoutOnRequestCompletion to 15 sec
antonfirsov Jul 19, 2022
e86115f
update tests
antonfirsov Jul 19, 2022
fb28662
Merge branch 'main' into cancel-stalled-connections
antonfirsov Jul 20, 2022
5458fe0
wip
antonfirsov Jul 20, 2022
480ec1e
tests
antonfirsov Jul 21, 2022
2dad3a5
change default timeout to 5sec & special-case infinite timeout
antonfirsov Jul 21, 2022
2faede2
better errors and tests
antonfirsov Jul 21, 2022
a31ba97
borrow test fix from #72615
antonfirsov Jul 21, 2022
c005994
forward original OCE when cancelledByOriginatingRequestCompletion
antonfirsov Jul 21, 2022
1a20776
delete sandbox logging code
antonfirsov Jul 21, 2022
4b5aa02
actually set CancelledByOriginatingRequestCompletion
antonfirsov Jul 21, 2022
cf5bee8
simplify connection exception code
antonfirsov Jul 21, 2022
dbdbbc3
fix assertion failure (hopefully)
antonfirsov Jul 25, 2022
785f375
Merge branch 'main' into cancel-stalled-connections
antonfirsov Jul 25, 2022
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 @@ -8,7 +8,7 @@ namespace System.Threading.Tasks
/// <seealso cref="OperationCanceledException"/>s contain the relevant <see cref="CancellationToken"/>,
/// while also avoiding unnecessary allocations for closure captures.
/// </summary>
internal sealed class TaskCompletionSourceWithCancellation<T> : TaskCompletionSource<T>
internal class TaskCompletionSourceWithCancellation<T> : TaskCompletionSource<T>
{
public TaskCompletionSourceWithCancellation() : base(TaskCreationOptions.RunContinuationsAsynchronously)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,9 @@ public abstract class GenericLoopbackConnection : IAsyncDisposable
/// <summary>Waits for the client to signal cancellation.</summary>
public abstract Task WaitForCloseAsync(CancellationToken cancellationToken);

/// <summary>Reset the connection's internal state so it can process further requests.</summary>
public virtual void CompleteRequestProcessing() { }

/// <summary>Helper function to make it easier to convert old test with strings.</summary>
public async Task SendResponseBodyAsync(string content, bool isFinal = true)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -873,7 +873,7 @@ public override async Task<Byte[]> ReadRequestBodyAsync()
return buffer;
}

public void CompleteRequestProcessing()
public override void CompleteRequestProcessing()
{
_contentLength = 0;
_bodyRead = false;
Expand Down
4 changes: 2 additions & 2 deletions src/libraries/System.Net.Http/src/System.Net.Http.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,6 @@
Link="Common\System\Net\HttpDateParser.cs" />
<Compile Include="$(CommonPath)System\Text\SimpleRegex.cs"
Link="Common\System\Text\SimpleRegex.cs" />
<Compile Include="$(CommonPath)System\Threading\Tasks\TaskCompletionSourceWithCancellation.cs"
Link="Common\System\Threading\Tasks\TaskCompletionSourceWithCancellation.cs" />
<Compile Include="$(CommonPath)System\HexConverter.cs"
Link="Common\System\HexConverter.cs" />
<Compile Include="$(CommonPath)System\Net\ArrayBuffer.cs"
Expand Down Expand Up @@ -226,6 +224,8 @@
Link="Common\System\Net\DebugSafeHandle.cs" />
<Compile Include="$(CommonPath)System\Net\DebugSafeHandleZeroOrMinusOneIsInvalid.cs"
Link="Common\System\Net\DebugSafeHandleZeroOrMinusOneIsInvalid.cs" />
<Compile Include="$(CommonPath)System\Threading\Tasks\TaskCompletionSourceWithCancellation.cs"
Link="Common\System\Threading\Tasks\TaskCompletionSourceWithCancellation.cs" />
<!-- Header support -->
<Compile Include="$(CommonPath)System\Net\Http\aspnetcore\IHttpStreamHeadersHandler.cs">
<Link>Common\System\Net\Http\aspnetcore\IHttpStreamHeadersHandler.cs</Link>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ internal static class SocketsHttpHandler
// Defaults to 1.0. Higher values result in shorter window, but slower downloads.
public static double Http2StreamWindowScaleThresholdMultiplier { get; } = GetHttp2StreamWindowScaleThresholdMultiplier();

public static int PendingConnectionTimeoutOnRequestCompletion { get; } = RuntimeSettingParser.QueryRuntimeSettingInt32(
"System.Net.SocketsHttpHandler.PendingConnectionTimeoutOnRequestCompletion",
antonfirsov marked this conversation as resolved.
Show resolved Hide resolved
"DOTNET_SYSTEM_NET_HTTP_SOCKETSHTTPHANDLER_PENDINGCONNECTIONTIMEOUTONREQUESTCOMPLETION", 5000);

public const int DefaultHttp2MaxStreamWindowSize = 16 * 1024 * 1024;
public const double DefaultHttp2StreamWindowScaleThresholdMultiplier = 1.0;

Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -821,7 +821,7 @@ public async Task ConnectTimeout_NotWrappedInMultipleTimeoutExceptions()
Assert.Contains("ConnectTimeout", connectTimeoutException.Message);

Assert.Null(connectTimeoutException.InnerException);
Assert.DoesNotContain("42", e.ToString());
Assert.DoesNotContain("HttpClient.Timeout", e.ToString());
}

[Fact]
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.IO;
using System.Net.Sockets;
using System.Net.Test.Common;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.DotNet.RemoteExecutor;
using Xunit;
using Xunit.Abstractions;

namespace System.Net.Http.Functional.Tests
{
[Collection(nameof(DisableParallelization))] // Reduces chance of timing-related issues
[ConditionalClass(typeof(SocketsHttpHandler), nameof(SocketsHttpHandler.IsSupported))]
public class SocketsHttpHandler_Cancellation_Test_NonParallel : HttpClientHandlerTestBase
{
public SocketsHttpHandler_Cancellation_Test_NonParallel(ITestOutputHelper output) : base(output)
{
}

[OuterLoop("Incurs significant delay.")]
[ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
[InlineData("1.1", 10_000, 1_000, 100)]
[InlineData("2.0", 10_000, 1_000, 100)]
[InlineData("1.1", 20_000, 10_000, null)]
[InlineData("2.0", 20_000, 10_000, null)]
public static void CancelPendingRequest_DropsStalledConnectionAttempt(string versionString, int firstConnectionDelayMs, int requestTimeoutMs, int? pendingConnectionTimeoutOnRequestCompletion)
{
RemoteInvokeOptions options = new RemoteInvokeOptions();
if (pendingConnectionTimeoutOnRequestCompletion is not null)
{
options.StartInfo.EnvironmentVariables["DOTNET_SYSTEM_NET_HTTP_SOCKETSHTTPHANDLER_PENDINGCONNECTIONTIMEOUTONREQUESTCOMPLETION"] = pendingConnectionTimeoutOnRequestCompletion.ToString();
}

RemoteExecutor.Invoke(CancelPendingRequest_DropsStalledConnectionAttempt_Impl, versionString, firstConnectionDelayMs.ToString(), requestTimeoutMs.ToString(), options).Dispose();
}

private static async Task CancelPendingRequest_DropsStalledConnectionAttempt_Impl(string versionString, string firstConnectionDelayMsString, string requestTimeoutMsString)
{
var version = Version.Parse(versionString);
LoopbackServerFactory factory = GetFactoryForVersion(version);

const int AttemptCount = 3;
int firstConnectionDelayMs = int.Parse(firstConnectionDelayMsString);
int requestTimeoutMs = int.Parse(requestTimeoutMsString);
bool firstConnection = true;

using CancellationTokenSource cts0 = new CancellationTokenSource(requestTimeoutMs);

await factory.CreateClientAndServerAsync(async uri =>
{
using var handler = CreateHttpClientHandler(version);
GetUnderlyingSocketsHttpHandler(handler).ConnectCallback = DoConnect;
using var client = new HttpClient(handler) { DefaultRequestVersion = version };

await Assert.ThrowsAnyAsync<TaskCanceledException>(async () =>
{
await client.GetAsync(uri, cts0.Token);
});

for (int i = 0; i < AttemptCount; i++)
{
using var cts1 = new CancellationTokenSource(requestTimeoutMs);
using var response = await client.GetAsync(uri, cts1.Token);
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
}
}, async server =>
{
await server.AcceptConnectionAsync(async connection =>
{
for (int i = 0; i < AttemptCount; i++)
{
await connection.ReadRequestDataAsync();
await connection.SendResponseAsync();
connection.CompleteRequestProcessing();
}
});
});

async ValueTask<Stream> DoConnect(SocketsHttpConnectionContext ctx, CancellationToken cancellationToken)
{
if (firstConnection)
{
firstConnection = false;
await Task.Delay(100, cancellationToken); // Wait for the request to be pushed to the queue
cts0.Cancel(); // cancel the first request faster than RequestTimeoutMs
await Task.Delay(firstConnectionDelayMs, cancellationToken); // Simulate stalled connection
}
var s = new Socket(SocketType.Stream, ProtocolType.Tcp) { NoDelay = true };
await s.ConnectAsync(ctx.DnsEndPoint, cancellationToken);

return new NetworkStream(s, ownsSocket: true);
}
}

[OuterLoop("Incurs significant delay.")]
[ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
[InlineData(20_000)]
[InlineData(Timeout.Infinite)]
public void PendingConnectionTimeout_HighValue_PendingConnectionIsNotCancelled(int timeout)
{
RemoteExecutor.Invoke(async timoutStr =>
{
// Setup "infinite" timeout of int.MaxValue milliseconds
AppContext.SetData("System.Net.SocketsHttpHandler.PendingConnectionTimeoutOnRequestCompletion", int.Parse(timoutStr));

bool connected = false;
CancellationTokenSource cts = new CancellationTokenSource();

await new Http11LoopbackServerFactory().CreateClientAndServerAsync(async uri =>
{
using var handler = CreateHttpClientHandler(HttpVersion.Version11);
GetUnderlyingSocketsHttpHandler(handler).ConnectCallback = DoConnect;
using var client = new HttpClient(handler) { DefaultRequestVersion = HttpVersion.Version11 };

await Assert.ThrowsAnyAsync<TaskCanceledException>(() => client.GetAsync(uri, cts.Token));
},
async server => {
await server.AcceptConnectionAsync(_ => Task.CompletedTask).WaitAsync(30_000);
});

async ValueTask<Stream> DoConnect(SocketsHttpConnectionContext ctx, CancellationToken cancellationToken)
{
var s = new Socket(SocketType.Stream, ProtocolType.Tcp) { NoDelay = true };
await Task.Delay(100, cancellationToken); // Wait for the request to be pushed to the queue
cts.Cancel();

await Task.Delay(10_000, cancellationToken);
await s.ConnectAsync(ctx.DnsEndPoint, cancellationToken);
connected = true;
return new NetworkStream(s, ownsSocket: true);
}

Assert.True(connected);
}, timeout.ToString()).Dispose();
}
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<StringResourcesPath>../../src/Resources/Strings.resx</StringResourcesPath>
<DefineConstants>$(DefineConstants);SYSNETHTTP_NO_OPENSSL;HTTP3</DefineConstants>
Expand Down Expand Up @@ -157,6 +157,7 @@
Link="Common\TestUtilities\System\DisableParallelization.cs" />
<Compile Include="HttpClientHandlerTest.AltSvc.cs" />
<Compile Include="SocketsHttpHandlerTest.Cancellation.cs" />
<Compile Include="SocketsHttpHandlerTest.Cancellation.NonParallel.cs" />
<Compile Include="SocketsHttpHandlerTest.Http2FlowControl.cs" />
<Compile Include="SocketsHttpHandlerTest.Http2KeepAlivePing.cs" />
<Compile Include="HttpClientHandlerTest.Connect.cs" />
Expand Down