Skip to content

PR feedback from #11412 #11474

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 24, 2019
Merged

PR feedback from #11412 #11474

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
38 changes: 38 additions & 0 deletions src/Servers/Kestrel/Core/test/AddressBinderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,19 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.IO;
using System.Net;
using System.Net.Sockets;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Connections;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Server.Kestrel.Core.Internal;
using Microsoft.AspNetCore.Server.Kestrel.Https;
using Microsoft.AspNetCore.Testing;
using Microsoft.AspNetCore.Testing.xunit;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging.Abstractions;
using Xunit;

Expand Down Expand Up @@ -149,5 +153,39 @@ await AddressBinder.BindAsync(addresses,
Assert.True(ipV6Attempt, "Should have attempted to bind to IPAddress.IPv6Any");
Assert.Contains(logger.Messages, f => f.Equals(CoreStrings.FormatFallbackToIPv4Any(80)));
}

[Fact]
public async Task DefaultAddressBinderWithoutDevCertButHttpsConfiguredBindsToHttpsPorts()
{
var x509Certificate2 = TestResources.GetTestCertificate();
var logger = new MockLogger();
var addresses = new ServerAddressesFeature();
var services = new ServiceCollection();
services.AddLogging();
var options = new KestrelServerOptions()
{
// This stops the dev cert from being loaded
IsDevCertLoaded = true,
ApplicationServices = services.BuildServiceProvider()
};

options.ConfigureEndpointDefaults(e =>
{
if (e.IPEndPoint.Port == 5001)
Copy link
Member

Choose a reason for hiding this comment

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

If you didn't have IsDevCertLoaded = true, above, this test would pass even without the custom ConfigureEndpointDefaults, right?

I think this test would be a little more convincing if it made port 5000 the TLS endpoint.

Copy link
Member Author

Choose a reason for hiding this comment

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

The default binding only checks 5001 for IsTls

{
e.UseHttps(new HttpsConnectionAdapterOptions { ServerCertificate = x509Certificate2 });
}
});

var endpoints = new List<ListenOptions>();
await AddressBinder.BindAsync(addresses, options, logger, listenOptions =>
{
endpoints.Add(listenOptions);
return Task.CompletedTask;
});

Assert.Contains(endpoints, e => e.IPEndPoint.Port == 5000 && !e.IsTls);
Assert.Contains(endpoints, e => e.IPEndPoint.Port == 5001 && e.IsTls);
}
}
}
0 ...ared/test/PassThroughConnectionAdapter.cs → ...d/test/PassThroughConnectionMiddleware.cs
View file
Open in desktop
File renamed without changes.
2 changes: 0 additions & 2 deletions src/Servers/Kestrel/shared/test/TestServiceContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,6 @@ private void Initialize(ILoggerFactory loggerFactory, IKestrelTrace kestrelTrace

public Func<MemoryPool<byte>> MemoryPoolFactory { get; set; } = System.Buffers.SlabMemoryPoolFactory.Create;

public int ExpectedConnectionMiddlewareCount { get; set; }

public string DateHeaderValue => DateHeaderValueManager.GetDateHeaderValues().String;
}
}
5 changes: 0 additions & 5 deletions src/Servers/Kestrel/shared/test/TransportTestHelpers/TestServer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,11 +89,6 @@ public TestServer(RequestDelegate app, TestServiceContext context, Action<Kestre
c.Configure(context.ServerOptions);
}

// Prevent ListenOptions reuse. This is easily done accidentally when trying to debug a test by running it
// in a loop, but will cause problems because only the app func from the first loop will ever be invoked.
Assert.All(context.ServerOptions.ListenOptions, lo =>
Assert.Equal(context.ExpectedConnectionMiddlewareCount, lo._middleware.Count));

return new KestrelServer(sp.GetRequiredService<IConnectionListenerFactory>(), context);
});
configureServices(services);
Expand Down
6 changes: 3 additions & 3 deletions ...FunctionalTests/ConnectionAdapterTests.cs → ...ctionalTests/ConnectionMiddlewareTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,15 @@

namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
{
public class ConnectionAdapterTests : LoggedTest
public class ConnectionMiddlewareTests : LoggedTest
{
[Fact]
public async Task ThrowingSynchronousConnectionAdapterDoesNotCrashServer()
public async Task ThrowingSynchronousConnectionMiddlewareDoesNotCrashServer()
{
var listenOptions = new ListenOptions(new IPEndPoint(IPAddress.Loopback, 0));
listenOptions.Use(next => context => throw new Exception());

var serviceContext = new TestServiceContext(LoggerFactory) { ExpectedConnectionMiddlewareCount = 1 };
var serviceContext = new TestServiceContext(LoggerFactory);

using (var server = new TestServer(TestApp.EchoApp, serviceContext, listenOptions))
{
Expand Down
6 changes: 3 additions & 3 deletions src/Servers/Kestrel/test/FunctionalTests/Http2/HandshakeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public void TlsAndHttp2NotSupportedOnMac()
var ex = Assert.Throws<NotSupportedException>(() => new TestServer(context =>
{
throw new NotImplementedException();
}, new TestServiceContext(LoggerFactory) { ExpectedConnectionMiddlewareCount = 1 },
}, new TestServiceContext(LoggerFactory),
kestrelOptions =>
{
kestrelOptions.Listen(IPAddress.Loopback, 0, listenOptions =>
Expand All @@ -71,7 +71,7 @@ public async Task TlsAlpnHandshakeSelectsHttp2From1and2()
"ALPN: " + tlsFeature.ApplicationProtocol.Length);

return context.Response.WriteAsync("hello world " + context.Request.Protocol);
}, new TestServiceContext(LoggerFactory) { ExpectedConnectionMiddlewareCount = 1 },
}, new TestServiceContext(LoggerFactory),
kestrelOptions =>
{
kestrelOptions.Listen(IPAddress.Loopback, 0, listenOptions =>
Expand Down Expand Up @@ -102,7 +102,7 @@ public async Task TlsAlpnHandshakeSelectsHttp2()
"ALPN: " + tlsFeature.ApplicationProtocol.Length);

return context.Response.WriteAsync("hello world " + context.Request.Protocol);
}, new TestServiceContext(LoggerFactory) { ExpectedConnectionMiddlewareCount = 1 },
}, new TestServiceContext(LoggerFactory),
kestrelOptions =>
{
kestrelOptions.Listen(IPAddress.Loopback, 0, listenOptions =>
Expand Down
5 changes: 2 additions & 3 deletions src/Servers/Kestrel/test/FunctionalTests/Http2/ShutdownTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public async Task GracefulShutdownWaitsForRequestsToFinish()
.Setup(m => m.Http2ConnectionClosing(It.IsAny<string>()))
.Callback(() => requestStopping.SetResult(null));

var testContext = new TestServiceContext(LoggerFactory, mockKestrelTrace.Object) { ExpectedConnectionMiddlewareCount = 1 };
var testContext = new TestServiceContext(LoggerFactory, mockKestrelTrace.Object);

testContext.InitializeHeartbeat();

Expand Down Expand Up @@ -112,8 +112,7 @@ public async Task GracefulTurnsAbortiveIfRequestsDoNotFinish()

var testContext = new TestServiceContext(LoggerFactory)
{
MemoryPoolFactory = memoryPoolFactory.Create,
ExpectedConnectionMiddlewareCount = 1
MemoryPoolFactory = memoryPoolFactory.Create
};

TestApplicationErrorLogger.ThrowOnUngracefulShutdown = false;
Expand Down
25 changes: 12 additions & 13 deletions src/Servers/Kestrel/test/FunctionalTests/RequestTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public class RequestTests : LoggedTest
private const int _connectionResetEventId = 19;
private static readonly int _semaphoreWaitTimeout = Debugger.IsAttached ? 10000 : 2500;

public static TheoryData<ListenOptions> ConnectionAdapterData => new TheoryData<ListenOptions>
public static TheoryData<ListenOptions> ConnectionMiddlewareData => new TheoryData<ListenOptions>
{
new ListenOptions(new IPEndPoint(IPAddress.Loopback, 0)),
new ListenOptions(new IPEndPoint(IPAddress.Loopback, 0)).UsePassThrough()
Expand Down Expand Up @@ -503,10 +503,10 @@ public async Task AbortingTheConnectionSendsFIN()
}

[Theory]
[MemberData(nameof(ConnectionAdapterData))]
[MemberData(nameof(ConnectionMiddlewareData))]
public async Task ConnectionClosedTokenFiresOnClientFIN(ListenOptions listenOptions)
{
var testContext = new TestServiceContext(LoggerFactory) { ExpectedConnectionMiddlewareCount = listenOptions._middleware.Count };
var testContext = new TestServiceContext(LoggerFactory);
var appStartedTcs = new TaskCompletionSource<object>(TaskCreationOptions.RunContinuationsAsynchronously);
var connectionClosedTcs = new TaskCompletionSource<object>(TaskCreationOptions.RunContinuationsAsynchronously);

Expand Down Expand Up @@ -540,10 +540,10 @@ await connection.Send(

[Theory]
[Flaky("https://github.com/aspnet/AspNetCore-Internal/issues/2181", FlakyOn.Helix.All)]
[MemberData(nameof(ConnectionAdapterData))]
[MemberData(nameof(ConnectionMiddlewareData))]
public async Task ConnectionClosedTokenFiresOnServerFIN(ListenOptions listenOptions)
{
var testContext = new TestServiceContext(LoggerFactory) { ExpectedConnectionMiddlewareCount = listenOptions._middleware.Count };
var testContext = new TestServiceContext(LoggerFactory);
var connectionClosedTcs = new TaskCompletionSource<object>(TaskCreationOptions.RunContinuationsAsynchronously);

using (var server = new TestServer(context =>
Expand Down Expand Up @@ -577,10 +577,10 @@ await connection.ReceiveEnd($"HTTP/1.1 200 OK",
}

[Theory]
[MemberData(nameof(ConnectionAdapterData))]
[MemberData(nameof(ConnectionMiddlewareData))]
public async Task ConnectionClosedTokenFiresOnServerAbort(ListenOptions listenOptions)
{
var testContext = new TestServiceContext(LoggerFactory) { ExpectedConnectionMiddlewareCount = listenOptions._middleware.Count };
var testContext = new TestServiceContext(LoggerFactory);
var connectionClosedTcs = new TaskCompletionSource<object>(TaskCreationOptions.RunContinuationsAsynchronously);

using (var server = new TestServer(context =>
Expand Down Expand Up @@ -619,13 +619,13 @@ await connection.Send(
}

[Theory]
[MemberData(nameof(ConnectionAdapterData))]
[MemberData(nameof(ConnectionMiddlewareData))]
public async Task RequestsCanBeAbortedMidRead(ListenOptions listenOptions)
{
// This needs a timeout.
const int applicationAbortedConnectionId = 34;

var testContext = new TestServiceContext(LoggerFactory) { ExpectedConnectionMiddlewareCount = listenOptions._middleware.Count };
var testContext = new TestServiceContext(LoggerFactory);

var readTcs = new TaskCompletionSource<object>(TaskCreationOptions.RunContinuationsAsynchronously);
var registrationTcs = new TaskCompletionSource<int>(TaskCreationOptions.RunContinuationsAsynchronously);
Expand Down Expand Up @@ -710,7 +710,7 @@ await connection.Send("POST / HTTP/1.1",
}

[Theory]
[MemberData(nameof(ConnectionAdapterData))]
[MemberData(nameof(ConnectionMiddlewareData))]
public async Task ServerCanAbortConnectionAfterUnobservedClose(ListenOptions listenOptions)
{
const int connectionPausedEventId = 4;
Expand Down Expand Up @@ -743,7 +743,6 @@ public async Task ServerCanAbortConnectionAfterUnobservedClose(ListenOptions lis
var mockKestrelTrace = new Mock<IKestrelTrace>();
var testContext = new TestServiceContext(LoggerFactory, mockKestrelTrace.Object)
{
ExpectedConnectionMiddlewareCount = listenOptions._middleware.Count,
ServerOptions =
{
Limits =
Expand Down Expand Up @@ -796,14 +795,14 @@ await connection.Send(
#if LIBUV
[Flaky("https://github.com/aspnet/AspNetCore-Internal/issues/1971", FlakyOn.Helix.All)]
#endif
[MemberData(nameof(ConnectionAdapterData))]
[MemberData(nameof(ConnectionMiddlewareData))]
public async Task AppCanHandleClientAbortingConnectionMidRequest(ListenOptions listenOptions)
{
var readTcs = new TaskCompletionSource<object>(TaskCreationOptions.RunContinuationsAsynchronously);
var appStartedTcs = new TaskCompletionSource<object>(TaskCreationOptions.RunContinuationsAsynchronously);

var mockKestrelTrace = new Mock<IKestrelTrace>();
var testContext = new TestServiceContext(LoggerFactory, mockKestrelTrace.Object) { ExpectedConnectionMiddlewareCount = listenOptions._middleware.Count };
var testContext = new TestServiceContext(LoggerFactory, mockKestrelTrace.Object);

var scratchBuffer = new byte[4096];

Expand Down
24 changes: 11 additions & 13 deletions src/Servers/Kestrel/test/FunctionalTests/ResponseTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
{
public class ResponseTests : TestApplicationErrorLoggerLoggedTest
{
public static TheoryData<ListenOptions> ConnectionAdapterData => new TheoryData<ListenOptions>
public static TheoryData<ListenOptions> ConnectionMiddlewareData => new TheoryData<ListenOptions>
{
new ListenOptions(new IPEndPoint(IPAddress.Loopback, 0)),
new ListenOptions(new IPEndPoint(IPAddress.Loopback, 0)).UsePassThrough()
Expand Down Expand Up @@ -136,7 +136,7 @@ public async Task IgnoreNullHeaderValues(string headerName, StringValues headerV
}

[Theory]
[MemberData(nameof(ConnectionAdapterData))]
[MemberData(nameof(ConnectionMiddlewareData))]
public async Task WriteAfterConnectionCloseNoops(ListenOptions listenOptions)
{
var connectionClosed = new TaskCompletionSource<object>(TaskCreationOptions.RunContinuationsAsynchronously);
Expand All @@ -157,7 +157,7 @@ public async Task WriteAfterConnectionCloseNoops(ListenOptions listenOptions)
{
appCompleted.TrySetException(ex);
}
}, new TestServiceContext(LoggerFactory) { ExpectedConnectionMiddlewareCount = listenOptions._middleware.Count }, listenOptions))
}, new TestServiceContext(LoggerFactory), listenOptions))
{
using (var connection = server.CreateConnection())
{
Expand All @@ -180,7 +180,7 @@ await connection.Send(
}

[Theory]
[MemberData(nameof(ConnectionAdapterData))]
[MemberData(nameof(ConnectionMiddlewareData))]
public async Task ThrowsOnWriteWithRequestAbortedTokenAfterRequestIsAborted(ListenOptions listenOptions)
{
// This should match _maxBytesPreCompleted in SocketOutput
Expand Down Expand Up @@ -219,7 +219,7 @@ public async Task ThrowsOnWriteWithRequestAbortedTokenAfterRequestIsAborted(List
}

writeTcs.SetException(new Exception("This shouldn't be reached."));
}, new TestServiceContext(LoggerFactory) { ExpectedConnectionMiddlewareCount = listenOptions._middleware.Count }, listenOptions))
}, new TestServiceContext(LoggerFactory), listenOptions))
{
using (var connection = server.CreateConnection())
{
Expand All @@ -244,7 +244,7 @@ await connection.Send(
}

[Theory]
[MemberData(nameof(ConnectionAdapterData))]
[MemberData(nameof(ConnectionMiddlewareData))]
public async Task WritingToConnectionAfterUnobservedCloseTriggersRequestAbortedToken(ListenOptions listenOptions)
{
const int connectionPausedEventId = 4;
Expand Down Expand Up @@ -273,7 +273,6 @@ public async Task WritingToConnectionAfterUnobservedCloseTriggersRequestAbortedT

var testContext = new TestServiceContext(LoggerFactory, mockKestrelTrace.Object)
{
ExpectedConnectionMiddlewareCount = listenOptions._middleware.Count,
ServerOptions =
{
Limits =
Expand Down Expand Up @@ -341,7 +340,7 @@ await connection.Send(

[Theory]
[Flaky("https://github.com/aspnet/AspNetCore-Internal/issues/1972", FlakyOn.All)]
[MemberData(nameof(ConnectionAdapterData))]
[MemberData(nameof(ConnectionMiddlewareData))]
public async Task AppCanHandleClientAbortingConnectionMidResponse(ListenOptions listenOptions)
{
const int connectionResetEventId = 19;
Expand All @@ -368,7 +367,7 @@ public async Task AppCanHandleClientAbortingConnectionMidResponse(ListenOptions

await requestAborted.Task.DefaultTimeout();
appCompletedTcs.SetResult(null);
}, new TestServiceContext(LoggerFactory) { ExpectedConnectionMiddlewareCount = listenOptions._middleware.Count }, listenOptions))
}, new TestServiceContext(LoggerFactory), listenOptions))
{
using (var connection = server.CreateConnection())
{
Expand Down Expand Up @@ -414,15 +413,15 @@ await connection.Send(
}

[Theory]
[MemberData(nameof(ConnectionAdapterData))]
[MemberData(nameof(ConnectionMiddlewareData))]
public async Task ClientAbortingConnectionImmediatelyIsNotLoggedHigherThanDebug(ListenOptions listenOptions)
{
// Attempt multiple connections to be extra sure the resets are consistently logged appropriately.
const int numConnections = 10;

// There's not guarantee that the app even gets invoked in this test. The connection reset can be observed
// as early as accept.
var testServiceContext = new TestServiceContext(LoggerFactory) { ExpectedConnectionMiddlewareCount = listenOptions._middleware.Count };
var testServiceContext = new TestServiceContext(LoggerFactory);
using (var server = new TestServer(context => Task.CompletedTask, testServiceContext, listenOptions))
{
for (var i = 0; i < numConnections; i++)
Expand Down Expand Up @@ -582,8 +581,7 @@ public async Task HttpsConnectionClosedWhenResponseDoesNotSatisfyMinimumDataRate
{
MinResponseDataRate = new MinDataRate(bytesPerSecond: 1024 * 1024, gracePeriod: TimeSpan.FromSeconds(2))
}
},
ExpectedConnectionMiddlewareCount = 1
}
};

testContext.InitializeHeartbeat();
Expand Down
5 changes: 1 addition & 4 deletions src/Servers/Kestrel/test/InMemory.FunctionalTests/ConnectionLimitTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,10 +207,7 @@ private TestServer CreateServerWithMaxConnections(RequestDelegate app, long max)

private TestServer CreateServerWithMaxConnections(RequestDelegate app, ResourceCounter concurrentConnectionCounter)
{
var serviceContext = new TestServiceContext(LoggerFactory)
{
ExpectedConnectionMiddlewareCount = 1
};
var serviceContext = new TestServiceContext(LoggerFactory);

var listenOptions = new ListenOptions(new IPEndPoint(IPAddress.Loopback, 0));
listenOptions.Use(next =>
Expand Down
Loading