From 8feb8e12671f0d62d2b33ba15decd9257933a326 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Mon, 8 May 2023 14:15:00 -0700 Subject: [PATCH 1/6] Make KestrelConfigurationLoader.Load safe to re-call If it is called again and nothing has changed, it should do nothing. Part of #45801 --- .../Core/src/KestrelConfigurationLoader.cs | 17 ++- .../Kestrel/Core/test/KestrelServerTests.cs | 2 +- .../test/KestrelConfigurationLoaderTests.cs | 120 ++++++++++++++++++ 3 files changed, 131 insertions(+), 8 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs b/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs index fdc16bd7cc45..3059a445441a 100644 --- a/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs +++ b/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs @@ -8,6 +8,7 @@ using Microsoft.AspNetCore.Server.Kestrel.Core.Internal; using Microsoft.AspNetCore.Server.Kestrel.Https; using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Primitives; namespace Microsoft.AspNetCore.Server.Kestrel; @@ -18,7 +19,7 @@ public class KestrelConfigurationLoader { private readonly IHttpsConfigurationService _httpsConfigurationService; - private bool _loaded; + private IChangeToken? _reloadToken; internal KestrelConfigurationLoader( KestrelServerOptions options, @@ -234,25 +235,27 @@ internal void ApplyHttpsDefaults(HttpsConnectionAdapterOptions httpsOptions) /// public void Load() { - if (_loaded) + if (_reloadToken is null || _reloadToken.HasChanged) { - // The loader has already been run. - return; + // Will update _reloadToken + _ = Reload(); } - _loaded = true; - - Reload(); foreach (var action in EndpointsToAdd) { action(); } + + // If Load is called again, we don't want to rerun these + EndpointsToAdd.Clear(); } // Adds endpoints from config to KestrelServerOptions.ConfigurationBackedListenOptions and configures some other options. // Any endpoints that were removed from the last time endpoints were loaded are returned. internal (List, List) Reload() { + _reloadToken = Configuration.GetReloadToken(); + var endpointsToStop = Options.ConfigurationBackedListenOptions.ToList(); var endpointsToStart = new List(); var endpointsToReuse = new List(); diff --git a/src/Servers/Kestrel/Core/test/KestrelServerTests.cs b/src/Servers/Kestrel/Core/test/KestrelServerTests.cs index a6415f735e77..3a646c7d9e27 100644 --- a/src/Servers/Kestrel/Core/test/KestrelServerTests.cs +++ b/src/Servers/Kestrel/Core/test/KestrelServerTests.cs @@ -958,7 +958,7 @@ public async Task DoesNotReloadOnConfigurationChangeByDefault() mockTransportFactory.Verify(f => f.BindAsync(new IPEndPoint(IPAddress.IPv6Any, 5000), It.IsAny()), Times.Once); mockTransportFactory.Verify(f => f.BindAsync(new IPEndPoint(IPAddress.IPv6Any, 5001), It.IsAny()), Times.Once); - mockConfig.Verify(c => c.GetReloadToken(), Times.Never); + mockConfig.Verify(c => c.GetReloadToken(), Times.Once); // Grabbed on Load await server.StopAsync(CancellationToken.None).DefaultTimeout(); } diff --git a/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs b/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs index 5b033b4952ef..45ba587aa6ee 100644 --- a/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs +++ b/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs @@ -14,6 +14,8 @@ using Microsoft.Extensions.FileProviders; using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Metrics; +using Microsoft.Extensions.Primitives; +using Moq; namespace Microsoft.AspNetCore.Server.Kestrel.Tests; @@ -1224,6 +1226,124 @@ public void Reload_RerunsNamedEndpointConfigurationOnChange() Assert.Equal(1, foundUnchangedCount); } + [Fact] + public void MultipleLoads_ConfigureBetween() + { + var currentConfig = new ConfigurationBuilder().AddInMemoryCollection(new[] + { + new KeyValuePair("Endpoints:A:Url", "http://*:5000"), + new KeyValuePair("Endpoints:B:Url", "http://*:5001"), + }).Build(); + + var mockConfig = new Mock(); + mockConfig.Setup(c => c.GetSection(It.IsAny())).Returns(currentConfig.GetSection); + mockConfig.Setup(c => c.GetChildren()).Returns(currentConfig.GetChildren); + + var serverOptions = CreateServerOptions(); + serverOptions.Configure(mockConfig.Object, reloadOnChange: false); + var oldConfigurationLoader = serverOptions.ConfigurationLoader; + + serverOptions.ConfigurationLoader.Load(); + + mockConfig.Verify(c => c.GetSection(It.IsAny()), Times.AtLeastOnce); + + mockConfig.Invocations.Clear(); + + serverOptions.Configure(mockConfig.Object, reloadOnChange: false); + var newConfigurationLoader = serverOptions.ConfigurationLoader; + Assert.NotSame(oldConfigurationLoader, newConfigurationLoader); + + serverOptions.ConfigurationLoader.Load(); + + // The change token is stored on the loader, so replacing the loader replaces the token. + mockConfig.Verify(c => c.GetSection(It.IsAny()), Times.AtLeastOnce); + } + + [Theory] + [InlineData(true, true)] + [InlineData(false, true)] + [InlineData(true, false)] + [InlineData(false, false)] + public void MultipleLoads_ConfigurationChanges(bool changeBeforeInitialLoad, bool changeAfterInitialLoad) + { + var currentConfig = new ConfigurationBuilder().AddInMemoryCollection(new[] + { + new KeyValuePair("Endpoints:A:Url", "http://*:5000"), + new KeyValuePair("Endpoints:B:Url", "http://*:5001"), + }).Build(); + + var mockReloadToken = new Mock(); + mockReloadToken.SetupGet(t => t.HasChanged).Returns(changeBeforeInitialLoad); + + var mockConfig = new Mock(); + mockConfig.Setup(c => c.GetSection(It.IsAny())).Returns(currentConfig.GetSection); + mockConfig.Setup(c => c.GetChildren()).Returns(currentConfig.GetChildren); + mockConfig.Setup(c => c.GetReloadToken()).Returns(mockReloadToken.Object); + + var serverOptions = CreateServerOptions(); + serverOptions.Configure(mockConfig.Object, reloadOnChange: false); + + serverOptions.ConfigurationLoader.Load(); + + mockReloadToken.VerifyGet(t => t.HasChanged, Times.AtLeastOnce); + mockConfig.Verify(c => c.GetSection(It.IsAny()), Times.AtLeastOnce); + + mockReloadToken.SetupGet(t => t.HasChanged).Returns(changeAfterInitialLoad); + + mockReloadToken.Invocations.Clear(); + mockConfig.Invocations.Clear(); + + serverOptions.ConfigurationLoader.Load(); + + mockReloadToken.VerifyGet(t => t.HasChanged, Times.AtLeastOnce); + mockConfig.Verify(c => c.GetSection(It.IsAny()), changeAfterInitialLoad ? Times.AtLeastOnce : Times.Never); + } + + [Theory] + [InlineData(0, 0)] + [InlineData(3, 0)] + [InlineData(2, 1)] + [InlineData(1, 2)] + [InlineData(0, 3)] + public void MultipleLoads_AddEndpoints(int beforeInitialLoadCount, int afterInitialLoadCount) + { + int _endpointAddedCount = 0; + + var serverOptions = CreateServerOptions(); + serverOptions.Configure(); + + for (int i = 0; i < beforeInitialLoadCount; i++) + { + serverOptions.ConfigurationLoader.LocalhostEndpoint(5000 + i, _ => _endpointAddedCount++); + } + + serverOptions.ConfigurationLoader.Load(); + + Assert.Equal(beforeInitialLoadCount, _endpointAddedCount); + + for (int i = 0; i < afterInitialLoadCount; i++) + { + serverOptions.ConfigurationLoader.LocalhostEndpoint(7000 + i, _ => _endpointAddedCount++); + } + + serverOptions.ConfigurationLoader.Load(); + + Assert.Equal(beforeInitialLoadCount + afterInitialLoadCount, _endpointAddedCount); + } + + [Fact] + public void ReloadDoesNotAddEndpoints() + { + var serverOptions = CreateServerOptions(); + serverOptions.Configure(); + + serverOptions.ConfigurationLoader.Load(); + + serverOptions.ConfigurationLoader.LocalhostEndpoint(7000, _ => Assert.Fail("New endpoints should not be added by Reload")); + + _ = serverOptions.ConfigurationLoader.Reload(); + } + private static string GetCertificatePath() { var appData = Environment.GetEnvironmentVariable("APPDATA"); From ee1fecaf91f36d51981830f7391dae55504e88f6 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Mon, 8 May 2023 14:32:37 -0700 Subject: [PATCH 2/6] Fix copy-paste error --- .../Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs b/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs index 45ba587aa6ee..7d6c5c0be29d 100644 --- a/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs +++ b/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs @@ -1285,7 +1285,7 @@ public void MultipleLoads_ConfigurationChanges(bool changeBeforeInitialLoad, boo serverOptions.ConfigurationLoader.Load(); - mockReloadToken.VerifyGet(t => t.HasChanged, Times.AtLeastOnce); + mockReloadToken.VerifyGet(t => t.HasChanged, Times.Never); mockConfig.Verify(c => c.GetSection(It.IsAny()), Times.AtLeastOnce); mockReloadToken.SetupGet(t => t.HasChanged).Returns(changeAfterInitialLoad); From 8cccd0097f551645012d21153bfabcb67983718b Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Tue, 9 May 2023 10:55:24 -0700 Subject: [PATCH 3/6] Update tests based on PR feedback --- .../Kestrel/Core/test/KestrelServerTests.cs | 2 -- .../test/KestrelConfigurationLoaderTests.cs | 17 +++++++++-------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/src/Servers/Kestrel/Core/test/KestrelServerTests.cs b/src/Servers/Kestrel/Core/test/KestrelServerTests.cs index 3a646c7d9e27..dd1864a78639 100644 --- a/src/Servers/Kestrel/Core/test/KestrelServerTests.cs +++ b/src/Servers/Kestrel/Core/test/KestrelServerTests.cs @@ -958,8 +958,6 @@ public async Task DoesNotReloadOnConfigurationChangeByDefault() mockTransportFactory.Verify(f => f.BindAsync(new IPEndPoint(IPAddress.IPv6Any, 5000), It.IsAny()), Times.Once); mockTransportFactory.Verify(f => f.BindAsync(new IPEndPoint(IPAddress.IPv6Any, 5001), It.IsAny()), Times.Once); - mockConfig.Verify(c => c.GetReloadToken(), Times.Once); // Grabbed on Load - await server.StopAsync(CancellationToken.None).DefaultTimeout(); } diff --git a/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs b/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs index 7d6c5c0be29d..a29fe50b5bc0 100644 --- a/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs +++ b/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs @@ -1260,11 +1260,9 @@ public void MultipleLoads_ConfigureBetween() } [Theory] - [InlineData(true, true)] - [InlineData(false, true)] - [InlineData(true, false)] - [InlineData(false, false)] - public void MultipleLoads_ConfigurationChanges(bool changeBeforeInitialLoad, bool changeAfterInitialLoad) + [InlineData(true)] + [InlineData(false)] + public void MultipleLoads_ConfigurationChanges(bool changeConfiguration) { var currentConfig = new ConfigurationBuilder().AddInMemoryCollection(new[] { @@ -1273,7 +1271,6 @@ public void MultipleLoads_ConfigurationChanges(bool changeBeforeInitialLoad, boo }).Build(); var mockReloadToken = new Mock(); - mockReloadToken.SetupGet(t => t.HasChanged).Returns(changeBeforeInitialLoad); var mockConfig = new Mock(); mockConfig.Setup(c => c.GetSection(It.IsAny())).Returns(currentConfig.GetSection); @@ -1288,7 +1285,7 @@ public void MultipleLoads_ConfigurationChanges(bool changeBeforeInitialLoad, boo mockReloadToken.VerifyGet(t => t.HasChanged, Times.Never); mockConfig.Verify(c => c.GetSection(It.IsAny()), Times.AtLeastOnce); - mockReloadToken.SetupGet(t => t.HasChanged).Returns(changeAfterInitialLoad); + mockReloadToken.SetupGet(t => t.HasChanged).Returns(changeConfiguration); mockReloadToken.Invocations.Clear(); mockConfig.Invocations.Clear(); @@ -1296,7 +1293,7 @@ public void MultipleLoads_ConfigurationChanges(bool changeBeforeInitialLoad, boo serverOptions.ConfigurationLoader.Load(); mockReloadToken.VerifyGet(t => t.HasChanged, Times.AtLeastOnce); - mockConfig.Verify(c => c.GetSection(It.IsAny()), changeAfterInitialLoad ? Times.AtLeastOnce : Times.Never); + mockConfig.Verify(c => c.GetSection(It.IsAny()), changeConfiguration ? Times.AtLeastOnce : Times.Never); } [Theory] @@ -1320,6 +1317,8 @@ public void MultipleLoads_AddEndpoints(int beforeInitialLoadCount, int afterInit serverOptions.ConfigurationLoader.Load(); Assert.Equal(beforeInitialLoadCount, _endpointAddedCount); + Assert.Equal(beforeInitialLoadCount, serverOptions.CodeBackedListenOptions.Count); + Assert.Empty(serverOptions.ConfigurationBackedListenOptions); for (int i = 0; i < afterInitialLoadCount; i++) { @@ -1329,6 +1328,8 @@ public void MultipleLoads_AddEndpoints(int beforeInitialLoadCount, int afterInit serverOptions.ConfigurationLoader.Load(); Assert.Equal(beforeInitialLoadCount + afterInitialLoadCount, _endpointAddedCount); + Assert.Equal(beforeInitialLoadCount + afterInitialLoadCount, serverOptions.CodeBackedListenOptions.Count); + Assert.Empty(serverOptions.ConfigurationBackedListenOptions); } [Fact] From 64a2ca6d226924a0eb10079c246b000af51a4b5d Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 10 May 2023 18:59:09 -0700 Subject: [PATCH 4/6] Factor LoadInternal and ProcessEndpointsToAdd out of Load ...so that `Load` can retain its current behavior. Also, only consume the change token when `ReloadOnChange` is true. --- .../Core/src/Internal/KestrelServerImpl.cs | 3 +- .../Core/src/KestrelConfigurationLoader.cs | 56 +++- .../test/KestrelConfigurationLoaderTests.cs | 278 ++++++++++++++---- 3 files changed, 278 insertions(+), 59 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/KestrelServerImpl.cs b/src/Servers/Kestrel/Core/src/Internal/KestrelServerImpl.cs index c6f2e2b3a0ec..3fe65ebb4101 100644 --- a/src/Servers/Kestrel/Core/src/Internal/KestrelServerImpl.cs +++ b/src/Servers/Kestrel/Core/src/Internal/KestrelServerImpl.cs @@ -304,7 +304,8 @@ private async Task BindAsync(CancellationToken cancellationToken) reloadToken = Options.ConfigurationLoader.Configuration.GetReloadToken(); } - Options.ConfigurationLoader?.Load(); + Options.ConfigurationLoader?.LoadInternal(); + Options.ConfigurationLoader?.ProcessEndpointsToAdd(); await AddressBinder.BindAsync(Options.GetListenOptions(), AddressBindContext!, _httpsConfigurationService.UseHttpsWithDefaults, cancellationToken).ConfigureAwait(false); _configChangedRegistration = reloadToken?.RegisterChangeCallback(TriggerRebind, this); diff --git a/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs b/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs index 3059a445441a..123226690bf2 100644 --- a/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs +++ b/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs @@ -1,8 +1,10 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Diagnostics; using System.Linq; using System.Net; +using System.Runtime.CompilerServices; using System.Security.Cryptography.X509Certificates; using Microsoft.AspNetCore.Server.Kestrel.Core; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal; @@ -19,6 +21,9 @@ public class KestrelConfigurationLoader { private readonly IHttpsConfigurationService _httpsConfigurationService; + private bool _loaded; + private bool _endpointsToAddProcessed; + private IChangeToken? _reloadToken; internal KestrelConfigurationLoader( @@ -230,31 +235,66 @@ internal void ApplyHttpsDefaults(HttpsConnectionAdapterOptions httpsOptions) } } + // Note: This method is obsolete, but we have to keep it around to avoid breaking the public API. + // Internally, we should always use . /// - /// Loads the configuration. + /// Loads the configuration. Does nothing if it has previously been invoked (including implicitly). /// public void Load() { - if (_reloadToken is null || _reloadToken.HasChanged) + if (!_loaded) { - // Will update _reloadToken - _ = Reload(); + LoadInternal(); } + // Has its own logic for skipping subsequent invocations + ProcessEndpointsToAdd(); + } + + /// + /// Always prefer this to since it can be called repeatedly and no-ops if + /// there's a change token indicating nothing has changed. + /// + internal void LoadInternal() + { + if (!_loaded || ReloadOnChange) + { + Debug.Assert(!!_loaded || _reloadToken is null, "Shouldn't have a reload token before first load"); + Debug.Assert(!!ReloadOnChange || _reloadToken is null, "Shouldn't have a reload token unless reload-on-change is set"); + + _loaded = true; + + if (_reloadToken is null || _reloadToken.HasChanged) + { + // Will update _reloadToken + _ = Reload(); + } + } + } + + internal void ProcessEndpointsToAdd() + { + if (_endpointsToAddProcessed) + { + return; + } + // Set this *before* invoking delegates, in case one throws + _endpointsToAddProcessed = true; + foreach (var action in EndpointsToAdd) { action(); } - - // If Load is called again, we don't want to rerun these - EndpointsToAdd.Clear(); } // Adds endpoints from config to KestrelServerOptions.ConfigurationBackedListenOptions and configures some other options. // Any endpoints that were removed from the last time endpoints were loaded are returned. internal (List, List) Reload() { - _reloadToken = Configuration.GetReloadToken(); + if (ReloadOnChange) + { + _reloadToken = Configuration.GetReloadToken(); + } var endpointsToStop = Options.ConfigurationBackedListenOptions.ToList(); var endpointsToStart = new List(); diff --git a/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs b/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs index a29fe50b5bc0..965f1fe841f3 100644 --- a/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs +++ b/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs @@ -35,6 +35,26 @@ private KestrelServerOptions CreateServerOptions() return serverOptions; } + private static Mock CreateMockConfiguration() => CreateMockConfiguration(out _); + + private static Mock CreateMockConfiguration(out Mock mockReloadToken) + { + var currentConfig = new ConfigurationBuilder().AddInMemoryCollection(new[] +{ + new KeyValuePair("Endpoints:A:Url", "http://*:5000"), + new KeyValuePair("Endpoints:B:Url", "http://*:5001"), + }).Build(); + + mockReloadToken = new Mock(); + + var mockConfig = new Mock(); + mockConfig.Setup(c => c.GetSection(It.IsAny())).Returns(currentConfig.GetSection); + mockConfig.Setup(c => c.GetChildren()).Returns(currentConfig.GetChildren); + mockConfig.Setup(c => c.GetReloadToken()).Returns(mockReloadToken.Object); + + return mockConfig; + } + [Fact] public void ConfigureNamedEndpoint_OnlyRunForMatchingConfig() { @@ -1226,24 +1246,52 @@ public void Reload_RerunsNamedEndpointConfigurationOnChange() Assert.Equal(1, foundUnchangedCount); } - [Fact] - public void MultipleLoads_ConfigureBetween() + [Theory] + [InlineData(true, true)] + [InlineData(true, false)] + [InlineData(false, true)] + [InlineData(false, false)] + public void MultipleLoads_Consecutive(bool loadInternal, bool reloadOnChange) { - var currentConfig = new ConfigurationBuilder().AddInMemoryCollection(new[] - { - new KeyValuePair("Endpoints:A:Url", "http://*:5000"), - new KeyValuePair("Endpoints:B:Url", "http://*:5001"), - }).Build(); + var serverOptions = CreateServerOptions(); + var mockConfig = CreateMockConfiguration(); + serverOptions.Configure(mockConfig.Object, reloadOnChange); - var mockConfig = new Mock(); - mockConfig.Setup(c => c.GetSection(It.IsAny())).Returns(currentConfig.GetSection); - mockConfig.Setup(c => c.GetChildren()).Returns(currentConfig.GetChildren); + Action load = loadInternal ? serverOptions.ConfigurationLoader.LoadInternal : serverOptions.ConfigurationLoader.Load; + + load(); + mockConfig.Verify(c => c.GetSection(It.IsAny()), Times.AtLeastOnce); + + mockConfig.Invocations.Clear(); + + load(); + + // In any case, nothing has changed, so nothing is read + mockConfig.Verify(c => c.GetSection(It.IsAny()), Times.Never); + } + + [Theory] + [InlineData(true, true)] + [InlineData(true, false)] + [InlineData(false, true)] + [InlineData(false, false)] + public void MultipleLoads_ConfigureBetween(bool loadInternal, bool reloadOnChange) + { var serverOptions = CreateServerOptions(); - serverOptions.Configure(mockConfig.Object, reloadOnChange: false); + var mockConfig = CreateMockConfiguration(); + serverOptions.Configure(mockConfig.Object, reloadOnChange); + var oldConfigurationLoader = serverOptions.ConfigurationLoader; - - serverOptions.ConfigurationLoader.Load(); + + if (loadInternal) + { + serverOptions.ConfigurationLoader.LoadInternal(); + } + else + { + serverOptions.ConfigurationLoader.Load(); + } mockConfig.Verify(c => c.GetSection(It.IsAny()), Times.AtLeastOnce); @@ -1253,85 +1301,215 @@ public void MultipleLoads_ConfigureBetween() var newConfigurationLoader = serverOptions.ConfigurationLoader; Assert.NotSame(oldConfigurationLoader, newConfigurationLoader); - serverOptions.ConfigurationLoader.Load(); + if (loadInternal) + { + serverOptions.ConfigurationLoader.LoadInternal(); + } + else + { + serverOptions.ConfigurationLoader.Load(); + } - // The change token is stored on the loader, so replacing the loader replaces the token. + // In any case, the configuration loader has been replaced, so this is a "first" load mockConfig.Verify(c => c.GetSection(It.IsAny()), Times.AtLeastOnce); } [Theory] - [InlineData(true)] - [InlineData(false)] - public void MultipleLoads_ConfigurationChanges(bool changeConfiguration) + [InlineData(true, true)] + [InlineData(true, false)] + [InlineData(false, true)] + [InlineData(false, false)] + public void MultipleLoadInternals_ConfigurationChanges(bool loadInternal, bool reloadOnChange) { - var currentConfig = new ConfigurationBuilder().AddInMemoryCollection(new[] - { - new KeyValuePair("Endpoints:A:Url", "http://*:5000"), - new KeyValuePair("Endpoints:B:Url", "http://*:5001"), - }).Build(); + var serverOptions = CreateServerOptions(); + var mockConfig = CreateMockConfiguration(out var mockReloadToken); + serverOptions.Configure(mockConfig.Object, reloadOnChange); - var mockReloadToken = new Mock(); + Action load = loadInternal ? serverOptions.ConfigurationLoader.LoadInternal : serverOptions.ConfigurationLoader.Load; - var mockConfig = new Mock(); - mockConfig.Setup(c => c.GetSection(It.IsAny())).Returns(currentConfig.GetSection); - mockConfig.Setup(c => c.GetChildren()).Returns(currentConfig.GetChildren); - mockConfig.Setup(c => c.GetReloadToken()).Returns(mockReloadToken.Object); + load(); + + mockReloadToken.VerifyGet(t => t.HasChanged, Times.Never); + mockConfig.Verify(c => c.GetSection(It.IsAny()), Times.AtLeastOnce); + + mockReloadToken.SetupGet(t => t.HasChanged).Returns(true); + + mockReloadToken.Invocations.Clear(); + mockConfig.Invocations.Clear(); + + load(); + Func reloadTimes = loadInternal && reloadOnChange ? Times.AtLeastOnce : Times.Never; + + mockReloadToken.VerifyGet(t => t.HasChanged, reloadTimes); + mockConfig.Verify(c => c.GetSection(It.IsAny()), reloadTimes); + } + + [Fact] + public void LoadInternalBeforeLoad() + { var serverOptions = CreateServerOptions(); - serverOptions.Configure(mockConfig.Object, reloadOnChange: false); + var mockConfig = CreateMockConfiguration(out var mockReloadToken); + serverOptions.Configure(mockConfig.Object, reloadOnChange: true); - serverOptions.ConfigurationLoader.Load(); + serverOptions.ConfigurationLoader.LoadInternal(); mockReloadToken.VerifyGet(t => t.HasChanged, Times.Never); mockConfig.Verify(c => c.GetSection(It.IsAny()), Times.AtLeastOnce); - mockReloadToken.SetupGet(t => t.HasChanged).Returns(changeConfiguration); + mockReloadToken.SetupGet(t => t.HasChanged).Returns(true); mockReloadToken.Invocations.Clear(); mockConfig.Invocations.Clear(); + serverOptions.ConfigurationLoader.LocalhostEndpoint(5000); + serverOptions.ConfigurationLoader.Load(); + mockReloadToken.VerifyGet(t => t.HasChanged, Times.Never); + mockConfig.Verify(c => c.GetSection(It.IsAny()), Times.Never); + Assert.Single(serverOptions.CodeBackedListenOptions); // Still have to process endpoints + } + + [Fact] + public void LoadInternalAfterLoad() + { + var serverOptions = CreateServerOptions(); + var mockConfig = CreateMockConfiguration(out var mockReloadToken); + serverOptions.Configure(mockConfig.Object, reloadOnChange: true); + + serverOptions.ConfigurationLoader.Load(); + + mockReloadToken.VerifyGet(t => t.HasChanged, Times.Never); + mockConfig.Verify(c => c.GetSection(It.IsAny()), Times.AtLeastOnce); + + mockReloadToken.SetupGet(t => t.HasChanged).Returns(true); + + mockReloadToken.Invocations.Clear(); + mockConfig.Invocations.Clear(); + + serverOptions.ConfigurationLoader.LoadInternal(); + mockReloadToken.VerifyGet(t => t.HasChanged, Times.AtLeastOnce); - mockConfig.Verify(c => c.GetSection(It.IsAny()), changeConfiguration ? Times.AtLeastOnce : Times.Never); + mockConfig.Verify(c => c.GetSection(It.IsAny()), Times.AtLeastOnce); } - [Theory] - [InlineData(0, 0)] - [InlineData(3, 0)] - [InlineData(2, 1)] - [InlineData(1, 2)] - [InlineData(0, 3)] - public void MultipleLoads_AddEndpoints(int beforeInitialLoadCount, int afterInitialLoadCount) + [Fact] + public void ProcessEndpointsToAdd() { - int _endpointAddedCount = 0; + int numEndpointsToAdd = 3; + int numEndpointsAdded = 0; var serverOptions = CreateServerOptions(); serverOptions.Configure(); - for (int i = 0; i < beforeInitialLoadCount; i++) + for (int i = 0; i < numEndpointsToAdd; i++) { - serverOptions.ConfigurationLoader.LocalhostEndpoint(5000 + i, _ => _endpointAddedCount++); + serverOptions.ConfigurationLoader.LocalhostEndpoint(5000 + i, _ => numEndpointsAdded++); } - serverOptions.ConfigurationLoader.Load(); + serverOptions.ConfigurationLoader.ProcessEndpointsToAdd(); - Assert.Equal(beforeInitialLoadCount, _endpointAddedCount); - Assert.Equal(beforeInitialLoadCount, serverOptions.CodeBackedListenOptions.Count); + Assert.Equal(numEndpointsToAdd, numEndpointsAdded); + Assert.Equal(numEndpointsToAdd, serverOptions.CodeBackedListenOptions.Count); Assert.Empty(serverOptions.ConfigurationBackedListenOptions); - for (int i = 0; i < afterInitialLoadCount; i++) + // Adding more endpoints and calling again has no effect + + for (int i = 0; i < numEndpointsToAdd; i++) { - serverOptions.ConfigurationLoader.LocalhostEndpoint(7000 + i, _ => _endpointAddedCount++); + serverOptions.ConfigurationLoader.LocalhostEndpoint(6000 + i, _ => numEndpointsAdded++); } - serverOptions.ConfigurationLoader.Load(); + serverOptions.ConfigurationLoader.ProcessEndpointsToAdd(); + + Assert.Equal(numEndpointsToAdd, numEndpointsAdded); + Assert.Equal(numEndpointsToAdd, serverOptions.CodeBackedListenOptions.Count); + Assert.Empty(serverOptions.ConfigurationBackedListenOptions); + } + + [Fact] + public void ProcessEndpointsToAdd_CallbackThrows() + { + int numEndpointsAdded = 0; + + var serverOptions = CreateServerOptions(); + serverOptions.Configure(); + + serverOptions.ConfigurationLoader.LocalhostEndpoint(5000, _ => numEndpointsAdded++); + serverOptions.ConfigurationLoader.LocalhostEndpoint(5001, _ => throw new InvalidOperationException()); + serverOptions.ConfigurationLoader.LocalhostEndpoint(5002, _ => numEndpointsAdded++); + + Assert.Throws(serverOptions.ConfigurationLoader.ProcessEndpointsToAdd); + + Assert.Equal(1, numEndpointsAdded); + Assert.Single(serverOptions.CodeBackedListenOptions); + Assert.Empty(serverOptions.ConfigurationBackedListenOptions); + + serverOptions.ConfigurationLoader.ProcessEndpointsToAdd(); - Assert.Equal(beforeInitialLoadCount + afterInitialLoadCount, _endpointAddedCount); - Assert.Equal(beforeInitialLoadCount + afterInitialLoadCount, serverOptions.CodeBackedListenOptions.Count); + // As in success scenarios, the second call has no effect + Assert.Equal(1, numEndpointsAdded); + Assert.Single(serverOptions.CodeBackedListenOptions); Assert.Empty(serverOptions.ConfigurationBackedListenOptions); } + [Fact] + public void ProcessEndpointsToAddBeforeLoad() + { + var serverOptions = CreateServerOptions(); + var mockConfig = CreateMockConfiguration(); + serverOptions.Configure(mockConfig.Object); + + serverOptions.ConfigurationLoader.LocalhostEndpoint(5000); + + serverOptions.ConfigurationLoader.ProcessEndpointsToAdd(); + + Assert.Single(serverOptions.CodeBackedListenOptions); + mockConfig.Verify(c => c.GetSection(It.IsNotIn("EndpointDefaults")), Times.Never); // It does read the EndpointDefaults sections + + mockConfig.Invocations.Clear(); + + serverOptions.ConfigurationLoader.LocalhostEndpoint(7000, _ => Assert.Fail("New endpoints should not be added after ProcessEndpointsToAdd")); + + serverOptions.ConfigurationLoader.Load(); + + Assert.Single(serverOptions.CodeBackedListenOptions); + mockConfig.Verify(c => c.GetSection(It.IsAny()), Times.AtLeastOnce); // Still need to load, even if endpoints have been processed + } + + [Fact] + public void ProcessEndpointsToAddAfterLoad() + { + var serverOptions = CreateServerOptions(); + var mockConfig = CreateMockConfiguration(); + serverOptions.Configure(mockConfig.Object); + + serverOptions.ConfigurationLoader.LocalhostEndpoint(5000); + + serverOptions.ConfigurationLoader.Load(); + + Assert.Single(serverOptions.CodeBackedListenOptions); + + mockConfig.Invocations.Clear(); + + serverOptions.ConfigurationLoader.LocalhostEndpoint(7000, _ => Assert.Fail("New endpoints should not be added after Load")); + + Assert.Single(serverOptions.CodeBackedListenOptions); + serverOptions.ConfigurationLoader.ProcessEndpointsToAdd(); + } + + [Fact] + public void LoadInternalDoesNotAddEndpoints() + { + var serverOptions = CreateServerOptions(); + serverOptions.Configure(); + + serverOptions.ConfigurationLoader.LocalhostEndpoint(7000, _ => Assert.Fail("New endpoints should not be added by LoadInternal")); + + serverOptions.ConfigurationLoader.LoadInternal(); + } + [Fact] public void ReloadDoesNotAddEndpoints() { From d1970ae4a324bc506a625673c2568b355371c330 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 11 May 2023 17:42:20 -0700 Subject: [PATCH 5/6] Remove unused using --- src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs b/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs index 123226690bf2..2a36d8001b4b 100644 --- a/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs +++ b/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs @@ -4,7 +4,6 @@ using System.Diagnostics; using System.Linq; using System.Net; -using System.Runtime.CompilerServices; using System.Security.Cryptography.X509Certificates; using Microsoft.AspNetCore.Server.Kestrel.Core; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal; From aff0268680e8f9a349ac0476b43d325e4f363877 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 11 May 2023 17:46:10 -0700 Subject: [PATCH 6/6] Revert KestrelServerTests --- src/Servers/Kestrel/Core/test/KestrelServerTests.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Servers/Kestrel/Core/test/KestrelServerTests.cs b/src/Servers/Kestrel/Core/test/KestrelServerTests.cs index dd1864a78639..a6415f735e77 100644 --- a/src/Servers/Kestrel/Core/test/KestrelServerTests.cs +++ b/src/Servers/Kestrel/Core/test/KestrelServerTests.cs @@ -958,6 +958,8 @@ public async Task DoesNotReloadOnConfigurationChangeByDefault() mockTransportFactory.Verify(f => f.BindAsync(new IPEndPoint(IPAddress.IPv6Any, 5000), It.IsAny()), Times.Once); mockTransportFactory.Verify(f => f.BindAsync(new IPEndPoint(IPAddress.IPv6Any, 5001), It.IsAny()), Times.Once); + mockConfig.Verify(c => c.GetReloadToken(), Times.Never); + await server.StopAsync(CancellationToken.None).DefaultTimeout(); }