More review feedback.

Author: halter73

src/Servers/Kestrel/Core/src/Internal/ServerAddressesCollection.cs

@@ -66,30 +66,6 @@ public void Clear()
             }
         }
 
-        public void InternalAdd(string item)
-        {
-            lock (_addresses)
-            {
-                _addresses.Add(item);
-            }
-        }
-
-        public bool InternalRemove(string item)
-        {
-            lock (_addresses)
-            {
-                return _addresses.Remove(item);
-            }
-        }
-
-        public void InternalClear()
-        {
-            lock (_addresses)
-            {
-                _addresses.Clear();
-            }
-        }
-
         public bool Contains(string item)
         {
             lock (_addresses)

src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs

@@ -24,21 +24,21 @@ public class KestrelConfigurationLoader
     {
         private bool _loaded = false;
 
-        internal KestrelConfigurationLoader(KestrelServerOptions options, IConfiguration configuration)
+        internal KestrelConfigurationLoader(KestrelServerOptions options, IConfiguration configuration, bool reloadOnChange)
         {
             Options = options ?? throw new ArgumentNullException(nameof(options));
             Configuration = configuration ?? throw new ArgumentNullException(nameof(configuration));
+            ReloadOnChange = reloadOnChange;
         }
 
         public KestrelServerOptions Options { get; }
         public IConfiguration Configuration { get; internal set; }
 
         /// <summary>
-        /// Gets or sets a boolean indicating if endpoints should be reconfigured when the configuration section changes.
-        /// Kestrel will bind and unbind to endpoints as necessary.
+        /// If <see langword="true" />, Kestrel will dynamically update endpoint bindings when configuration changes.
+        /// This will only reload endpoints defined in the "Endpoints" section of your Kestrel configuration. Endpoints defined in code will not be reloaded.
         /// </summary>
-        /// <remarks>If set the <see langword="true" />, Kestrel will dynamically update endpoint bindings when configuration changes. This will only reload endpoints defined in the "Endpoints" section of your Kestrel configuration. Endpoints defined in code will not be reloaded.</remarks>
-        public bool ReloadOnChange { get; } = true;
+        internal bool ReloadOnChange { get; }
 
         private ConfigurationReader ConfigurationReader { get; set; }
 
@@ -269,7 +269,7 @@ public void Load()
                 else
                 {
                     // Ensure endpoint is reloaded if it used the default protocol and the protocol changed.
-                    // listenOptions.Progocols should already be set to this by ApplyEndpointDefaults.
+                    // listenOptions.Protocols should already be set to this by ApplyEndpointDefaults.
                     endpoint.Protocols = ConfigurationReader.EndpointDefaults.Protocols;
                 }
 
@@ -345,16 +345,17 @@ private void LoadDefaultCert(ConfigurationReader configReader)
             else
             {
                 var logger = Options.ApplicationServices.GetRequiredService<ILogger<KestrelServer>>();
-                var certificate = FindDeveloperCertificateFile(configReader, logger);
+                var (certificate, certificateConfig) = FindDeveloperCertificateFile(configReader, logger);
                 if (certificate != null)
                 {
                     logger.LocatedDevelopmentCertificate(certificate);
+                    DefaultCertificateConfig = certificateConfig;
                     Options.DefaultCertificate = certificate;
                 }
             }
         }
 
-        private X509Certificate2 FindDeveloperCertificateFile(ConfigurationReader configReader, ILogger<KestrelServer> logger)
+        private (X509Certificate2, CertificateConfig) FindDeveloperCertificateFile(ConfigurationReader configReader, ILogger<KestrelServer> logger)
         {
             string certificatePath = null;
             try
@@ -369,13 +370,10 @@ private X509Certificate2 FindDeveloperCertificateFile(ConfigurationReader config
 
                     if (IsDevelopmentCertificate(certificate))
                     {
-                        DefaultCertificateConfig = certificateConfig;
-                        return certificate;
+                        return (certificate, certificateConfig);
                     }
-
-                    return null;
                 }
-                else if (!File.Exists(certificatePath))
+                else if (!string.IsNullOrEmpty(certificatePath))
                 {
                     logger.FailedToLocateDevelopmentCertificateFile(certificatePath);
                 }
@@ -385,7 +383,7 @@ private X509Certificate2 FindDeveloperCertificateFile(ConfigurationReader config
                 logger.FailedToLoadDevelopmentCertificate(certificatePath);
             }
 
-            return null;
+            return (null, null);
         }
 
         private static bool IsDevelopmentCertificate(X509Certificate2 certificate)

src/Servers/Kestrel/Core/src/KestrelServer.cs

@@ -294,10 +294,12 @@ private async Task RebindAsync()
                 reloadToken = Options.ConfigurationLoader.Configuration.GetReloadToken();
                 var (endpointsToStop, endpointsToStart) = Options.ConfigurationLoader.Reload();
 
+                Trace.LogDebug("Config reload token fired. Checking for changes...");
+
                 if (endpointsToStop.Count > 0)
                 {
                     var urlsToStop = endpointsToStop.Select(lo => lo.EndpointConfig.Url ?? "<unknown>");
-                    Trace.LogInformation("Config changed. Stopping the following endpoints: {endpoints}", string.Join(", ", urlsToStop));
+                    Trace.LogInformation("Config changed. Stopping the following endpoints: '{endpoints}'", string.Join("', '", urlsToStop));
 
                     // 5 is the default value for WebHost's "shutdownTimeoutSeconds", so use that.
                     using var timeoutCts = new CancellationTokenSource(TimeSpan.FromSeconds(5));
@@ -318,7 +320,7 @@ private async Task RebindAsync()
                 if (endpointsToStart.Count > 0)
                 {
                     var urlsToStart = endpointsToStart.Select(lo => lo.EndpointConfig.Url ?? "<unknown>");
-                    Trace.LogInformation("Config changed. Starting the following endpoints: {endpoints}", string.Join(", ", urlsToStart));
+                    Trace.LogInformation("Config changed. Starting the following endpoints: '{endpoints}'", string.Join("', '", urlsToStart));
 
                     foreach (var listenOption in endpointsToStart)
                     {

src/Servers/Kestrel/Core/src/KestrelServerOptions.cs

@@ -202,20 +202,43 @@ private void EnsureDefaultCert()
         /// <summary>
         /// Creates a configuration loader for setting up Kestrel.
         /// </summary>
+        /// <returns>A <see cref="KestrelConfigurationLoader"/> for configuring endpoints.</returns>
         public KestrelConfigurationLoader Configure()
         {
-            var loader = new KestrelConfigurationLoader(this, new ConfigurationBuilder().Build());
+            var loader = new KestrelConfigurationLoader(this, new ConfigurationBuilder().Build(), reloadOnChange: false);
             ConfigurationLoader = loader;
             return loader;
         }
 
         /// <summary>
-        /// Creates a configuration loader for setting up Kestrel that takes an IConfiguration as input.
+        /// Creates a configuration loader for setting up Kestrel that takes an <see cref="IConfiguration"/> as input.
         /// This configuration must be scoped to the configuration section for Kestrel.
+        /// Kestrel will dynamically update endpoint bindings when configuration changes.
+        /// This will only reload endpoints defined in the "Endpoints" section of your <paramref name="config"/>. Endpoints defined in code will not be reloaded.
+        /// Call <see cref="Configure(IConfiguration, bool)"/> to disable dynamic endpoint binding updates.
         /// </summary>
+        /// <param name="config">The configuration section for Kestrel.</param>
+        /// <returns>A <see cref="KestrelConfigurationLoader"/> for further endpoint configuration.</returns>
         public KestrelConfigurationLoader Configure(IConfiguration config)
         {
-            var loader = new KestrelConfigurationLoader(this, config);
+            var loader = new KestrelConfigurationLoader(this, config, reloadOnChange: true);
+            ConfigurationLoader = loader;
+            return loader;
+        }
+
+        /// <summary>
+        /// Creates a configuration loader for setting up Kestrel that takes an <see cref="IConfiguration"/> as input.
+        /// This configuration must be scoped to the configuration section for Kestrel.
+        /// </summary>
+        /// <param name="config">The configuration section for Kestrel.</param>
+        /// <param name="reloadOnChange">
+        /// If <see langword="true" />, Kestrel will dynamically update endpoint bindings when configuration changes.
+        /// This will only reload endpoints defined in the "Endpoints" section of your <paramref name="config"/>. Endpoints defined in code will not be reloaded.
+        /// </param>
+        /// <returns>A <see cref="KestrelConfigurationLoader"/> for further endpoint configuration.</returns>
+        public KestrelConfigurationLoader Configure(IConfiguration config, bool reloadOnChange)
+        {
+            var loader = new KestrelConfigurationLoader(this, config, reloadOnChange);
             ConfigurationLoader = loader;
             return loader;
         }

src/Servers/Kestrel/samples/SampleApp/Startup.cs

@@ -134,8 +134,9 @@ public static Task Main(string[] args)
                         .LocalhostEndpoint(basePort + 7)
                         .Load();
 
+                    // reloadOnChange: true is the default
                     options
-                        .Configure(context.Configuration.GetSection("Kestrel"))
+                        .Configure(context.Configuration.GetSection("Kestrel"), reloadOnChange: true)
                         .Endpoint("NamedEndpoint", opt =>
                         {