Skip to content

Commit d1dbbae

Browse files
halter73halter73
committed
More review feedback.
1 parent de055d1 commit d1dbbae

File tree

6 files changed

+51
-47
lines changed

6 files changed

+51
-47
lines changed

‎src/Servers/Kestrel/Core/src/Internal/ConfigSectionClone.cs

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

4+
#nullable enable
5+
46
using System;
57
using System.Collections.Generic;
68
using System.Linq;
@@ -19,10 +21,10 @@ public ConfigSectionClone(IConfigurationSection configSection)
1921
Children = children.ToDictionary(child => child.Key, child => new ConfigSectionClone(child));
2022
}
2123

22-
public string Value { get; set; }
23-
public Dictionary<string, ConfigSectionClone> Children { get; set; }
24+
public string Value { get; }
25+
public Dictionary<string, ConfigSectionClone> Children { get; }
2426

25-
public override bool Equals(object obj)
27+
public override bool Equals(object? obj)
2628
{
2729
if (!(obj is ConfigSectionClone other))
2830
{

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

Lines changed: 2 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

4+
#nullable enable
5+
46
using System;
57
using System.Collections;
68
using System.Collections.Generic;
@@ -66,30 +68,6 @@ public void Clear()
6668
}
6769
}
6870

69-
public void InternalAdd(string item)
70-
{
71-
lock (_addresses)
72-
{
73-
_addresses.Add(item);
74-
}
75-
}
76-
77-
public bool InternalRemove(string item)
78-
{
79-
lock (_addresses)
80-
{
81-
return _addresses.Remove(item);
82-
}
83-
}
84-
85-
public void InternalClear()
86-
{
87-
lock (_addresses)
88-
{
89-
_addresses.Clear();
90-
}
91-
}
92-
9371
public bool Contains(string item)
9472
{
9573
lock (_addresses)

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

Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -24,21 +24,21 @@ public class KestrelConfigurationLoader
2424
{
2525
private bool _loaded = false;
2626

27-
internal KestrelConfigurationLoader(KestrelServerOptions options, IConfiguration configuration)
27+
internal KestrelConfigurationLoader(KestrelServerOptions options, IConfiguration configuration, bool reloadOnChange)
2828
{
2929
Options = options ?? throw new ArgumentNullException(nameof(options));
3030
Configuration = configuration ?? throw new ArgumentNullException(nameof(configuration));
31+
ReloadOnChange = reloadOnChange;
3132
}
3233

3334
public KestrelServerOptions Options { get; }
3435
public IConfiguration Configuration { get; internal set; }
3536

3637
/// <summary>
37-
/// Gets or sets a boolean indicating if endpoints should be reconfigured when the configuration section changes.
38-
/// Kestrel will bind and unbind to endpoints as necessary.
38+
/// If <see langword="true" />, Kestrel will dynamically update endpoint bindings when configuration changes.
39+
/// This will only reload endpoints defined in the "Endpoints" section of your Kestrel configuration. Endpoints defined in code will not be reloaded.
3940
/// </summary>
40-
/// <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>
41-
public bool ReloadOnChange { get; } = true;
41+
internal bool ReloadOnChange { get; }
4242

4343
private ConfigurationReader ConfigurationReader { get; set; }
4444

@@ -269,7 +269,7 @@ public void Load()
269269
else
270270
{
271271
// Ensure endpoint is reloaded if it used the default protocol and the protocol changed.
272-
// listenOptions.Progocols should already be set to this by ApplyEndpointDefaults.
272+
// listenOptions.Protocols should already be set to this by ApplyEndpointDefaults.
273273
endpoint.Protocols = ConfigurationReader.EndpointDefaults.Protocols;
274274
}
275275

@@ -345,16 +345,17 @@ private void LoadDefaultCert(ConfigurationReader configReader)
345345
else
346346
{
347347
var logger = Options.ApplicationServices.GetRequiredService<ILogger<KestrelServer>>();
348-
var certificate = FindDeveloperCertificateFile(configReader, logger);
348+
var (certificate, certificateConfig) = FindDeveloperCertificateFile(configReader, logger);
349349
if (certificate != null)
350350
{
351351
logger.LocatedDevelopmentCertificate(certificate);
352+
DefaultCertificateConfig = certificateConfig;
352353
Options.DefaultCertificate = certificate;
353354
}
354355
}
355356
}
356357

357-
private X509Certificate2 FindDeveloperCertificateFile(ConfigurationReader configReader, ILogger<KestrelServer> logger)
358+
private (X509Certificate2, CertificateConfig) FindDeveloperCertificateFile(ConfigurationReader configReader, ILogger<KestrelServer> logger)
358359
{
359360
string certificatePath = null;
360361
try
@@ -369,13 +370,10 @@ private X509Certificate2 FindDeveloperCertificateFile(ConfigurationReader config
369370

370371
if (IsDevelopmentCertificate(certificate))
371372
{
372-
DefaultCertificateConfig = certificateConfig;
373-
return certificate;
373+
return (certificate, certificateConfig);
374374
}
375-
376-
return null;
377375
}
378-
else if (!File.Exists(certificatePath))
376+
else if (!string.IsNullOrEmpty(certificatePath))
379377
{
380378
logger.FailedToLocateDevelopmentCertificateFile(certificatePath);
381379
}
@@ -385,7 +383,7 @@ private X509Certificate2 FindDeveloperCertificateFile(ConfigurationReader config
385383
logger.FailedToLoadDevelopmentCertificate(certificatePath);
386384
}
387385

388-
return null;
386+
return (null, null);
389387
}
390388

391389
private static bool IsDevelopmentCertificate(X509Certificate2 certificate)

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -294,10 +294,12 @@ private async Task RebindAsync()
294294
reloadToken = Options.ConfigurationLoader.Configuration.GetReloadToken();
295295
var (endpointsToStop, endpointsToStart) = Options.ConfigurationLoader.Reload();
296296

297+
Trace.LogDebug("Config reload token fired. Checking for changes...");
298+
297299
if (endpointsToStop.Count > 0)
298300
{
299301
var urlsToStop = endpointsToStop.Select(lo => lo.EndpointConfig.Url ?? "<unknown>");
300-
Trace.LogInformation("Config changed. Stopping the following endpoints: {endpoints}", string.Join(", ", urlsToStop));
302+
Trace.LogInformation("Config changed. Stopping the following endpoints: '{endpoints}'", string.Join("', '", urlsToStop));
301303

302304
// 5 is the default value for WebHost's "shutdownTimeoutSeconds", so use that.
303305
using var timeoutCts = new CancellationTokenSource(TimeSpan.FromSeconds(5));
@@ -318,7 +320,7 @@ private async Task RebindAsync()
318320
if (endpointsToStart.Count > 0)
319321
{
320322
var urlsToStart = endpointsToStart.Select(lo => lo.EndpointConfig.Url ?? "<unknown>");
321-
Trace.LogInformation("Config changed. Starting the following endpoints: {endpoints}", string.Join(", ", urlsToStart));
323+
Trace.LogInformation("Config changed. Starting the following endpoints: '{endpoints}'", string.Join("', '", urlsToStart));
322324

323325
foreach (var listenOption in endpointsToStart)
324326
{

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

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -202,20 +202,43 @@ private void EnsureDefaultCert()
202202
/// <summary>
203203
/// Creates a configuration loader for setting up Kestrel.
204204
/// </summary>
205+
/// <returns>A <see cref="KestrelConfigurationLoader"/> for configuring endpoints.</returns>
205206
public KestrelConfigurationLoader Configure()
206207
{
207-
var loader = new KestrelConfigurationLoader(this, new ConfigurationBuilder().Build());
208+
var loader = new KestrelConfigurationLoader(this, new ConfigurationBuilder().Build(), reloadOnChange: false);
208209
ConfigurationLoader = loader;
209210
return loader;
210211
}
211212

212213
/// <summary>
213-
/// Creates a configuration loader for setting up Kestrel that takes an IConfiguration as input.
214+
/// Creates a configuration loader for setting up Kestrel that takes an <see cref="IConfiguration"/> as input.
214215
/// This configuration must be scoped to the configuration section for Kestrel.
216+
/// Kestrel will dynamically update endpoint bindings when configuration changes.
217+
/// This will only reload endpoints defined in the "Endpoints" section of your <paramref name="config"/>. Endpoints defined in code will not be reloaded.
218+
/// Call <see cref="Configure(IConfiguration, bool)"/> to disable dynamic endpoint binding updates.
215219
/// </summary>
220+
/// <param name="config">The configuration section for Kestrel.</param>
221+
/// <returns>A <see cref="KestrelConfigurationLoader"/> for further endpoint configuration.</returns>
216222
public KestrelConfigurationLoader Configure(IConfiguration config)
217223
{
218-
var loader = new KestrelConfigurationLoader(this, config);
224+
var loader = new KestrelConfigurationLoader(this, config, reloadOnChange: true);
225+
ConfigurationLoader = loader;
226+
return loader;
227+
}
228+
229+
/// <summary>
230+
/// Creates a configuration loader for setting up Kestrel that takes an <see cref="IConfiguration"/> as input.
231+
/// This configuration must be scoped to the configuration section for Kestrel.
232+
/// </summary>
233+
/// <param name="config">The configuration section for Kestrel.</param>
234+
/// <param name="reloadOnChange">
235+
/// If <see langword="true" />, Kestrel will dynamically update endpoint bindings when configuration changes.
236+
/// This will only reload endpoints defined in the "Endpoints" section of your <paramref name="config"/>. Endpoints defined in code will not be reloaded.
237+
/// </param>
238+
/// <returns>A <see cref="KestrelConfigurationLoader"/> for further endpoint configuration.</returns>
239+
public KestrelConfigurationLoader Configure(IConfiguration config, bool reloadOnChange)
240+
{
241+
var loader = new KestrelConfigurationLoader(this, config, reloadOnChange);
219242
ConfigurationLoader = loader;
220243
return loader;
221244
}

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,8 +134,9 @@ public static Task Main(string[] args)
134134
.LocalhostEndpoint(basePort + 7)
135135
.Load();
136136

137+
// reloadOnChange: true is the default
137138
options
138-
.Configure(context.Configuration.GetSection("Kestrel"))
139+
.Configure(context.Configuration.GetSection("Kestrel"), reloadOnChange: true)
139140
.Endpoint("NamedEndpoint", opt =>
140141
{
141142

0 commit comments

Comments
 (0)