From 0f6d97d9da77f1a3d71d011cbcea3b44006c6752 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Mon, 5 Jun 2023 16:55:14 -0700 Subject: [PATCH 01/17] Watch file-based certificates for changes When `IConfiguration` specifies certificates by path and `reloadOnChange` is true, monitor those paths for changes and trigger reloads. The reload behavior is the same as if the path in the configuration file were changed, rather than the contents of the referenced file. - Does not apply to code-based configuration, which never triggers reloads - Does not trigger on deletion - that would just tear down the server - Does not look through symlinks to determine whether the target has changed - On by default (when `reloadOnChange` is true) but can be disabled with app context switch `Microsoft.AspNetCore.Server.Kestrel.DisableCertificateFileWatching` Fixes #32351 --- .../src/Internal/CertificatePathWatcher.cs | 317 ++++++++++ .../CertificatePathWatcherLoggerExtensions.cs | 63 ++ .../Core/src/Internal/ConfigurationReader.cs | 22 +- .../Core/src/Internal/KestrelServerImpl.cs | 4 +- .../Core/src/KestrelConfigurationLoader.cs | 100 ++- .../Kestrel/Core/src/KestrelServerOptions.cs | 10 +- .../Core/test/CertificatePathWatcherTests.cs | 569 ++++++++++++++++++ .../Kestrel/Core/test/KestrelServerTests.cs | 3 + ...spNetCore.Server.Kestrel.Core.Tests.csproj | 1 + .../HttpsConnectionMiddlewareTests.cs | 2 +- 10 files changed, 1077 insertions(+), 14 deletions(-) create mode 100644 src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcher.cs create mode 100644 src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcherLoggerExtensions.cs create mode 100644 src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs diff --git a/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcher.cs b/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcher.cs new file mode 100644 index 000000000000..717d80dd97f6 --- /dev/null +++ b/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcher.cs @@ -0,0 +1,317 @@ +// 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 Microsoft.Extensions.Configuration; +using Microsoft.Extensions.FileProviders; +using Microsoft.Extensions.FileProviders.Physical; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Primitives; + +namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal; + +internal sealed partial class CertificatePathWatcher : IDisposable +{ + private readonly string _contentRootDir; + private readonly ILogger _logger; + + private readonly object _metadataLock = new(); + + /// Acquire before accessing. + private readonly Dictionary _metadataForDirectory = new(); + /// Acquire before accessing. + private readonly Dictionary _metadataForFile = new(); + + private ConfigurationReloadToken _reloadToken = new(); + private bool _disposed; + + public CertificatePathWatcher(IHostEnvironment hostEnvironment, ILogger logger) + { + _contentRootDir = hostEnvironment.ContentRootPath; + _logger = logger; + } + + /// + /// Returns a token that will fire when any watched is changed on disk. + /// The affected will have + /// set to true. + /// + public IChangeToken GetChangeToken() + { + return _reloadToken; + } + + /// + /// Update the set of s being watched for file changes. + /// If a given appears in both lists, it is first removed and then added. + /// + /// + /// Does not look consider targets when watching files that are symlinks. + /// + public void UpdateWatches(List certificateConfigsToRemove, List certificateConfigsToAdd) + { + var addSet = new HashSet(certificateConfigsToAdd, ReferenceEqualityComparer.Instance); + var removeSet = new HashSet(certificateConfigsToRemove, ReferenceEqualityComparer.Instance); + + // Don't remove anything we're going to re-add anyway. + // Don't remove such items from addSet to guard against the (hypothetical) possibility + // that a caller might remove a config that isn't already present. + removeSet.ExceptWith(certificateConfigsToAdd); + + if (addSet.Count == 0 && removeSet.Count == 0) + { + return; + } + + lock (_metadataLock) + { + // Adds before removes to increase the chances of watcher reuse. + // Since removeSet doesn't contain any of these configs, this won't change the semantics. + foreach (var certificateConfig in addSet) + { + AddWatch(certificateConfig); + } + + foreach (var certificateConfig in removeSet) + { + RemoveWatch(certificateConfig); + } + } + } + + /// + /// Start watching a certificate's file path for changes. + /// must have set to true. + /// + /// + /// Internal for testing. + /// + internal void AddWatch(CertificateConfig certificateConfig) + { + Debug.Assert(certificateConfig.IsFileCert, "AddWatch called on non-file cert"); + + var path = Path.Combine(_contentRootDir, certificateConfig.Path!); + var dir = Path.GetDirectoryName(path)!; + + if (!_metadataForDirectory.TryGetValue(dir, out var dirMetadata)) + { + if (!Directory.Exists(dir)) + { + _logger.DirectoryDoesNotExist(dir, path); + return; + } + + // If we wanted to detected deletions of this whole directory (which we don't since we ignore deletions), + // we'd probably need to watch the whole directory hierarchy + + var fileProvider = new PhysicalFileProvider(dir, ExclusionFilters.None); + dirMetadata = new DirectoryWatchMetadata(fileProvider); + _metadataForDirectory.Add(dir, dirMetadata); + + _logger.CreatedDirectoryWatcher(dir); + } + + if (!_metadataForFile.TryGetValue(path, out var fileMetadata)) + { + // PhysicalFileProvider appears to be able to tolerate non-existent files, as long as the directory exists + + var disposable = ChangeToken.OnChange( + () => dirMetadata.FileProvider.Watch(Path.GetFileName(path)), + static tuple => tuple.Item1.OnChange(tuple.Item2), + ValueTuple.Create(this, path)); + + fileMetadata = new FileWatchMetadata(disposable); + _metadataForFile.Add(path, fileMetadata); + dirMetadata.FileWatchCount++; + + // We actually don't care if the file doesn't exist - we'll watch in case it is created + fileMetadata.LastModifiedTime = GetLastModifiedTimeOrMinimum(path); + + _logger.CreatedFileWatcher(path); + } + + if (!fileMetadata.Configs.Add(certificateConfig)) + { + _logger.ReusedObserver(path); + return; + } + + _logger.AddedObserver(path); + + _logger.ObserverCount(path, fileMetadata.Configs.Count); + _logger.FileCount(dir, dirMetadata.FileWatchCount); + } + + private DateTime GetLastModifiedTimeOrMinimum(string path) + { + try + { + var fileInfo = new FileInfo(path); + if (fileInfo.Exists) + { + return fileInfo.LastWriteTimeUtc; + } + } + catch (Exception e) + { + _logger.LastModifiedTimeError(path, e); + } + + return DateTime.MinValue; + } + + private void OnChange(string path) + { + // Block until any in-progress updates are complete + lock (_metadataLock) + { + if (!_metadataForFile.TryGetValue(path, out var fileMetadata)) + { + _logger.UntrackedFileEvent(path); + return; + } + + // We ignore file changes that don't advance the last modified time. + // For example, if we lose access to the network share the file is + // stored on, we don't notify our listeners because no one wants + // their endpoint/server to shutdown when that happens. + // We also anticipate that a cert file might be renamed to cert.bak + // before a new cert is introduced with the old name. + // This also helps us in scenarios where the underlying file system + // reports more than one change for a single logical operation. + var lastModifiedTime = GetLastModifiedTimeOrMinimum(path); + if (lastModifiedTime > fileMetadata.LastModifiedTime) + { + fileMetadata.LastModifiedTime = lastModifiedTime; + } + else + { + if (lastModifiedTime == DateTime.MinValue) + { + _logger.EventWithoutLastModifiedTime(path); + } + else if (lastModifiedTime == fileMetadata.LastModifiedTime) + { + _logger.RedundantEvent(path); + } + else + { + _logger.OutOfOrderEvent(path); + } + return; + } + + var configs = fileMetadata.Configs; + foreach (var config in configs) + { + config.FileHasChanged = true; + } + } + + // AddWatch and RemoveWatch don't affect the token, so this doesn't need to be under the semaphore. + // It does however need to be synchronized, since there could be multiple concurrent events. + var previousToken = Interlocked.Exchange(ref _reloadToken, new()); + previousToken.OnReload(); + } + + /// + /// Stop watching a certificate's file path for changes (previously started by . + /// must have set to true. + /// + /// + /// Internal for testing. + /// + internal void RemoveWatch(CertificateConfig certificateConfig) + { + Debug.Assert(certificateConfig.IsFileCert, "RemoveWatch called on non-file cert"); + + var path = Path.Combine(_contentRootDir, certificateConfig.Path!); + var dir = Path.GetDirectoryName(path)!; + + if (!_metadataForFile.TryGetValue(path, out var fileMetadata)) + { + _logger.UnknownFile(path); + return; + } + + var configs = fileMetadata.Configs; + + if (!configs.Remove(certificateConfig)) + { + _logger.UnknownObserver(path); + return; + } + + _logger.RemovedObserver(path); + + var dirMetadata = _metadataForDirectory[dir]; + + if (configs.Count == 0) + { + fileMetadata.Dispose(); + _metadataForFile.Remove(path); + dirMetadata.FileWatchCount--; + + _logger.RemovedFileWatcher(path); + + if (dirMetadata.FileWatchCount == 0) + { + dirMetadata.Dispose(); + _metadataForDirectory.Remove(dir); + + _logger.RemovedDirectoryWatcher(dir); + } + } + + _logger.ObserverCount(path, configs.Count); + _logger.FileCount(dir, dirMetadata.FileWatchCount); + } + + /// Test hook + internal int TestGetDirectoryWatchCount() => _metadataForDirectory.Count; + + /// Test hook + internal int TestGetFileWatchCount(string dir) => _metadataForDirectory.TryGetValue(dir, out var metadata) ? metadata.FileWatchCount : 0; + + /// Test hook + internal int TestGetObserverCount(string path) => _metadataForFile.TryGetValue(path, out var metadata) ? metadata.Configs.Count : 0; + + void IDisposable.Dispose() + { + if (_disposed) + { + return; + } + _disposed = true; + + foreach (var dirMetadata in _metadataForDirectory.Values) + { + dirMetadata.Dispose(); + } + + foreach (var fileMetadata in _metadataForFile.Values) + { + fileMetadata.Dispose(); + } + + GC.SuppressFinalize(this); + } + + private sealed class DirectoryWatchMetadata(PhysicalFileProvider fileProvider) : IDisposable + { + public readonly PhysicalFileProvider FileProvider = fileProvider; + public int FileWatchCount; + + public void Dispose() => FileProvider.Dispose(); + } + + private sealed class FileWatchMetadata(IDisposable disposable) : IDisposable + { + public readonly IDisposable Disposable = disposable; + public readonly HashSet Configs = new(ReferenceEqualityComparer.Instance); + public DateTime LastModifiedTime = DateTime.MinValue; + + public void Dispose() => Disposable.Dispose(); + } +} diff --git a/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcherLoggerExtensions.cs b/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcherLoggerExtensions.cs new file mode 100644 index 000000000000..6ab725afde77 --- /dev/null +++ b/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcherLoggerExtensions.cs @@ -0,0 +1,63 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Microsoft.Extensions.Logging; + +namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal; + +internal static partial class CertificatePathWatcherLoggerExtensions +{ + [LoggerMessage(0, LogLevel.Warning, @"Directory ""{dir}"" does not exist so changes to the certificate ""{path}"" will not be tracked", EventName = "DirectoryDoesNotExist")] + public static partial void DirectoryDoesNotExist(this ILogger logger, string dir, string path); + + [LoggerMessage(1, LogLevel.Warning, @"Attempted to remove watch from unwatched path ""{path}""", EventName = "UnknownFile")] + public static partial void UnknownFile(this ILogger logger, string path); + + [LoggerMessage(2, LogLevel.Warning, @"Attempted to remove unknown observer from path ""{path}""", EventName = "UnknownObserver")] + public static partial void UnknownObserver(this ILogger logger, string path); + + [LoggerMessage(3, LogLevel.Debug, @"Created directory watcher for ""{dir}""", EventName = "CreatedDirectoryWatcher")] + public static partial void CreatedDirectoryWatcher(this ILogger logger, string dir); + + [LoggerMessage(4, LogLevel.Debug, @"Created file watcher for ""{path}""", EventName = "CreatedFileWatcher")] + public static partial void CreatedFileWatcher(this ILogger logger, string path); + + [LoggerMessage(5, LogLevel.Debug, @"Removed directory watcher for ""{dir}""", EventName = "RemovedDirectoryWatcher")] + public static partial void RemovedDirectoryWatcher(this ILogger logger, string dir); + + [LoggerMessage(6, LogLevel.Debug, @"Removed file watcher for ""{path}""", EventName = "RemovedFileWatcher")] + public static partial void RemovedFileWatcher(this ILogger logger, string path); + + [LoggerMessage(7, LogLevel.Debug, @"Error retrieving last modified time for ""{path}""", EventName = "LastModifiedTimeError")] + public static partial void LastModifiedTimeError(this ILogger logger, string path, Exception e); + + [LoggerMessage(8, LogLevel.Debug, @"Ignored event for presently untracked file ""{path}""", EventName = "UntrackedFileEvent")] + public static partial void UntrackedFileEvent(this ILogger logger, string path); + + [LoggerMessage(9, LogLevel.Debug, @"Ignored out-of-order event for file ""{path}""", EventName = "OutOfOrderEvent")] + public static partial void OutOfOrderEvent(this ILogger logger, string path); + + [LoggerMessage(10, LogLevel.Trace, @"Reused existing observer on file watcher for ""{path}""", EventName = "ReusedObserver")] + public static partial void ReusedObserver(this ILogger logger, string path); + + [LoggerMessage(11, LogLevel.Trace, @"Added observer to file watcher for ""{path}""", EventName = "AddedObserver")] + public static partial void AddedObserver(this ILogger logger, string path); + + [LoggerMessage(12, LogLevel.Trace, @"Removed observer from file watcher for ""{path}""", EventName = "RemovedObserver")] + public static partial void RemovedObserver(this ILogger logger, string path); + + [LoggerMessage(13, LogLevel.Trace, @"File ""{path}"" now has {count} observers", EventName = "ObserverCount")] + public static partial void ObserverCount(this ILogger logger, string path, int count); + + [LoggerMessage(14, LogLevel.Trace, @"Directory ""{dir}"" now has watchers on {count} files", EventName = "FileCount")] + public static partial void FileCount(this ILogger logger, string dir, int count); + + [LoggerMessage(15, LogLevel.Trace, @"Ignored event since last modified time for ""{path}"" was unavailable", EventName = "EventWithoutLastModifiedTime")] + public static partial void EventWithoutLastModifiedTime(this ILogger logger, string path); + + [LoggerMessage(16, LogLevel.Trace, @"Ignored redundant event for ""{path}""", EventName = "RedundantEvent")] + public static partial void RedundantEvent(this ILogger logger, string path); + + [LoggerMessage(17, LogLevel.Trace, @"Flagged {count} observers of ""{path}"" as changed", EventName = "FlaggedObservers")] + public static partial void FlaggedObservers(this ILogger logger, string path, int count); +} diff --git a/src/Servers/Kestrel/Core/src/Internal/ConfigurationReader.cs b/src/Servers/Kestrel/Core/src/Internal/ConfigurationReader.cs index 152026d05edf..0d0583a2065d 100644 --- a/src/Servers/Kestrel/Core/src/Internal/ConfigurationReader.cs +++ b/src/Servers/Kestrel/Core/src/Internal/ConfigurationReader.cs @@ -390,35 +390,41 @@ internal CertificateConfig() // File public bool IsFileCert => !string.IsNullOrEmpty(Path); - public string? Path { get; set; } + public string? Path { get; init; } - public string? KeyPath { get; set; } + public string? KeyPath { get; init; } - public string? Password { get; set; } + public string? Password { get; init; } + + /// + /// Vacuously false if this isn't a file cert. + /// + public bool FileHasChanged { get; internal set; } // Cert store public bool IsStoreCert => !string.IsNullOrEmpty(Subject); - public string? Subject { get; set; } + public string? Subject { get; init; } - public string? Store { get; set; } + public string? Store { get; init; } - public string? Location { get; set; } + public string? Location { get; init; } - public bool? AllowInvalid { get; set; } + public bool? AllowInvalid { get; init; } public override bool Equals(object? obj) => obj is CertificateConfig other && Path == other.Path && KeyPath == other.KeyPath && Password == other.Password && + FileHasChanged == other.FileHasChanged && Subject == other.Subject && Store == other.Store && Location == other.Location && (AllowInvalid ?? false) == (other.AllowInvalid ?? false); - public override int GetHashCode() => HashCode.Combine(Path, KeyPath, Password, Subject, Store, Location, AllowInvalid ?? false); + public override int GetHashCode() => HashCode.Combine(Path, KeyPath, Password, FileHasChanged, Subject, Store, Location, AllowInvalid ?? false); public static bool operator ==(CertificateConfig? lhs, CertificateConfig? rhs) => lhs is null ? rhs is null : lhs.Equals(rhs); public static bool operator !=(CertificateConfig? lhs, CertificateConfig? rhs) => !(lhs == rhs); diff --git a/src/Servers/Kestrel/Core/src/Internal/KestrelServerImpl.cs b/src/Servers/Kestrel/Core/src/Internal/KestrelServerImpl.cs index e749d52f2016..b8af73a906dd 100644 --- a/src/Servers/Kestrel/Core/src/Internal/KestrelServerImpl.cs +++ b/src/Servers/Kestrel/Core/src/Internal/KestrelServerImpl.cs @@ -300,7 +300,7 @@ private async Task BindAsync(CancellationToken cancellationToken) if (Options.ConfigurationLoader?.ReloadOnChange == true && (!_serverAddresses.PreferHostingUrls || _serverAddresses.InternalCollection.Count == 0)) { - reloadToken = Options.ConfigurationLoader.Configuration.GetReloadToken(); + reloadToken = Options.ConfigurationLoader.GetReloadToken(); } Options.ConfigurationLoader?.LoadInternal(); @@ -340,7 +340,7 @@ private async Task RebindAsync() Debug.Assert(Options.ConfigurationLoader != null, "Rebind can only happen when there is a ConfigurationLoader."); - reloadToken = Options.ConfigurationLoader.Configuration.GetReloadToken(); + reloadToken = Options.ConfigurationLoader.GetReloadToken(); var (endpointsToStop, endpointsToStart) = Options.ConfigurationLoader.Reload(); Trace.LogDebug("Config reload token fired. Checking for changes..."); diff --git a/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs b/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs index 8104a2e00d4a..99982a3e98e1 100644 --- a/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs +++ b/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs @@ -20,15 +20,22 @@ public class KestrelConfigurationLoader { private readonly IHttpsConfigurationService _httpsConfigurationService; + /// + /// Non-null only makes sense if is true. + /// + private readonly CertificatePathWatcher? _certificatePathWatcher; + private bool _loaded; private bool _endpointsToAddProcessed; + // This is not used to trigger reloads but to suppress redundant reloads triggered in other ways private IChangeToken? _reloadToken; internal KestrelConfigurationLoader( KestrelServerOptions options, IConfiguration configuration, IHttpsConfigurationService httpsConfigurationService, + CertificatePathWatcher? certificatePathWatcher, bool reloadOnChange) { Options = options; @@ -39,6 +46,8 @@ internal KestrelConfigurationLoader( ConfigurationReader = new ConfigurationReader(configuration); _httpsConfigurationService = httpsConfigurationService; + _certificatePathWatcher = certificatePathWatcher; + Debug.Assert(!!reloadOnChange || (certificatePathWatcher is null), "If reloadOnChange is false, then certificatePathWatcher should be null"); } /// @@ -49,7 +58,7 @@ internal KestrelConfigurationLoader( /// /// Gets the application . /// - public IConfiguration Configuration { get; internal set; } + public IConfiguration Configuration { get; internal set; } // Setter internal for testing /// /// If , Kestrel will dynamically update endpoint bindings when configuration changes. @@ -283,19 +292,36 @@ internal void ProcessEndpointsToAdd() } } + internal IChangeToken? GetReloadToken() + { + Debug.Assert(ReloadOnChange); + + var configToken = Configuration.GetReloadToken(); + + if (_certificatePathWatcher is null) + { + return configToken; + } + + var watcherToken = _certificatePathWatcher.GetChangeToken(); + return new CompositeChangeToken(new[] { configToken, watcherToken }); + } + // 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() { if (ReloadOnChange) { - _reloadToken = Configuration.GetReloadToken(); + _reloadToken = GetReloadToken(); } var endpointsToStop = Options.ConfigurationBackedListenOptions.ToList(); var endpointsToStart = new List(); var endpointsToReuse = new List(); + var oldDefaultCertificateConfig = DefaultCertificateConfig; + DefaultCertificateConfig = null; DefaultCertificate = null; @@ -345,6 +371,7 @@ internal void ProcessEndpointsToAdd() { if (o.EndpointConfig == endpoint) { + Debug.Assert(o.EndpointConfig?.Certificate?.FileHasChanged != true, "Preserving an endpoint with file changes"); matchingBoundEndpoints.Add(o); } } @@ -384,6 +411,75 @@ internal void ProcessEndpointsToAdd() Options.ConfigurationBackedListenOptions.AddRange(endpointsToReuse); Options.ConfigurationBackedListenOptions.AddRange(endpointsToStart); + if (ReloadOnChange && _certificatePathWatcher is not null) + { + var certificateConfigsToRemove = new List(); + var certificateConfigsToAdd = new List(); + + if (DefaultCertificateConfig != oldDefaultCertificateConfig) + { + if (DefaultCertificateConfig?.IsFileCert == true) + { + certificateConfigsToAdd.Add(DefaultCertificateConfig); + } + + if (oldDefaultCertificateConfig is not null) + { + certificateConfigsToRemove.Add(oldDefaultCertificateConfig); + } + } + + foreach (var endpointToStart in endpointsToStart) + { + var endpointConfig = endpointToStart.EndpointConfig; + if (endpointConfig is null) + { + continue; + } + + var certConfig = endpointConfig.Certificate; + if (certConfig?.IsFileCert == true) + { + certificateConfigsToAdd.Add(certConfig); + } + + foreach (var sniConfig in endpointConfig.Sni.Values) + { + var sniCertConfig = sniConfig.Certificate; + if (sniCertConfig?.IsFileCert == true) + { + certificateConfigsToAdd.Add(sniCertConfig); + } + } + } + + foreach (var endpointToStop in endpointsToStop) + { + var endpointConfig = endpointToStop.EndpointConfig; + if (endpointConfig is null) + { + continue; + } + + var certConfig = endpointConfig.Certificate; + if (certConfig?.IsFileCert == true) + { + certificateConfigsToRemove.Add(certConfig); + } + + foreach (var sniConfig in endpointConfig.Sni.Values) + { + var sniCertConfig = sniConfig.Certificate; + if (sniCertConfig?.IsFileCert == true) + { + certificateConfigsToRemove.Add(sniCertConfig); + } + } + } + + _certificatePathWatcher.UpdateWatches(certificateConfigsToRemove, certificateConfigsToAdd); + } + return (endpointsToStop, endpointsToStart); } } diff --git a/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs b/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs index 175c208efc9e..0820b36b02f6 100644 --- a/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs +++ b/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs @@ -28,11 +28,14 @@ public class KestrelServerOptions { internal const string DisableHttp1LineFeedTerminatorsSwitchKey = "Microsoft.AspNetCore.Server.Kestrel.DisableHttp1LineFeedTerminators"; private const string FinOnErrorSwitch = "Microsoft.AspNetCore.Server.Kestrel.FinOnError"; + private const string CertificateFileWatchingSwitch = "Microsoft.AspNetCore.Server.Kestrel.DisableCertificateFileWatching"; private static readonly bool _finOnError; + private static readonly bool _disableCertificateFileWatching; static KestrelServerOptions() { AppContext.TryGetSwitch(FinOnErrorSwitch, out _finOnError); + AppContext.TryGetSwitch(CertificateFileWatchingSwitch, out _disableCertificateFileWatching); } // internal to fast-path header decoding when RequestHeaderEncodingSelector is unchanged. @@ -443,7 +446,12 @@ public KestrelConfigurationLoader Configure(IConfiguration config, bool reloadOn } var httpsConfigurationService = ApplicationServices.GetRequiredService(); - var loader = new KestrelConfigurationLoader(this, config, httpsConfigurationService, reloadOnChange); + var certificatePathWatcher = reloadOnChange && !_disableCertificateFileWatching + ? new CertificatePathWatcher( + ApplicationServices.GetRequiredService(), + ApplicationServices.GetRequiredService>()) + : null; + var loader = new KestrelConfigurationLoader(this, config, httpsConfigurationService, certificatePathWatcher, reloadOnChange); ConfigurationLoader = loader; return loader; } diff --git a/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs b/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs new file mode 100644 index 000000000000..fa6cf27dfd06 --- /dev/null +++ b/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs @@ -0,0 +1,569 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Microsoft.AspNetCore.Server.Kestrel.Core.Internal; +using Microsoft.AspNetCore.Testing; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Testing; +using Moq; + +namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests; + +public class CertificatePathWatcherTests : LoggedTest +{ + [Theory] + [InlineData(true, true)] + [InlineData(true, false)] + [InlineData(false, true)] + [InlineData(false, false)] + public void AddAndRemoveWatch(bool fileExists, bool absoluteFilePath) + { + var dirInfo = Directory.CreateTempSubdirectory(); + try + { + var dir = dirInfo.FullName; + var fileName = Path.GetRandomFileName(); + var filePath = Path.Combine(dir, fileName); + + var hostEnvironment = new Mock(); + hostEnvironment.SetupGet(h => h.ContentRootPath).Returns(dir); + + var logger = LoggerFactory.CreateLogger(); + + using var watcher = new CertificatePathWatcher(hostEnvironment.Object, logger); + + var changeToken = watcher.GetChangeToken(); + + var certificateConfig = new CertificateConfig + { + Path = absoluteFilePath ? filePath : fileName, + }; + + if (fileExists) + { + Touch(filePath); + } + + Assert.Equal(fileExists, File.Exists(filePath)); + + IDictionary messageProps; + + watcher.AddWatch(certificateConfig); + + messageProps = GetLogMessageProperties(TestSink, "CreatedDirectoryWatcher"); + Assert.Equal(dir, messageProps["dir"]); + + messageProps = GetLogMessageProperties(TestSink, "CreatedFileWatcher"); + Assert.Equal(filePath, messageProps["path"]); + + Assert.Equal(1, watcher.TestGetDirectoryWatchCount()); + Assert.Equal(1, watcher.TestGetFileWatchCount(dir)); + Assert.Equal(1, watcher.TestGetObserverCount(filePath)); + + watcher.RemoveWatch(certificateConfig); + + messageProps = GetLogMessageProperties(TestSink, "RemovedFileWatcher"); + Assert.Equal(filePath, messageProps["path"]); + + messageProps = GetLogMessageProperties(TestSink, "RemovedDirectoryWatcher"); + Assert.Equal(dir, messageProps["dir"]); + + Assert.Equal(0, watcher.TestGetDirectoryWatchCount()); + Assert.Equal(0, watcher.TestGetFileWatchCount(dir)); + Assert.Equal(0, watcher.TestGetObserverCount(filePath)); + + Assert.Same(changeToken, watcher.GetChangeToken()); + Assert.False(changeToken.HasChanged); + } + finally + { + dirInfo.Delete(recursive: true); + } + } + + [Theory] + [InlineData(2, 4)] + [InlineData(5, 3)] + [InlineData(5, 13)] + public void WatchMultipleDirectories(int dirCount, int fileCount) + { + var rootDirInfo = Directory.CreateTempSubdirectory(); + try + { + var rootDir = rootDirInfo.FullName; + var dirs = new string[dirCount]; + + var hostEnvironment = new Mock(); + hostEnvironment.SetupGet(h => h.ContentRootPath).Returns(rootDir); + + var logger = LoggerFactory.CreateLogger(); + + for (int i = 0; i < dirCount; i++) + { + dirs[i] = Path.Combine(rootDir, $"dir{i}"); + Directory.CreateDirectory(dirs[i]); + } + + using var watcher = new CertificatePathWatcher(hostEnvironment.Object, logger); + + var certificateConfigs = new CertificateConfig[fileCount]; + var filesInDir = new int[dirCount]; + for (int i = 0; i < fileCount; i++) + { + certificateConfigs[i] = new CertificateConfig + { + Path = $"dir{i % dirCount}/file{i % fileCount}", + }; + filesInDir[i % dirCount]++; + } + + foreach (var certificateConfig in certificateConfigs) + { + watcher.AddWatch(certificateConfig); + } + + Assert.Equal(Math.Min(dirCount, fileCount), watcher.TestGetDirectoryWatchCount()); + + for (int i = 0; i < dirCount; i++) + { + Assert.Equal(filesInDir[i], watcher.TestGetFileWatchCount(dirs[i])); + } + + foreach (var certificateConfig in certificateConfigs) + { + watcher.RemoveWatch(certificateConfig); + } + + Assert.Equal(0, watcher.TestGetDirectoryWatchCount()); + } + finally + { + rootDirInfo.Delete(recursive: true); + } + } + + [Theory] + [InlineData(1)] + [InlineData(4)] + public async Task FileChanged(int observerCount) + { + var dirInfo = Directory.CreateTempSubdirectory(); + try + { + var dir = dirInfo.FullName; + var fileName = Path.GetRandomFileName(); + var filePath = Path.Combine(dir, fileName); + + Touch(filePath); + + var hostEnvironment = new Mock(); + hostEnvironment.SetupGet(h => h.ContentRootPath).Returns(dir); + + var logger = LoggerFactory.CreateLogger(); + + using var watcher = new CertificatePathWatcher(hostEnvironment.Object, logger); + + var signalTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + var oldChangeToken = watcher.GetChangeToken(); + oldChangeToken.RegisterChangeCallback(_ => signalTcs.SetResult(), state: null); + + var certificateConfigs = new CertificateConfig[observerCount]; + for (int i = 0; i < observerCount; i++) + { + certificateConfigs[i] = new CertificateConfig + { + Path = filePath, + }; + + watcher.AddWatch(certificateConfigs[i]); + } + + Assert.Equal(1, watcher.TestGetDirectoryWatchCount()); + Assert.Equal(1, watcher.TestGetFileWatchCount(dir)); + Assert.Equal(observerCount, watcher.TestGetObserverCount(filePath)); + + Touch(filePath); + + await signalTcs.Task.DefaultTimeout(); + + var newChangeToken = watcher.GetChangeToken(); + + Assert.NotSame(oldChangeToken, newChangeToken); + Assert.True(oldChangeToken.HasChanged); + Assert.False(newChangeToken.HasChanged); + + Assert.All(certificateConfigs, cc => Assert.True(cc.FileHasChanged)); + } + finally + { + dirInfo.Delete(recursive: true); + } + } + + [Fact] + public void DirectoryDoesNotExist() + { + var dir = Path.Combine(Directory.GetCurrentDirectory(), Path.GetRandomFileName()); + + Assert.False(Directory.Exists(dir)); + + var hostEnvironment = new Mock(); + hostEnvironment.SetupGet(h => h.ContentRootPath).Returns(dir); + + var logger = LoggerFactory.CreateLogger(); + + using var watcher = new CertificatePathWatcher(hostEnvironment.Object, logger); + + var certificateConfig = new CertificateConfig + { + Path = Path.Combine(dir, "test.pfx"), + }; + + watcher.AddWatch(certificateConfig); + + var messageProps = GetLogMessageProperties(TestSink, "DirectoryDoesNotExist"); + Assert.Equal(dir, messageProps["dir"]); + + Assert.Equal(0, watcher.TestGetDirectoryWatchCount()); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public void RemoveUnknownFileWatch(bool previouslyAdded) + { + var dirInfo = Directory.CreateTempSubdirectory(); + try + { + var dir = dirInfo.FullName; + var fileName = Path.GetRandomFileName(); + var filePath = Path.Combine(dir, fileName); + + Touch(filePath); + + var hostEnvironment = new Mock(); + hostEnvironment.SetupGet(h => h.ContentRootPath).Returns(dir); + + var logger = LoggerFactory.CreateLogger(); + + using var watcher = new CertificatePathWatcher(hostEnvironment.Object, logger); + + var certificateConfig = new CertificateConfig + { + Path = filePath, + }; + + if (previouslyAdded) + { + watcher.AddWatch(certificateConfig); + watcher.RemoveWatch(certificateConfig); + } + + Assert.Equal(0, watcher.TestGetObserverCount(filePath)); + + watcher.RemoveWatch(certificateConfig); + + var messageProps = GetLogMessageProperties(TestSink, "UnknownFile"); + Assert.Equal(filePath, messageProps["path"]); + + Assert.Equal(0, watcher.TestGetObserverCount(filePath)); + } + finally + { + dirInfo.Delete(recursive: true); + } + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public void RemoveUnknownFileObserver(bool previouslyAdded) + { + var dirInfo = Directory.CreateTempSubdirectory(); + try + { + var dir = dirInfo.FullName; + var fileName = Path.GetRandomFileName(); + var filePath = Path.Combine(dir, fileName); + + Touch(filePath); + + var hostEnvironment = new Mock(); + hostEnvironment.SetupGet(h => h.ContentRootPath).Returns(dir); + + var logger = LoggerFactory.CreateLogger(); + + using var watcher = new CertificatePathWatcher(hostEnvironment.Object, logger); + + var certificateConfig1 = new CertificateConfig + { + Path = filePath, + }; + + var certificateConfig2 = new CertificateConfig + { + Path = filePath, + }; + + watcher.AddWatch(certificateConfig1); + + if (previouslyAdded) + { + watcher.AddWatch(certificateConfig2); + watcher.RemoveWatch(certificateConfig2); + } + + Assert.Equal(1, watcher.TestGetObserverCount(filePath)); + + watcher.RemoveWatch(certificateConfig2); + + var messageProps = GetLogMessageProperties(TestSink, "UnknownObserver"); + Assert.Equal(filePath, messageProps["path"]); + + Assert.Equal(1, watcher.TestGetObserverCount(filePath)); + } + finally + { + dirInfo.Delete(recursive: true); + } + } + + [Fact] + [LogLevel(LogLevel.Trace)] + public void ReuseFileObserver() + { + var dirInfo = Directory.CreateTempSubdirectory(); + try + { + var dir = dirInfo.FullName; + var fileName = Path.GetRandomFileName(); + var filePath = Path.Combine(dir, fileName); + + Touch(filePath); + + var hostEnvironment = new Mock(); + hostEnvironment.SetupGet(h => h.ContentRootPath).Returns(dir); + + var logger = LoggerFactory.CreateLogger(); + + using var watcher = new CertificatePathWatcher(hostEnvironment.Object, logger); + + var certificateConfig = new CertificateConfig + { + Path = filePath, + }; + + watcher.AddWatch(certificateConfig); + + Assert.Equal(1, watcher.TestGetObserverCount(filePath)); + + watcher.AddWatch(certificateConfig); + + var messageProps = GetLogMessageProperties(TestSink, "ReusedObserver"); + Assert.Equal(filePath, messageProps["path"]); + + Assert.Equal(1, watcher.TestGetObserverCount(filePath)); + } + finally + { + dirInfo.Delete(recursive: true); + } + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + [LogLevel(LogLevel.Trace)] + public async Task IgnoreDeletion(bool deleteDirectory) + { + var dirInfo = Directory.CreateTempSubdirectory(); + try + { + var dir = dirInfo.FullName; + var fileName = Path.GetRandomFileName(); + var filePath = Path.Combine(dir, fileName); + + Touch(filePath); + + var hostEnvironment = new Mock(); + hostEnvironment.SetupGet(h => h.ContentRootPath).Returns(dir); + + var logger = LoggerFactory.CreateLogger(); + + using var watcher = new CertificatePathWatcher(hostEnvironment.Object, logger); + + var certificateConfig = new CertificateConfig + { + Path = filePath, + }; + + watcher.AddWatch(certificateConfig); + + Assert.Equal(1, watcher.TestGetDirectoryWatchCount()); + Assert.Equal(1, watcher.TestGetFileWatchCount(dir)); + Assert.Equal(1, watcher.TestGetObserverCount(filePath)); + + var changeTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + watcher.GetChangeToken().RegisterChangeCallback(_ => changeTcs.SetResult(), state: null); + + var logTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + TestSink.MessageLogged += writeContext => + { + if (writeContext.EventId.Name == "EventWithoutLastModifiedTime") + { + logTcs.SetResult(); + } + }; + + // "Delete" the file or directory + if (deleteDirectory) + { + dirInfo.MoveTo(dir + ".bak"); + } + else + { + File.Move(filePath, filePath + ".bak"); + } + + Assert.False(File.Exists(filePath)); + + try + { + await logTcs.Task.DefaultTimeout(); + } + catch (TimeoutException) + { + // In some scenarios, the directory deletion won't trigger an event. + // For example, a `FileSystemWatcher` on Windows won't fire one, + // whereas the polling watcher will. + Assert.True(deleteDirectory); + } + + Assert.Equal(1, watcher.TestGetDirectoryWatchCount()); + Assert.Equal(1, watcher.TestGetFileWatchCount(dir)); + Assert.Equal(1, watcher.TestGetObserverCount(filePath)); + + Assert.False(changeTcs.Task.IsCompleted); + + // Restore the file or directory + if (deleteDirectory) + { + dirInfo.MoveTo(dir); + } + else + { + File.Move(filePath + ".bak", filePath); + } + + Assert.True(File.Exists(filePath)); + + Touch(filePath); // In some scenarios, renaming the file back doesn't change the last modified time + + await changeTcs.Task.DefaultTimeout(); + } + finally + { + dirInfo.Delete(recursive: true); + } + } + + [Fact] + public void UpdateWatches() + { + var dirInfo = Directory.CreateTempSubdirectory(); + try + { + var dir = dirInfo.FullName; + var fileName = Path.GetRandomFileName(); + var filePath = Path.Combine(dir, fileName); + + var hostEnvironment = new Mock(); + hostEnvironment.SetupGet(h => h.ContentRootPath).Returns(dir); + + var logger = LoggerFactory.CreateLogger(); + + using var watcher = new CertificatePathWatcher(hostEnvironment.Object, logger); + + var changeToken = watcher.GetChangeToken(); + + var certificateConfig1 = new CertificateConfig + { + Path = filePath, + }; + + var certificateConfig2 = new CertificateConfig + { + Path = filePath, + }; + + var certificateConfig3 = new CertificateConfig + { + Path = filePath, + }; + + // Add certificateConfig1 + watcher.UpdateWatches(new List { }, new List { certificateConfig1 }); + + Assert.Equal(1, watcher.TestGetDirectoryWatchCount()); + Assert.Equal(1, watcher.TestGetFileWatchCount(dir)); + Assert.Equal(1, watcher.TestGetObserverCount(filePath)); + + // Remove certificateConfig1 + watcher.UpdateWatches(new List { certificateConfig1 }, new List { }); + + Assert.Equal(0, watcher.TestGetDirectoryWatchCount()); + Assert.Equal(0, watcher.TestGetFileWatchCount(dir)); + Assert.Equal(0, watcher.TestGetObserverCount(filePath)); + + // Re-add certificateConfig1 + watcher.UpdateWatches(new List { }, new List { certificateConfig1 }); + + Assert.Equal(1, watcher.TestGetDirectoryWatchCount()); + Assert.Equal(1, watcher.TestGetFileWatchCount(dir)); + Assert.Equal(1, watcher.TestGetObserverCount(filePath)); + + watcher.UpdateWatches( + new List + { + certificateConfig1, // Delete something present + certificateConfig1, // Delete it again + certificateConfig2, // Delete something never added + certificateConfig2, // Delete it again + }, + new List + { + certificateConfig1, // Re-add something removed above + certificateConfig1, // Re-add it again + certificateConfig2, // Add something vacuously removed above + certificateConfig2, // Add it again + certificateConfig3, // Add something new + certificateConfig3, // Add it again + }); + + Assert.Equal(1, watcher.TestGetDirectoryWatchCount()); + Assert.Equal(1, watcher.TestGetFileWatchCount(dir)); + Assert.Equal(3, watcher.TestGetObserverCount(filePath)); + } + finally + { + dirInfo.Delete(recursive: true); + } + } + + private static void Touch(string filePath) + { + File.Create(filePath).Dispose(); + } + + private static IDictionary GetLogMessageProperties(ITestSink testSink, string eventName) + { + var writeContext = Assert.Single(testSink.Writes.Where(wc => wc.EventId.Name == eventName)); + var pairs = (IReadOnlyList>)writeContext.State; + var dict = new Dictionary(pairs); + return dict; + } +} diff --git a/src/Servers/Kestrel/Core/test/KestrelServerTests.cs b/src/Servers/Kestrel/Core/test/KestrelServerTests.cs index e13e3717688e..f3f4e3a60afd 100644 --- a/src/Servers/Kestrel/Core/test/KestrelServerTests.cs +++ b/src/Servers/Kestrel/Core/test/KestrelServerTests.cs @@ -8,6 +8,7 @@ using Microsoft.AspNetCore.Hosting.Server; using Microsoft.AspNetCore.Hosting.Server.Features; using Microsoft.AspNetCore.Http.Features; +using Microsoft.AspNetCore.Server.Kestrel.Core.Internal; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure; using Microsoft.AspNetCore.Server.Kestrel.Https.Internal; @@ -760,6 +761,7 @@ public async Task ReloadsOnConfigurationChangeWhenOptedIn() TaskCompletionSource changeCallbackRegisteredTcs = null; var mockChangeToken = new Mock(); + mockChangeToken.Setup(t => t.ActiveChangeCallbacks).Returns(true); mockChangeToken.Setup(t => t.RegisterChangeCallback(It.IsAny>(), It.IsAny())).Returns, object>((callback, state) => { changeCallbackRegisteredTcs?.SetResult(); @@ -786,6 +788,7 @@ public async Task ReloadsOnConfigurationChangeWhenOptedIn() serviceCollection.AddSingleton(Mock.Of()); serviceCollection.AddSingleton(Mock.Of>()); serviceCollection.AddSingleton(Mock.Of>()); + serviceCollection.AddSingleton(Mock.Of>()); serviceCollection.AddSingleton(Mock.Of()); var options = new KestrelServerOptions diff --git a/src/Servers/Kestrel/Core/test/Microsoft.AspNetCore.Server.Kestrel.Core.Tests.csproj b/src/Servers/Kestrel/Core/test/Microsoft.AspNetCore.Server.Kestrel.Core.Tests.csproj index 34d9d34335dc..638330a77a1d 100644 --- a/src/Servers/Kestrel/Core/test/Microsoft.AspNetCore.Server.Kestrel.Core.Tests.csproj +++ b/src/Servers/Kestrel/Core/test/Microsoft.AspNetCore.Server.Kestrel.Core.Tests.csproj @@ -28,6 +28,7 @@ + diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsConnectionMiddlewareTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsConnectionMiddlewareTests.cs index 7d7c61e5a477..12ec2aadd9d2 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsConnectionMiddlewareTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsConnectionMiddlewareTests.cs @@ -79,7 +79,7 @@ public async Task CanReadAndWriteWithHttpsConnectionMiddlewareWithPemCertificate var options = CreateServerOptions(); - var loader = new KestrelConfigurationLoader(options, configuration, options.ApplicationServices.GetRequiredService(), reloadOnChange: false); + var loader = new KestrelConfigurationLoader(options, configuration, options.ApplicationServices.GetRequiredService(), certificatePathWatcher: null, reloadOnChange: false); options.ConfigurationLoader = loader; // Since we're constructing it explicitly, we have to hook it up explicitly loader.Load(); From 428df83e8b4b6ba93266958f1c4f3ed1b2c77dcc Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 10 Aug 2023 12:01:54 -0700 Subject: [PATCH 02/17] Annotate CertificateConfig with MemberNotNullWhenAttribute --- .../Kestrel/Core/src/Internal/CertificatePathWatcher.cs | 4 ++-- .../Kestrel/Core/src/Internal/ConfigurationReader.cs | 4 ++++ .../Kestrel/Core/test/CertificatePathWatcherTests.cs | 8 +++++--- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcher.cs b/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcher.cs index 717d80dd97f6..e2f7d2848522 100644 --- a/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcher.cs +++ b/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcher.cs @@ -91,7 +91,7 @@ internal void AddWatch(CertificateConfig certificateConfig) { Debug.Assert(certificateConfig.IsFileCert, "AddWatch called on non-file cert"); - var path = Path.Combine(_contentRootDir, certificateConfig.Path!); + var path = Path.Combine(_contentRootDir, certificateConfig.Path); var dir = Path.GetDirectoryName(path)!; if (!_metadataForDirectory.TryGetValue(dir, out var dirMetadata)) @@ -226,7 +226,7 @@ internal void RemoveWatch(CertificateConfig certificateConfig) { Debug.Assert(certificateConfig.IsFileCert, "RemoveWatch called on non-file cert"); - var path = Path.Combine(_contentRootDir, certificateConfig.Path!); + var path = Path.Combine(_contentRootDir, certificateConfig.Path); var dir = Path.GetDirectoryName(path)!; if (!_metadataForFile.TryGetValue(path, out var fileMetadata)) diff --git a/src/Servers/Kestrel/Core/src/Internal/ConfigurationReader.cs b/src/Servers/Kestrel/Core/src/Internal/ConfigurationReader.cs index 0d0583a2065d..97c8ed55c595 100644 --- a/src/Servers/Kestrel/Core/src/Internal/ConfigurationReader.cs +++ b/src/Servers/Kestrel/Core/src/Internal/ConfigurationReader.cs @@ -1,6 +1,7 @@ // 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.CodeAnalysis; using System.Linq; using System.Security.Authentication; using Microsoft.AspNetCore.Server.Kestrel.Https; @@ -388,6 +389,8 @@ internal CertificateConfig() public IConfigurationSection? ConfigSection { get; } // File + + [MemberNotNullWhen(true, nameof(Path))] public bool IsFileCert => !string.IsNullOrEmpty(Path); public string? Path { get; init; } @@ -403,6 +406,7 @@ internal CertificateConfig() // Cert store + [MemberNotNullWhen(true, nameof(Subject))] public bool IsStoreCert => !string.IsNullOrEmpty(Subject); public string? Subject { get; init; } diff --git a/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs b/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs index fa6cf27dfd06..2e79f855713e 100644 --- a/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs +++ b/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs @@ -12,6 +12,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests; public class CertificatePathWatcherTests : LoggedTest { + private static readonly TimeSpan FileChangeTimeout = TimeSpan.FromSeconds(10); + [Theory] [InlineData(true, true)] [InlineData(true, false)] @@ -186,7 +188,7 @@ public async Task FileChanged(int observerCount) Touch(filePath); - await signalTcs.Task.DefaultTimeout(); + await signalTcs.Task.TimeoutAfter(FileChangeTimeout); var newChangeToken = watcher.GetChangeToken(); @@ -433,7 +435,7 @@ public async Task IgnoreDeletion(bool deleteDirectory) try { - await logTcs.Task.DefaultTimeout(); + await logTcs.Task.TimeoutAfter(FileChangeTimeout); } catch (TimeoutException) { @@ -463,7 +465,7 @@ public async Task IgnoreDeletion(bool deleteDirectory) Touch(filePath); // In some scenarios, renaming the file back doesn't change the last modified time - await changeTcs.Task.DefaultTimeout(); + await changeTcs.Task.TimeoutAfter(FileChangeTimeout); } finally { From eada2be5cde526dd56b175f99208c2d3feb6eaf9 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 10 Aug 2023 12:02:53 -0700 Subject: [PATCH 03/17] Index log messages from 1 --- .../CertificatePathWatcherLoggerExtensions.cs | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcherLoggerExtensions.cs b/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcherLoggerExtensions.cs index 6ab725afde77..19ceae34f970 100644 --- a/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcherLoggerExtensions.cs +++ b/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcherLoggerExtensions.cs @@ -7,57 +7,57 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal; internal static partial class CertificatePathWatcherLoggerExtensions { - [LoggerMessage(0, LogLevel.Warning, @"Directory ""{dir}"" does not exist so changes to the certificate ""{path}"" will not be tracked", EventName = "DirectoryDoesNotExist")] + [LoggerMessage(1, LogLevel.Warning, @"Directory ""{dir}"" does not exist so changes to the certificate ""{path}"" will not be tracked", EventName = "DirectoryDoesNotExist")] public static partial void DirectoryDoesNotExist(this ILogger logger, string dir, string path); - [LoggerMessage(1, LogLevel.Warning, @"Attempted to remove watch from unwatched path ""{path}""", EventName = "UnknownFile")] + [LoggerMessage(2, LogLevel.Warning, @"Attempted to remove watch from unwatched path ""{path}""", EventName = "UnknownFile")] public static partial void UnknownFile(this ILogger logger, string path); - [LoggerMessage(2, LogLevel.Warning, @"Attempted to remove unknown observer from path ""{path}""", EventName = "UnknownObserver")] + [LoggerMessage(3, LogLevel.Warning, @"Attempted to remove unknown observer from path ""{path}""", EventName = "UnknownObserver")] public static partial void UnknownObserver(this ILogger logger, string path); - [LoggerMessage(3, LogLevel.Debug, @"Created directory watcher for ""{dir}""", EventName = "CreatedDirectoryWatcher")] + [LoggerMessage(4, LogLevel.Debug, @"Created directory watcher for ""{dir}""", EventName = "CreatedDirectoryWatcher")] public static partial void CreatedDirectoryWatcher(this ILogger logger, string dir); - [LoggerMessage(4, LogLevel.Debug, @"Created file watcher for ""{path}""", EventName = "CreatedFileWatcher")] + [LoggerMessage(5, LogLevel.Debug, @"Created file watcher for ""{path}""", EventName = "CreatedFileWatcher")] public static partial void CreatedFileWatcher(this ILogger logger, string path); - [LoggerMessage(5, LogLevel.Debug, @"Removed directory watcher for ""{dir}""", EventName = "RemovedDirectoryWatcher")] + [LoggerMessage(6, LogLevel.Debug, @"Removed directory watcher for ""{dir}""", EventName = "RemovedDirectoryWatcher")] public static partial void RemovedDirectoryWatcher(this ILogger logger, string dir); - [LoggerMessage(6, LogLevel.Debug, @"Removed file watcher for ""{path}""", EventName = "RemovedFileWatcher")] + [LoggerMessage(7, LogLevel.Debug, @"Removed file watcher for ""{path}""", EventName = "RemovedFileWatcher")] public static partial void RemovedFileWatcher(this ILogger logger, string path); - [LoggerMessage(7, LogLevel.Debug, @"Error retrieving last modified time for ""{path}""", EventName = "LastModifiedTimeError")] + [LoggerMessage(8, LogLevel.Debug, @"Error retrieving last modified time for ""{path}""", EventName = "LastModifiedTimeError")] public static partial void LastModifiedTimeError(this ILogger logger, string path, Exception e); - [LoggerMessage(8, LogLevel.Debug, @"Ignored event for presently untracked file ""{path}""", EventName = "UntrackedFileEvent")] + [LoggerMessage(9, LogLevel.Debug, @"Ignored event for presently untracked file ""{path}""", EventName = "UntrackedFileEvent")] public static partial void UntrackedFileEvent(this ILogger logger, string path); - [LoggerMessage(9, LogLevel.Debug, @"Ignored out-of-order event for file ""{path}""", EventName = "OutOfOrderEvent")] + [LoggerMessage(10, LogLevel.Debug, @"Ignored out-of-order event for file ""{path}""", EventName = "OutOfOrderEvent")] public static partial void OutOfOrderEvent(this ILogger logger, string path); - [LoggerMessage(10, LogLevel.Trace, @"Reused existing observer on file watcher for ""{path}""", EventName = "ReusedObserver")] + [LoggerMessage(11, LogLevel.Trace, @"Reused existing observer on file watcher for ""{path}""", EventName = "ReusedObserver")] public static partial void ReusedObserver(this ILogger logger, string path); - [LoggerMessage(11, LogLevel.Trace, @"Added observer to file watcher for ""{path}""", EventName = "AddedObserver")] + [LoggerMessage(12, LogLevel.Trace, @"Added observer to file watcher for ""{path}""", EventName = "AddedObserver")] public static partial void AddedObserver(this ILogger logger, string path); - [LoggerMessage(12, LogLevel.Trace, @"Removed observer from file watcher for ""{path}""", EventName = "RemovedObserver")] + [LoggerMessage(13, LogLevel.Trace, @"Removed observer from file watcher for ""{path}""", EventName = "RemovedObserver")] public static partial void RemovedObserver(this ILogger logger, string path); - [LoggerMessage(13, LogLevel.Trace, @"File ""{path}"" now has {count} observers", EventName = "ObserverCount")] + [LoggerMessage(14, LogLevel.Trace, @"File ""{path}"" now has {count} observers", EventName = "ObserverCount")] public static partial void ObserverCount(this ILogger logger, string path, int count); - [LoggerMessage(14, LogLevel.Trace, @"Directory ""{dir}"" now has watchers on {count} files", EventName = "FileCount")] + [LoggerMessage(15, LogLevel.Trace, @"Directory ""{dir}"" now has watchers on {count} files", EventName = "FileCount")] public static partial void FileCount(this ILogger logger, string dir, int count); - [LoggerMessage(15, LogLevel.Trace, @"Ignored event since last modified time for ""{path}"" was unavailable", EventName = "EventWithoutLastModifiedTime")] + [LoggerMessage(16, LogLevel.Trace, @"Ignored event since last modified time for ""{path}"" was unavailable", EventName = "EventWithoutLastModifiedTime")] public static partial void EventWithoutLastModifiedTime(this ILogger logger, string path); - [LoggerMessage(16, LogLevel.Trace, @"Ignored redundant event for ""{path}""", EventName = "RedundantEvent")] + [LoggerMessage(17, LogLevel.Trace, @"Ignored redundant event for ""{path}""", EventName = "RedundantEvent")] public static partial void RedundantEvent(this ILogger logger, string path); - [LoggerMessage(17, LogLevel.Trace, @"Flagged {count} observers of ""{path}"" as changed", EventName = "FlaggedObservers")] + [LoggerMessage(18, LogLevel.Trace, @"Flagged {count} observers of ""{path}"" as changed", EventName = "FlaggedObservers")] public static partial void FlaggedObservers(this ILogger logger, string path, int count); } From 02f3e0977ef8f4437bccceb2160fe037388ff264 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 10 Aug 2023 12:04:51 -0700 Subject: [PATCH 04/17] Make log message placeholders PascalCase --- .../CertificatePathWatcherLoggerExtensions.cs | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcherLoggerExtensions.cs b/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcherLoggerExtensions.cs index 19ceae34f970..97e169e2b555 100644 --- a/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcherLoggerExtensions.cs +++ b/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcherLoggerExtensions.cs @@ -7,57 +7,57 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal; internal static partial class CertificatePathWatcherLoggerExtensions { - [LoggerMessage(1, LogLevel.Warning, @"Directory ""{dir}"" does not exist so changes to the certificate ""{path}"" will not be tracked", EventName = "DirectoryDoesNotExist")] + [LoggerMessage(1, LogLevel.Warning, @"Directory ""{Dir}"" does not exist so changes to the certificate ""{Path}"" will not be tracked", EventName = "DirectoryDoesNotExist")] public static partial void DirectoryDoesNotExist(this ILogger logger, string dir, string path); - [LoggerMessage(2, LogLevel.Warning, @"Attempted to remove watch from unwatched path ""{path}""", EventName = "UnknownFile")] + [LoggerMessage(2, LogLevel.Warning, @"Attempted to remove watch from unwatched path ""{Path}""", EventName = "UnknownFile")] public static partial void UnknownFile(this ILogger logger, string path); - [LoggerMessage(3, LogLevel.Warning, @"Attempted to remove unknown observer from path ""{path}""", EventName = "UnknownObserver")] + [LoggerMessage(3, LogLevel.Warning, @"Attempted to remove unknown observer from path ""{Path}""", EventName = "UnknownObserver")] public static partial void UnknownObserver(this ILogger logger, string path); - [LoggerMessage(4, LogLevel.Debug, @"Created directory watcher for ""{dir}""", EventName = "CreatedDirectoryWatcher")] + [LoggerMessage(4, LogLevel.Debug, @"Created directory watcher for ""{Dir}""", EventName = "CreatedDirectoryWatcher")] public static partial void CreatedDirectoryWatcher(this ILogger logger, string dir); - [LoggerMessage(5, LogLevel.Debug, @"Created file watcher for ""{path}""", EventName = "CreatedFileWatcher")] + [LoggerMessage(5, LogLevel.Debug, @"Created file watcher for ""{Path}""", EventName = "CreatedFileWatcher")] public static partial void CreatedFileWatcher(this ILogger logger, string path); - [LoggerMessage(6, LogLevel.Debug, @"Removed directory watcher for ""{dir}""", EventName = "RemovedDirectoryWatcher")] + [LoggerMessage(6, LogLevel.Debug, @"Removed directory watcher for ""{Dir}""", EventName = "RemovedDirectoryWatcher")] public static partial void RemovedDirectoryWatcher(this ILogger logger, string dir); - [LoggerMessage(7, LogLevel.Debug, @"Removed file watcher for ""{path}""", EventName = "RemovedFileWatcher")] + [LoggerMessage(7, LogLevel.Debug, @"Removed file watcher for ""{Path}""", EventName = "RemovedFileWatcher")] public static partial void RemovedFileWatcher(this ILogger logger, string path); - [LoggerMessage(8, LogLevel.Debug, @"Error retrieving last modified time for ""{path}""", EventName = "LastModifiedTimeError")] + [LoggerMessage(8, LogLevel.Debug, @"Error retrieving last modified time for ""{Path}""", EventName = "LastModifiedTimeError")] public static partial void LastModifiedTimeError(this ILogger logger, string path, Exception e); - [LoggerMessage(9, LogLevel.Debug, @"Ignored event for presently untracked file ""{path}""", EventName = "UntrackedFileEvent")] + [LoggerMessage(9, LogLevel.Debug, @"Ignored event for presently untracked file ""{Path}""", EventName = "UntrackedFileEvent")] public static partial void UntrackedFileEvent(this ILogger logger, string path); - [LoggerMessage(10, LogLevel.Debug, @"Ignored out-of-order event for file ""{path}""", EventName = "OutOfOrderEvent")] + [LoggerMessage(10, LogLevel.Debug, @"Ignored out-of-order event for file ""{Path}""", EventName = "OutOfOrderEvent")] public static partial void OutOfOrderEvent(this ILogger logger, string path); - [LoggerMessage(11, LogLevel.Trace, @"Reused existing observer on file watcher for ""{path}""", EventName = "ReusedObserver")] + [LoggerMessage(11, LogLevel.Trace, @"Reused existing observer on file watcher for ""{Path}""", EventName = "ReusedObserver")] public static partial void ReusedObserver(this ILogger logger, string path); - [LoggerMessage(12, LogLevel.Trace, @"Added observer to file watcher for ""{path}""", EventName = "AddedObserver")] + [LoggerMessage(12, LogLevel.Trace, @"Added observer to file watcher for ""{Path}""", EventName = "AddedObserver")] public static partial void AddedObserver(this ILogger logger, string path); - [LoggerMessage(13, LogLevel.Trace, @"Removed observer from file watcher for ""{path}""", EventName = "RemovedObserver")] + [LoggerMessage(13, LogLevel.Trace, @"Removed observer from file watcher for ""{Path}""", EventName = "RemovedObserver")] public static partial void RemovedObserver(this ILogger logger, string path); - [LoggerMessage(14, LogLevel.Trace, @"File ""{path}"" now has {count} observers", EventName = "ObserverCount")] + [LoggerMessage(14, LogLevel.Trace, @"File ""{Path}"" now has {Count} observers", EventName = "ObserverCount")] public static partial void ObserverCount(this ILogger logger, string path, int count); - [LoggerMessage(15, LogLevel.Trace, @"Directory ""{dir}"" now has watchers on {count} files", EventName = "FileCount")] + [LoggerMessage(15, LogLevel.Trace, @"Directory ""{Dir}"" now has watchers on {Count} files", EventName = "FileCount")] public static partial void FileCount(this ILogger logger, string dir, int count); - [LoggerMessage(16, LogLevel.Trace, @"Ignored event since last modified time for ""{path}"" was unavailable", EventName = "EventWithoutLastModifiedTime")] + [LoggerMessage(16, LogLevel.Trace, @"Ignored event since last modified time for ""{Path}"" was unavailable", EventName = "EventWithoutLastModifiedTime")] public static partial void EventWithoutLastModifiedTime(this ILogger logger, string path); - [LoggerMessage(17, LogLevel.Trace, @"Ignored redundant event for ""{path}""", EventName = "RedundantEvent")] + [LoggerMessage(17, LogLevel.Trace, @"Ignored redundant event for ""{Path}""", EventName = "RedundantEvent")] public static partial void RedundantEvent(this ILogger logger, string path); - [LoggerMessage(18, LogLevel.Trace, @"Flagged {count} observers of ""{path}"" as changed", EventName = "FlaggedObservers")] + [LoggerMessage(18, LogLevel.Trace, @"Flagged {Count} observers of ""{Path}"" as changed", EventName = "FlaggedObservers")] public static partial void FlaggedObservers(this ILogger logger, string path, int count); } From 5cf296d240f576ea0a5706c7a274d483257418e8 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 10 Aug 2023 12:07:51 -0700 Subject: [PATCH 05/17] Append 'Unsynchronized' to test hook names --- .../src/Internal/CertificatePathWatcher.cs | 18 +-- .../Core/test/CertificatePathWatcherTests.cs | 106 +++++++++--------- 2 files changed, 62 insertions(+), 62 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcher.cs b/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcher.cs index e2f7d2848522..7004c5643ffd 100644 --- a/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcher.cs +++ b/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcher.cs @@ -47,7 +47,7 @@ public IChangeToken GetChangeToken() /// If a given appears in both lists, it is first removed and then added. /// /// - /// Does not look consider targets when watching files that are symlinks. + /// Does not consider targets when watching files that are symlinks. /// public void UpdateWatches(List certificateConfigsToRemove, List certificateConfigsToAdd) { @@ -70,12 +70,12 @@ public void UpdateWatches(List certificateConfigsToRemove, Li // Since removeSet doesn't contain any of these configs, this won't change the semantics. foreach (var certificateConfig in addSet) { - AddWatch(certificateConfig); + AddWatchUnsynchronized(certificateConfig); } foreach (var certificateConfig in removeSet) { - RemoveWatch(certificateConfig); + RemoveWatchUnsynchronized(certificateConfig); } } } @@ -87,7 +87,7 @@ public void UpdateWatches(List certificateConfigsToRemove, Li /// /// Internal for testing. /// - internal void AddWatch(CertificateConfig certificateConfig) + internal void AddWatchUnsynchronized(CertificateConfig certificateConfig) { Debug.Assert(certificateConfig.IsFileCert, "AddWatch called on non-file cert"); @@ -216,13 +216,13 @@ private void OnChange(string path) } /// - /// Stop watching a certificate's file path for changes (previously started by . + /// Stop watching a certificate's file path for changes (previously started by . /// must have set to true. /// /// /// Internal for testing. /// - internal void RemoveWatch(CertificateConfig certificateConfig) + internal void RemoveWatchUnsynchronized(CertificateConfig certificateConfig) { Debug.Assert(certificateConfig.IsFileCert, "RemoveWatch called on non-file cert"); @@ -269,13 +269,13 @@ internal void RemoveWatch(CertificateConfig certificateConfig) } /// Test hook - internal int TestGetDirectoryWatchCount() => _metadataForDirectory.Count; + internal int TestGetDirectoryWatchCountUnsynchronized() => _metadataForDirectory.Count; /// Test hook - internal int TestGetFileWatchCount(string dir) => _metadataForDirectory.TryGetValue(dir, out var metadata) ? metadata.FileWatchCount : 0; + internal int TestGetFileWatchCountUnsynchronized(string dir) => _metadataForDirectory.TryGetValue(dir, out var metadata) ? metadata.FileWatchCount : 0; /// Test hook - internal int TestGetObserverCount(string path) => _metadataForFile.TryGetValue(path, out var metadata) ? metadata.Configs.Count : 0; + internal int TestGetObserverCountUnsynchronized(string path) => _metadataForFile.TryGetValue(path, out var metadata) ? metadata.Configs.Count : 0; void IDisposable.Dispose() { diff --git a/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs b/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs index 2e79f855713e..53aa64875f92 100644 --- a/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs +++ b/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs @@ -51,7 +51,7 @@ public void AddAndRemoveWatch(bool fileExists, bool absoluteFilePath) IDictionary messageProps; - watcher.AddWatch(certificateConfig); + watcher.AddWatchUnsynchronized(certificateConfig); messageProps = GetLogMessageProperties(TestSink, "CreatedDirectoryWatcher"); Assert.Equal(dir, messageProps["dir"]); @@ -59,11 +59,11 @@ public void AddAndRemoveWatch(bool fileExists, bool absoluteFilePath) messageProps = GetLogMessageProperties(TestSink, "CreatedFileWatcher"); Assert.Equal(filePath, messageProps["path"]); - Assert.Equal(1, watcher.TestGetDirectoryWatchCount()); - Assert.Equal(1, watcher.TestGetFileWatchCount(dir)); - Assert.Equal(1, watcher.TestGetObserverCount(filePath)); + Assert.Equal(1, watcher.TestGetDirectoryWatchCountUnsynchronized()); + Assert.Equal(1, watcher.TestGetFileWatchCountUnsynchronized(dir)); + Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); - watcher.RemoveWatch(certificateConfig); + watcher.RemoveWatchUnsynchronized(certificateConfig); messageProps = GetLogMessageProperties(TestSink, "RemovedFileWatcher"); Assert.Equal(filePath, messageProps["path"]); @@ -71,9 +71,9 @@ public void AddAndRemoveWatch(bool fileExists, bool absoluteFilePath) messageProps = GetLogMessageProperties(TestSink, "RemovedDirectoryWatcher"); Assert.Equal(dir, messageProps["dir"]); - Assert.Equal(0, watcher.TestGetDirectoryWatchCount()); - Assert.Equal(0, watcher.TestGetFileWatchCount(dir)); - Assert.Equal(0, watcher.TestGetObserverCount(filePath)); + Assert.Equal(0, watcher.TestGetDirectoryWatchCountUnsynchronized()); + Assert.Equal(0, watcher.TestGetFileWatchCountUnsynchronized(dir)); + Assert.Equal(0, watcher.TestGetObserverCountUnsynchronized(filePath)); Assert.Same(changeToken, watcher.GetChangeToken()); Assert.False(changeToken.HasChanged); @@ -122,22 +122,22 @@ public void WatchMultipleDirectories(int dirCount, int fileCount) foreach (var certificateConfig in certificateConfigs) { - watcher.AddWatch(certificateConfig); + watcher.AddWatchUnsynchronized(certificateConfig); } - Assert.Equal(Math.Min(dirCount, fileCount), watcher.TestGetDirectoryWatchCount()); + Assert.Equal(Math.Min(dirCount, fileCount), watcher.TestGetDirectoryWatchCountUnsynchronized()); for (int i = 0; i < dirCount; i++) { - Assert.Equal(filesInDir[i], watcher.TestGetFileWatchCount(dirs[i])); + Assert.Equal(filesInDir[i], watcher.TestGetFileWatchCountUnsynchronized(dirs[i])); } foreach (var certificateConfig in certificateConfigs) { - watcher.RemoveWatch(certificateConfig); + watcher.RemoveWatchUnsynchronized(certificateConfig); } - Assert.Equal(0, watcher.TestGetDirectoryWatchCount()); + Assert.Equal(0, watcher.TestGetDirectoryWatchCountUnsynchronized()); } finally { @@ -179,12 +179,12 @@ public async Task FileChanged(int observerCount) Path = filePath, }; - watcher.AddWatch(certificateConfigs[i]); + watcher.AddWatchUnsynchronized(certificateConfigs[i]); } - Assert.Equal(1, watcher.TestGetDirectoryWatchCount()); - Assert.Equal(1, watcher.TestGetFileWatchCount(dir)); - Assert.Equal(observerCount, watcher.TestGetObserverCount(filePath)); + Assert.Equal(1, watcher.TestGetDirectoryWatchCountUnsynchronized()); + Assert.Equal(1, watcher.TestGetFileWatchCountUnsynchronized(dir)); + Assert.Equal(observerCount, watcher.TestGetObserverCountUnsynchronized(filePath)); Touch(filePath); @@ -223,12 +223,12 @@ public void DirectoryDoesNotExist() Path = Path.Combine(dir, "test.pfx"), }; - watcher.AddWatch(certificateConfig); + watcher.AddWatchUnsynchronized(certificateConfig); var messageProps = GetLogMessageProperties(TestSink, "DirectoryDoesNotExist"); Assert.Equal(dir, messageProps["dir"]); - Assert.Equal(0, watcher.TestGetDirectoryWatchCount()); + Assert.Equal(0, watcher.TestGetDirectoryWatchCountUnsynchronized()); } [Theory] @@ -259,18 +259,18 @@ public void RemoveUnknownFileWatch(bool previouslyAdded) if (previouslyAdded) { - watcher.AddWatch(certificateConfig); - watcher.RemoveWatch(certificateConfig); + watcher.AddWatchUnsynchronized(certificateConfig); + watcher.RemoveWatchUnsynchronized(certificateConfig); } - Assert.Equal(0, watcher.TestGetObserverCount(filePath)); + Assert.Equal(0, watcher.TestGetObserverCountUnsynchronized(filePath)); - watcher.RemoveWatch(certificateConfig); + watcher.RemoveWatchUnsynchronized(certificateConfig); var messageProps = GetLogMessageProperties(TestSink, "UnknownFile"); Assert.Equal(filePath, messageProps["path"]); - Assert.Equal(0, watcher.TestGetObserverCount(filePath)); + Assert.Equal(0, watcher.TestGetObserverCountUnsynchronized(filePath)); } finally { @@ -309,22 +309,22 @@ public void RemoveUnknownFileObserver(bool previouslyAdded) Path = filePath, }; - watcher.AddWatch(certificateConfig1); + watcher.AddWatchUnsynchronized(certificateConfig1); if (previouslyAdded) { - watcher.AddWatch(certificateConfig2); - watcher.RemoveWatch(certificateConfig2); + watcher.AddWatchUnsynchronized(certificateConfig2); + watcher.RemoveWatchUnsynchronized(certificateConfig2); } - Assert.Equal(1, watcher.TestGetObserverCount(filePath)); + Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); - watcher.RemoveWatch(certificateConfig2); + watcher.RemoveWatchUnsynchronized(certificateConfig2); var messageProps = GetLogMessageProperties(TestSink, "UnknownObserver"); Assert.Equal(filePath, messageProps["path"]); - Assert.Equal(1, watcher.TestGetObserverCount(filePath)); + Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); } finally { @@ -357,16 +357,16 @@ public void ReuseFileObserver() Path = filePath, }; - watcher.AddWatch(certificateConfig); + watcher.AddWatchUnsynchronized(certificateConfig); - Assert.Equal(1, watcher.TestGetObserverCount(filePath)); + Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); - watcher.AddWatch(certificateConfig); + watcher.AddWatchUnsynchronized(certificateConfig); var messageProps = GetLogMessageProperties(TestSink, "ReusedObserver"); Assert.Equal(filePath, messageProps["path"]); - Assert.Equal(1, watcher.TestGetObserverCount(filePath)); + Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); } finally { @@ -401,11 +401,11 @@ public async Task IgnoreDeletion(bool deleteDirectory) Path = filePath, }; - watcher.AddWatch(certificateConfig); + watcher.AddWatchUnsynchronized(certificateConfig); - Assert.Equal(1, watcher.TestGetDirectoryWatchCount()); - Assert.Equal(1, watcher.TestGetFileWatchCount(dir)); - Assert.Equal(1, watcher.TestGetObserverCount(filePath)); + Assert.Equal(1, watcher.TestGetDirectoryWatchCountUnsynchronized()); + Assert.Equal(1, watcher.TestGetFileWatchCountUnsynchronized(dir)); + Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); var changeTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); @@ -445,9 +445,9 @@ public async Task IgnoreDeletion(bool deleteDirectory) Assert.True(deleteDirectory); } - Assert.Equal(1, watcher.TestGetDirectoryWatchCount()); - Assert.Equal(1, watcher.TestGetFileWatchCount(dir)); - Assert.Equal(1, watcher.TestGetObserverCount(filePath)); + Assert.Equal(1, watcher.TestGetDirectoryWatchCountUnsynchronized()); + Assert.Equal(1, watcher.TestGetFileWatchCountUnsynchronized(dir)); + Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); Assert.False(changeTcs.Task.IsCompleted); @@ -510,23 +510,23 @@ public void UpdateWatches() // Add certificateConfig1 watcher.UpdateWatches(new List { }, new List { certificateConfig1 }); - Assert.Equal(1, watcher.TestGetDirectoryWatchCount()); - Assert.Equal(1, watcher.TestGetFileWatchCount(dir)); - Assert.Equal(1, watcher.TestGetObserverCount(filePath)); + Assert.Equal(1, watcher.TestGetDirectoryWatchCountUnsynchronized()); + Assert.Equal(1, watcher.TestGetFileWatchCountUnsynchronized(dir)); + Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); // Remove certificateConfig1 watcher.UpdateWatches(new List { certificateConfig1 }, new List { }); - Assert.Equal(0, watcher.TestGetDirectoryWatchCount()); - Assert.Equal(0, watcher.TestGetFileWatchCount(dir)); - Assert.Equal(0, watcher.TestGetObserverCount(filePath)); + Assert.Equal(0, watcher.TestGetDirectoryWatchCountUnsynchronized()); + Assert.Equal(0, watcher.TestGetFileWatchCountUnsynchronized(dir)); + Assert.Equal(0, watcher.TestGetObserverCountUnsynchronized(filePath)); // Re-add certificateConfig1 watcher.UpdateWatches(new List { }, new List { certificateConfig1 }); - Assert.Equal(1, watcher.TestGetDirectoryWatchCount()); - Assert.Equal(1, watcher.TestGetFileWatchCount(dir)); - Assert.Equal(1, watcher.TestGetObserverCount(filePath)); + Assert.Equal(1, watcher.TestGetDirectoryWatchCountUnsynchronized()); + Assert.Equal(1, watcher.TestGetFileWatchCountUnsynchronized(dir)); + Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); watcher.UpdateWatches( new List @@ -546,9 +546,9 @@ public void UpdateWatches() certificateConfig3, // Add it again }); - Assert.Equal(1, watcher.TestGetDirectoryWatchCount()); - Assert.Equal(1, watcher.TestGetFileWatchCount(dir)); - Assert.Equal(3, watcher.TestGetObserverCount(filePath)); + Assert.Equal(1, watcher.TestGetDirectoryWatchCountUnsynchronized()); + Assert.Equal(1, watcher.TestGetFileWatchCountUnsynchronized(dir)); + Assert.Equal(3, watcher.TestGetObserverCountUnsynchronized(filePath)); } finally { From 7bc7dc2386c26acda401cd1402e8f4cb57d0e2d2 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 10 Aug 2023 12:09:40 -0700 Subject: [PATCH 06/17] Add explanatory comment --- src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcher.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcher.cs b/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcher.cs index 7004c5643ffd..bea01af643ec 100644 --- a/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcher.cs +++ b/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcher.cs @@ -245,6 +245,7 @@ internal void RemoveWatchUnsynchronized(CertificateConfig certificateConfig) _logger.RemovedObserver(path); + // If we found fileMetadata, there must be a containing/corresponding dirMetadata var dirMetadata = _metadataForDirectory[dir]; if (configs.Count == 0) From 8350cb312de1f9ae3e5d4bd47fd3fea6dacdeb43 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 10 Aug 2023 12:13:06 -0700 Subject: [PATCH 07/17] Fix tests now that structured logging property names are PascalCase --- .../Core/test/CertificatePathWatcherTests.cs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs b/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs index 53aa64875f92..51ac376b9977 100644 --- a/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs +++ b/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs @@ -54,10 +54,10 @@ public void AddAndRemoveWatch(bool fileExists, bool absoluteFilePath) watcher.AddWatchUnsynchronized(certificateConfig); messageProps = GetLogMessageProperties(TestSink, "CreatedDirectoryWatcher"); - Assert.Equal(dir, messageProps["dir"]); + Assert.Equal(dir, messageProps["Dir"]); messageProps = GetLogMessageProperties(TestSink, "CreatedFileWatcher"); - Assert.Equal(filePath, messageProps["path"]); + Assert.Equal(filePath, messageProps["Path"]); Assert.Equal(1, watcher.TestGetDirectoryWatchCountUnsynchronized()); Assert.Equal(1, watcher.TestGetFileWatchCountUnsynchronized(dir)); @@ -66,10 +66,10 @@ public void AddAndRemoveWatch(bool fileExists, bool absoluteFilePath) watcher.RemoveWatchUnsynchronized(certificateConfig); messageProps = GetLogMessageProperties(TestSink, "RemovedFileWatcher"); - Assert.Equal(filePath, messageProps["path"]); + Assert.Equal(filePath, messageProps["Path"]); messageProps = GetLogMessageProperties(TestSink, "RemovedDirectoryWatcher"); - Assert.Equal(dir, messageProps["dir"]); + Assert.Equal(dir, messageProps["Dir"]); Assert.Equal(0, watcher.TestGetDirectoryWatchCountUnsynchronized()); Assert.Equal(0, watcher.TestGetFileWatchCountUnsynchronized(dir)); @@ -226,7 +226,7 @@ public void DirectoryDoesNotExist() watcher.AddWatchUnsynchronized(certificateConfig); var messageProps = GetLogMessageProperties(TestSink, "DirectoryDoesNotExist"); - Assert.Equal(dir, messageProps["dir"]); + Assert.Equal(dir, messageProps["Dir"]); Assert.Equal(0, watcher.TestGetDirectoryWatchCountUnsynchronized()); } @@ -268,7 +268,7 @@ public void RemoveUnknownFileWatch(bool previouslyAdded) watcher.RemoveWatchUnsynchronized(certificateConfig); var messageProps = GetLogMessageProperties(TestSink, "UnknownFile"); - Assert.Equal(filePath, messageProps["path"]); + Assert.Equal(filePath, messageProps["Path"]); Assert.Equal(0, watcher.TestGetObserverCountUnsynchronized(filePath)); } @@ -322,7 +322,7 @@ public void RemoveUnknownFileObserver(bool previouslyAdded) watcher.RemoveWatchUnsynchronized(certificateConfig2); var messageProps = GetLogMessageProperties(TestSink, "UnknownObserver"); - Assert.Equal(filePath, messageProps["path"]); + Assert.Equal(filePath, messageProps["Path"]); Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); } @@ -364,7 +364,7 @@ public void ReuseFileObserver() watcher.AddWatchUnsynchronized(certificateConfig); var messageProps = GetLogMessageProperties(TestSink, "ReusedObserver"); - Assert.Equal(filePath, messageProps["path"]); + Assert.Equal(filePath, messageProps["Path"]); Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); } From a50c2a7166d54e5b3a6e080168a5074d0977b565 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 10 Aug 2023 14:52:21 -0700 Subject: [PATCH 08/17] Add a test at the KestrelConfigurationLoader level --- .../Kestrel/Core/src/KestrelServerOptions.cs | 2 +- .../test/KestrelConfigurationLoaderTests.cs | 81 +++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) diff --git a/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs b/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs index 0820b36b02f6..c2670be70067 100644 --- a/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs +++ b/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs @@ -28,7 +28,7 @@ public class KestrelServerOptions { internal const string DisableHttp1LineFeedTerminatorsSwitchKey = "Microsoft.AspNetCore.Server.Kestrel.DisableHttp1LineFeedTerminators"; private const string FinOnErrorSwitch = "Microsoft.AspNetCore.Server.Kestrel.FinOnError"; - private const string CertificateFileWatchingSwitch = "Microsoft.AspNetCore.Server.Kestrel.DisableCertificateFileWatching"; + internal const string CertificateFileWatchingSwitch = "Microsoft.AspNetCore.Server.Kestrel.DisableCertificateFileWatching"; private static readonly bool _finOnError; private static readonly bool _disableCertificateFileWatching; diff --git a/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs b/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs index 83777bc0e2aa..d05144c3ea48 100644 --- a/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs +++ b/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs @@ -840,6 +840,87 @@ public void ConfigureEndpoint_DoesNotThrowWhen_HttpsConfigIsDeclaredInEndpointDe Assert.Null(end1.EndpointConfig.SslProtocols); } + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task CertificateChangedOnDisk(bool reloadOnChange) + { + var certificatePath = GetCertificatePath(); + + try + { + var serverOptions = CreateServerOptions(); + + var certificatePassword = "1234"; + + var oldCertificate = new X509Certificate2(TestResources.GetCertPath("aspnetdevcert.pfx"), "testPassword", X509KeyStorageFlags.Exportable); + var oldCertificateBytes = oldCertificate.Export(X509ContentType.Pkcs12, certificatePassword); + + var newCertificate = new X509Certificate2(TestResources.TestCertificatePath, "testPassword", X509KeyStorageFlags.Exportable); + var newCertificateBytes = newCertificate.Export(X509ContentType.Pkcs12, certificatePassword); + + Directory.CreateDirectory(Path.GetDirectoryName(certificatePath)); + File.WriteAllBytes(certificatePath, oldCertificateBytes); + + var endpointConfigurationCallCount = 0; + var config = new ConfigurationBuilder().AddInMemoryCollection(new[] + { + new KeyValuePair("Endpoints:End1:Url", "https://*:5001"), + new KeyValuePair("Endpoints:End1:Certificate:Path", certificatePath), + new KeyValuePair("Endpoints:End1:Certificate:Password", certificatePassword), + }).Build(); + + var configLoader = serverOptions + .Configure(config, reloadOnChange) + .Endpoint("End1", opt => + { + Assert.True(opt.IsHttps); + var expectedSerialNumber = endpointConfigurationCallCount == 0 + ? oldCertificate.SerialNumber + : newCertificate.SerialNumber; + Assert.Equal(opt.HttpsOptions.ServerCertificate.SerialNumber, expectedSerialNumber); + endpointConfigurationCallCount++; + }); + + configLoader.Load(); + + var fileTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + if (reloadOnChange) // There's no reload token if !reloadOnChange + { + configLoader.GetReloadToken().RegisterChangeCallback(_ => fileTcs.SetResult(), state: null); + } + File.WriteAllBytes(certificatePath, newCertificateBytes); + + try + { + await fileTcs.Task.TimeoutAfter(TimeSpan.FromSeconds(10)); + Assert.True(reloadOnChange, "Received an unexpected change event"); + } + catch (TimeoutException) + { + Assert.False(reloadOnChange, "Timed out even though responding to file changes was enabled"); + } + + Assert.Equal(1, endpointConfigurationCallCount); + + if (reloadOnChange) + { + configLoader.Reload(); + + Assert.Equal(2, endpointConfigurationCallCount); + } + } + finally + { + if (File.Exists(certificatePath)) + { + // Note: the watcher will see this event, but we ignore deletions, so it shouldn't matter + File.Delete(certificatePath); + } + } + } + [ConditionalTheory] [InlineData("http1", HttpProtocols.Http1)] // [InlineData("http2", HttpProtocols.Http2)] // Not supported due to missing ALPN support. https://github.com/dotnet/corefx/issues/33016 From fdb7c6d44f9f4e0f6ef2488d301d97bb1d371076 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 10 Aug 2023 14:58:52 -0700 Subject: [PATCH 09/17] Try a different way of updating a file's last write time to address CI flakiness --- src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs b/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs index 51ac376b9977..9a3dafd10460 100644 --- a/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs +++ b/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs @@ -558,7 +558,7 @@ public void UpdateWatches() private static void Touch(string filePath) { - File.Create(filePath).Dispose(); + File.WriteAllBytes(filePath, Array.Empty()); } private static IDictionary GetLogMessageProperties(ITestSink testSink, string eventName) From 0b4e8f4ec0ecdf453a34c67445b39461ebb4212f Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 10 Aug 2023 20:01:51 -0700 Subject: [PATCH 10/17] Speculatively use deterministically distinct names in flaky tests --- src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs b/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs index 9a3dafd10460..586d56844010 100644 --- a/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs +++ b/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs @@ -147,6 +147,7 @@ public void WatchMultipleDirectories(int dirCount, int fileCount) [Theory] [InlineData(1)] + [InlineData(2)] [InlineData(4)] public async Task FileChanged(int observerCount) { @@ -154,7 +155,7 @@ public async Task FileChanged(int observerCount) try { var dir = dirInfo.FullName; - var fileName = Path.GetRandomFileName(); + var fileName = $"{nameof(FileChanged)}-{observerCount}.pfx"; var filePath = Path.Combine(dir, fileName); Touch(filePath); @@ -384,7 +385,7 @@ public async Task IgnoreDeletion(bool deleteDirectory) try { var dir = dirInfo.FullName; - var fileName = Path.GetRandomFileName(); + var fileName = $"{nameof(IgnoreDeletion)}-{deleteDirectory}.pfx"; var filePath = Path.Combine(dir, fileName); Touch(filePath); From 8551da1a697bd698c8486bb9e0bea0527b17e53d Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 10 Aug 2023 20:46:26 -0700 Subject: [PATCH 11/17] Make more aggressive changes to files --- .../Core/test/CertificatePathWatcherTests.cs | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs b/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs index 586d56844010..f7a56883ed19 100644 --- a/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs +++ b/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs @@ -44,7 +44,7 @@ public void AddAndRemoveWatch(bool fileExists, bool absoluteFilePath) if (fileExists) { - Touch(filePath); + ChangeFile(filePath); } Assert.Equal(fileExists, File.Exists(filePath)); @@ -155,10 +155,10 @@ public async Task FileChanged(int observerCount) try { var dir = dirInfo.FullName; - var fileName = $"{nameof(FileChanged)}-{observerCount}.pfx"; + var fileName = Path.GetRandomFileName(); var filePath = Path.Combine(dir, fileName); - Touch(filePath); + ChangeFile(filePath); var hostEnvironment = new Mock(); hostEnvironment.SetupGet(h => h.ContentRootPath).Returns(dir); @@ -187,7 +187,7 @@ public async Task FileChanged(int observerCount) Assert.Equal(1, watcher.TestGetFileWatchCountUnsynchronized(dir)); Assert.Equal(observerCount, watcher.TestGetObserverCountUnsynchronized(filePath)); - Touch(filePath); + ChangeFile(filePath); await signalTcs.Task.TimeoutAfter(FileChangeTimeout); @@ -244,7 +244,7 @@ public void RemoveUnknownFileWatch(bool previouslyAdded) var fileName = Path.GetRandomFileName(); var filePath = Path.Combine(dir, fileName); - Touch(filePath); + ChangeFile(filePath); var hostEnvironment = new Mock(); hostEnvironment.SetupGet(h => h.ContentRootPath).Returns(dir); @@ -291,7 +291,7 @@ public void RemoveUnknownFileObserver(bool previouslyAdded) var fileName = Path.GetRandomFileName(); var filePath = Path.Combine(dir, fileName); - Touch(filePath); + ChangeFile(filePath); var hostEnvironment = new Mock(); hostEnvironment.SetupGet(h => h.ContentRootPath).Returns(dir); @@ -344,7 +344,7 @@ public void ReuseFileObserver() var fileName = Path.GetRandomFileName(); var filePath = Path.Combine(dir, fileName); - Touch(filePath); + ChangeFile(filePath); var hostEnvironment = new Mock(); hostEnvironment.SetupGet(h => h.ContentRootPath).Returns(dir); @@ -385,10 +385,10 @@ public async Task IgnoreDeletion(bool deleteDirectory) try { var dir = dirInfo.FullName; - var fileName = $"{nameof(IgnoreDeletion)}-{deleteDirectory}.pfx"; + var fileName = Path.GetRandomFileName(); var filePath = Path.Combine(dir, fileName); - Touch(filePath); + ChangeFile(filePath); var hostEnvironment = new Mock(); hostEnvironment.SetupGet(h => h.ContentRootPath).Returns(dir); @@ -464,7 +464,7 @@ public async Task IgnoreDeletion(bool deleteDirectory) Assert.True(File.Exists(filePath)); - Touch(filePath); // In some scenarios, renaming the file back doesn't change the last modified time + ChangeFile(filePath); // In some scenarios, renaming the file back doesn't change the last modified time await changeTcs.Task.TimeoutAfter(FileChangeTimeout); } @@ -557,9 +557,10 @@ public void UpdateWatches() } } - private static void Touch(string filePath) + private static void ChangeFile(string filePath) { - File.WriteAllBytes(filePath, Array.Empty()); + File.WriteAllText(filePath, DateTime.UtcNow.ToLongTimeString()); + File.SetLastWriteTimeUtc(filePath, DateTime.UtcNow); } private static IDictionary GetLogMessageProperties(ITestSink testSink, string eventName) From 6cee51a2f4fab3de450032f8c99e2f0180e972db Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 10 Aug 2023 20:49:58 -0700 Subject: [PATCH 12/17] Format log messages consistently --- .../CertificatePathWatcherLoggerExtensions.cs | 44 +++++++++---------- .../Core/test/CertificatePathWatcherTests.cs | 6 +-- 2 files changed, 25 insertions(+), 25 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcherLoggerExtensions.cs b/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcherLoggerExtensions.cs index 97e169e2b555..0ba961ba5a10 100644 --- a/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcherLoggerExtensions.cs +++ b/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcherLoggerExtensions.cs @@ -7,57 +7,57 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal; internal static partial class CertificatePathWatcherLoggerExtensions { - [LoggerMessage(1, LogLevel.Warning, @"Directory ""{Dir}"" does not exist so changes to the certificate ""{Path}"" will not be tracked", EventName = "DirectoryDoesNotExist")] - public static partial void DirectoryDoesNotExist(this ILogger logger, string dir, string path); + [LoggerMessage(1, LogLevel.Warning, @"Directory '{Directory}' does not exist so changes to the certificate '{Path}' will not be tracked.", EventName = "DirectoryDoesNotExist")] + public static partial void DirectoryDoesNotExist(this ILogger logger, string directory, string path); - [LoggerMessage(2, LogLevel.Warning, @"Attempted to remove watch from unwatched path ""{Path}""", EventName = "UnknownFile")] + [LoggerMessage(2, LogLevel.Warning, @"Attempted to remove watch from unwatched path '{Path}'.", EventName = "UnknownFile")] public static partial void UnknownFile(this ILogger logger, string path); - [LoggerMessage(3, LogLevel.Warning, @"Attempted to remove unknown observer from path ""{Path}""", EventName = "UnknownObserver")] + [LoggerMessage(3, LogLevel.Warning, @"Attempted to remove unknown observer from path '{Path}'.", EventName = "UnknownObserver")] public static partial void UnknownObserver(this ILogger logger, string path); - [LoggerMessage(4, LogLevel.Debug, @"Created directory watcher for ""{Dir}""", EventName = "CreatedDirectoryWatcher")] - public static partial void CreatedDirectoryWatcher(this ILogger logger, string dir); + [LoggerMessage(4, LogLevel.Debug, @"Created directory watcher for '{Directory}'.", EventName = "CreatedDirectoryWatcher")] + public static partial void CreatedDirectoryWatcher(this ILogger logger, string directory); - [LoggerMessage(5, LogLevel.Debug, @"Created file watcher for ""{Path}""", EventName = "CreatedFileWatcher")] + [LoggerMessage(5, LogLevel.Debug, @"Created file watcher for '{Path}'.", EventName = "CreatedFileWatcher")] public static partial void CreatedFileWatcher(this ILogger logger, string path); - [LoggerMessage(6, LogLevel.Debug, @"Removed directory watcher for ""{Dir}""", EventName = "RemovedDirectoryWatcher")] - public static partial void RemovedDirectoryWatcher(this ILogger logger, string dir); + [LoggerMessage(6, LogLevel.Debug, @"Removed directory watcher for '{Directory}'.", EventName = "RemovedDirectoryWatcher")] + public static partial void RemovedDirectoryWatcher(this ILogger logger, string directory); - [LoggerMessage(7, LogLevel.Debug, @"Removed file watcher for ""{Path}""", EventName = "RemovedFileWatcher")] + [LoggerMessage(7, LogLevel.Debug, @"Removed file watcher for '{Path}'.", EventName = "RemovedFileWatcher")] public static partial void RemovedFileWatcher(this ILogger logger, string path); - [LoggerMessage(8, LogLevel.Debug, @"Error retrieving last modified time for ""{Path}""", EventName = "LastModifiedTimeError")] + [LoggerMessage(8, LogLevel.Debug, @"Error retrieving last modified time for '{Path}'.", EventName = "LastModifiedTimeError")] public static partial void LastModifiedTimeError(this ILogger logger, string path, Exception e); - [LoggerMessage(9, LogLevel.Debug, @"Ignored event for presently untracked file ""{Path}""", EventName = "UntrackedFileEvent")] + [LoggerMessage(9, LogLevel.Debug, @"Ignored event for presently untracked file '{Path}'.", EventName = "UntrackedFileEvent")] public static partial void UntrackedFileEvent(this ILogger logger, string path); - [LoggerMessage(10, LogLevel.Debug, @"Ignored out-of-order event for file ""{Path}""", EventName = "OutOfOrderEvent")] + [LoggerMessage(10, LogLevel.Debug, @"Ignored out-of-order event for file '{Path}'.", EventName = "OutOfOrderEvent")] public static partial void OutOfOrderEvent(this ILogger logger, string path); - [LoggerMessage(11, LogLevel.Trace, @"Reused existing observer on file watcher for ""{Path}""", EventName = "ReusedObserver")] + [LoggerMessage(11, LogLevel.Trace, @"Reused existing observer on file watcher for '{Path}'.", EventName = "ReusedObserver")] public static partial void ReusedObserver(this ILogger logger, string path); - [LoggerMessage(12, LogLevel.Trace, @"Added observer to file watcher for ""{Path}""", EventName = "AddedObserver")] + [LoggerMessage(12, LogLevel.Trace, @"Added observer to file watcher for '{Path}'.", EventName = "AddedObserver")] public static partial void AddedObserver(this ILogger logger, string path); - [LoggerMessage(13, LogLevel.Trace, @"Removed observer from file watcher for ""{Path}""", EventName = "RemovedObserver")] + [LoggerMessage(13, LogLevel.Trace, @"Removed observer from file watcher for '{Path}'.", EventName = "RemovedObserver")] public static partial void RemovedObserver(this ILogger logger, string path); - [LoggerMessage(14, LogLevel.Trace, @"File ""{Path}"" now has {Count} observers", EventName = "ObserverCount")] + [LoggerMessage(14, LogLevel.Trace, @"File '{Path}' now has {Count} observers.", EventName = "ObserverCount")] public static partial void ObserverCount(this ILogger logger, string path, int count); - [LoggerMessage(15, LogLevel.Trace, @"Directory ""{Dir}"" now has watchers on {Count} files", EventName = "FileCount")] - public static partial void FileCount(this ILogger logger, string dir, int count); + [LoggerMessage(15, LogLevel.Trace, @"Directory '{Directory}' now has watchers on {Count} files.", EventName = "FileCount")] + public static partial void FileCount(this ILogger logger, string directory, int count); - [LoggerMessage(16, LogLevel.Trace, @"Ignored event since last modified time for ""{Path}"" was unavailable", EventName = "EventWithoutLastModifiedTime")] + [LoggerMessage(16, LogLevel.Trace, @"Ignored event since last modified time for '{Path}' was unavailable.", EventName = "EventWithoutLastModifiedTime")] public static partial void EventWithoutLastModifiedTime(this ILogger logger, string path); - [LoggerMessage(17, LogLevel.Trace, @"Ignored redundant event for ""{Path}""", EventName = "RedundantEvent")] + [LoggerMessage(17, LogLevel.Trace, @"Ignored redundant event for '{Path}'.", EventName = "RedundantEvent")] public static partial void RedundantEvent(this ILogger logger, string path); - [LoggerMessage(18, LogLevel.Trace, @"Flagged {Count} observers of ""{Path}"" as changed", EventName = "FlaggedObservers")] + [LoggerMessage(18, LogLevel.Trace, @"Flagged {Count} observers of '{Path}' as changed.", EventName = "FlaggedObservers")] public static partial void FlaggedObservers(this ILogger logger, string path, int count); } diff --git a/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs b/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs index f7a56883ed19..a623e6df648e 100644 --- a/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs +++ b/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs @@ -54,7 +54,7 @@ public void AddAndRemoveWatch(bool fileExists, bool absoluteFilePath) watcher.AddWatchUnsynchronized(certificateConfig); messageProps = GetLogMessageProperties(TestSink, "CreatedDirectoryWatcher"); - Assert.Equal(dir, messageProps["Dir"]); + Assert.Equal(dir, messageProps["Directory"]); messageProps = GetLogMessageProperties(TestSink, "CreatedFileWatcher"); Assert.Equal(filePath, messageProps["Path"]); @@ -69,7 +69,7 @@ public void AddAndRemoveWatch(bool fileExists, bool absoluteFilePath) Assert.Equal(filePath, messageProps["Path"]); messageProps = GetLogMessageProperties(TestSink, "RemovedDirectoryWatcher"); - Assert.Equal(dir, messageProps["Dir"]); + Assert.Equal(dir, messageProps["Directory"]); Assert.Equal(0, watcher.TestGetDirectoryWatchCountUnsynchronized()); Assert.Equal(0, watcher.TestGetFileWatchCountUnsynchronized(dir)); @@ -227,7 +227,7 @@ public void DirectoryDoesNotExist() watcher.AddWatchUnsynchronized(certificateConfig); var messageProps = GetLogMessageProperties(TestSink, "DirectoryDoesNotExist"); - Assert.Equal(dir, messageProps["Dir"]); + Assert.Equal(dir, messageProps["Directory"]); Assert.Equal(0, watcher.TestGetDirectoryWatchCountUnsynchronized()); } From 59702e4d412b1a68089fc1078746b534e857ae38 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 11 Aug 2023 09:55:52 -0700 Subject: [PATCH 13/17] Remove redundant @s --- .../CertificatePathWatcherLoggerExtensions.cs | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcherLoggerExtensions.cs b/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcherLoggerExtensions.cs index 0ba961ba5a10..d41543a342a6 100644 --- a/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcherLoggerExtensions.cs +++ b/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcherLoggerExtensions.cs @@ -7,57 +7,57 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal; internal static partial class CertificatePathWatcherLoggerExtensions { - [LoggerMessage(1, LogLevel.Warning, @"Directory '{Directory}' does not exist so changes to the certificate '{Path}' will not be tracked.", EventName = "DirectoryDoesNotExist")] + [LoggerMessage(1, LogLevel.Warning, "Directory '{Directory}' does not exist so changes to the certificate '{Path}' will not be tracked.", EventName = "DirectoryDoesNotExist")] public static partial void DirectoryDoesNotExist(this ILogger logger, string directory, string path); - [LoggerMessage(2, LogLevel.Warning, @"Attempted to remove watch from unwatched path '{Path}'.", EventName = "UnknownFile")] + [LoggerMessage(2, LogLevel.Warning, "Attempted to remove watch from unwatched path '{Path}'.", EventName = "UnknownFile")] public static partial void UnknownFile(this ILogger logger, string path); - [LoggerMessage(3, LogLevel.Warning, @"Attempted to remove unknown observer from path '{Path}'.", EventName = "UnknownObserver")] + [LoggerMessage(3, LogLevel.Warning, "Attempted to remove unknown observer from path '{Path}'.", EventName = "UnknownObserver")] public static partial void UnknownObserver(this ILogger logger, string path); - [LoggerMessage(4, LogLevel.Debug, @"Created directory watcher for '{Directory}'.", EventName = "CreatedDirectoryWatcher")] + [LoggerMessage(4, LogLevel.Debug, "Created directory watcher for '{Directory}'.", EventName = "CreatedDirectoryWatcher")] public static partial void CreatedDirectoryWatcher(this ILogger logger, string directory); - [LoggerMessage(5, LogLevel.Debug, @"Created file watcher for '{Path}'.", EventName = "CreatedFileWatcher")] + [LoggerMessage(5, LogLevel.Debug, "Created file watcher for '{Path}'.", EventName = "CreatedFileWatcher")] public static partial void CreatedFileWatcher(this ILogger logger, string path); - [LoggerMessage(6, LogLevel.Debug, @"Removed directory watcher for '{Directory}'.", EventName = "RemovedDirectoryWatcher")] + [LoggerMessage(6, LogLevel.Debug, "Removed directory watcher for '{Directory}'.", EventName = "RemovedDirectoryWatcher")] public static partial void RemovedDirectoryWatcher(this ILogger logger, string directory); - [LoggerMessage(7, LogLevel.Debug, @"Removed file watcher for '{Path}'.", EventName = "RemovedFileWatcher")] + [LoggerMessage(7, LogLevel.Debug, "Removed file watcher for '{Path}'.", EventName = "RemovedFileWatcher")] public static partial void RemovedFileWatcher(this ILogger logger, string path); - [LoggerMessage(8, LogLevel.Debug, @"Error retrieving last modified time for '{Path}'.", EventName = "LastModifiedTimeError")] + [LoggerMessage(8, LogLevel.Debug, "Error retrieving last modified time for '{Path}'.", EventName = "LastModifiedTimeError")] public static partial void LastModifiedTimeError(this ILogger logger, string path, Exception e); - [LoggerMessage(9, LogLevel.Debug, @"Ignored event for presently untracked file '{Path}'.", EventName = "UntrackedFileEvent")] + [LoggerMessage(9, LogLevel.Debug, "Ignored event for presently untracked file '{Path}'.", EventName = "UntrackedFileEvent")] public static partial void UntrackedFileEvent(this ILogger logger, string path); - [LoggerMessage(10, LogLevel.Debug, @"Ignored out-of-order event for file '{Path}'.", EventName = "OutOfOrderEvent")] + [LoggerMessage(10, LogLevel.Debug, "Ignored out-of-order event for file '{Path}'.", EventName = "OutOfOrderEvent")] public static partial void OutOfOrderEvent(this ILogger logger, string path); - [LoggerMessage(11, LogLevel.Trace, @"Reused existing observer on file watcher for '{Path}'.", EventName = "ReusedObserver")] + [LoggerMessage(11, LogLevel.Trace, "Reused existing observer on file watcher for '{Path}'.", EventName = "ReusedObserver")] public static partial void ReusedObserver(this ILogger logger, string path); - [LoggerMessage(12, LogLevel.Trace, @"Added observer to file watcher for '{Path}'.", EventName = "AddedObserver")] + [LoggerMessage(12, LogLevel.Trace, "Added observer to file watcher for '{Path}'.", EventName = "AddedObserver")] public static partial void AddedObserver(this ILogger logger, string path); - [LoggerMessage(13, LogLevel.Trace, @"Removed observer from file watcher for '{Path}'.", EventName = "RemovedObserver")] + [LoggerMessage(13, LogLevel.Trace, "Removed observer from file watcher for '{Path}'.", EventName = "RemovedObserver")] public static partial void RemovedObserver(this ILogger logger, string path); - [LoggerMessage(14, LogLevel.Trace, @"File '{Path}' now has {Count} observers.", EventName = "ObserverCount")] + [LoggerMessage(14, LogLevel.Trace, "File '{Path}' now has {Count} observers.", EventName = "ObserverCount")] public static partial void ObserverCount(this ILogger logger, string path, int count); - [LoggerMessage(15, LogLevel.Trace, @"Directory '{Directory}' now has watchers on {Count} files.", EventName = "FileCount")] + [LoggerMessage(15, LogLevel.Trace, "Directory '{Directory}' now has watchers on {Count} files.", EventName = "FileCount")] public static partial void FileCount(this ILogger logger, string directory, int count); - [LoggerMessage(16, LogLevel.Trace, @"Ignored event since last modified time for '{Path}' was unavailable.", EventName = "EventWithoutLastModifiedTime")] + [LoggerMessage(16, LogLevel.Trace, "Ignored event since last modified time for '{Path}' was unavailable.", EventName = "EventWithoutLastModifiedTime")] public static partial void EventWithoutLastModifiedTime(this ILogger logger, string path); - [LoggerMessage(17, LogLevel.Trace, @"Ignored redundant event for '{Path}'.", EventName = "RedundantEvent")] + [LoggerMessage(17, LogLevel.Trace, "Ignored redundant event for '{Path}'.", EventName = "RedundantEvent")] public static partial void RedundantEvent(this ILogger logger, string path); - [LoggerMessage(18, LogLevel.Trace, @"Flagged {Count} observers of '{Path}' as changed.", EventName = "FlaggedObservers")] + [LoggerMessage(18, LogLevel.Trace, "Flagged {Count} observers of '{Path}' as changed.", EventName = "FlaggedObservers")] public static partial void FlaggedObservers(this ILogger logger, string path, int count); } From 87eca27762e20a2ab8399553d673f56934777f11 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 11 Aug 2023 12:39:08 -0700 Subject: [PATCH 14/17] Add an abstraction layer between CertificatePathWatcher and the file system to make testing more reliable --- .../src/Internal/CertificatePathWatcher.cs | 52 +- .../Core/test/CertificatePathWatcherTests.cs | 785 +++++++++--------- 2 files changed, 439 insertions(+), 398 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcher.cs b/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcher.cs index bea01af643ec..38194ca4309c 100644 --- a/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcher.cs +++ b/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcher.cs @@ -13,6 +13,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal; internal sealed partial class CertificatePathWatcher : IDisposable { + private readonly Func _fileProviderFactory; private readonly string _contentRootDir; private readonly ILogger _logger; @@ -27,9 +28,21 @@ internal sealed partial class CertificatePathWatcher : IDisposable private bool _disposed; public CertificatePathWatcher(IHostEnvironment hostEnvironment, ILogger logger) + : this( + hostEnvironment.ContentRootPath, + logger, + dir => Directory.Exists(dir) ? new PhysicalFileProvider(dir, ExclusionFilters.None) : null) { - _contentRootDir = hostEnvironment.ContentRootPath; + } + + /// + /// For testing. + /// + internal CertificatePathWatcher(string contentRootPath, ILogger logger, Func fileProviderFactory) + { + _contentRootDir = contentRootPath; _logger = logger; + _fileProviderFactory = fileProviderFactory; } /// @@ -96,16 +109,16 @@ internal void AddWatchUnsynchronized(CertificateConfig certificateConfig) if (!_metadataForDirectory.TryGetValue(dir, out var dirMetadata)) { - if (!Directory.Exists(dir)) + // If we wanted to detected deletions of this whole directory (which we don't since we ignore deletions), + // we'd probably need to watch the whole directory hierarchy + + var fileProvider = _fileProviderFactory(dir); + if (fileProvider is null) { _logger.DirectoryDoesNotExist(dir, path); return; } - // If we wanted to detected deletions of this whole directory (which we don't since we ignore deletions), - // we'd probably need to watch the whole directory hierarchy - - var fileProvider = new PhysicalFileProvider(dir, ExclusionFilters.None); dirMetadata = new DirectoryWatchMetadata(fileProvider); _metadataForDirectory.Add(dir, dirMetadata); @@ -126,7 +139,7 @@ internal void AddWatchUnsynchronized(CertificateConfig certificateConfig) dirMetadata.FileWatchCount++; // We actually don't care if the file doesn't exist - we'll watch in case it is created - fileMetadata.LastModifiedTime = GetLastModifiedTimeOrMinimum(path); + fileMetadata.LastModifiedTime = GetLastModifiedTimeOrMinimum(path, dirMetadata.FileProvider); _logger.CreatedFileWatcher(path); } @@ -143,22 +156,18 @@ internal void AddWatchUnsynchronized(CertificateConfig certificateConfig) _logger.FileCount(dir, dirMetadata.FileWatchCount); } - private DateTime GetLastModifiedTimeOrMinimum(string path) + private DateTimeOffset GetLastModifiedTimeOrMinimum(string path, IFileProvider fileProvider) { try { - var fileInfo = new FileInfo(path); - if (fileInfo.Exists) - { - return fileInfo.LastWriteTimeUtc; - } + return fileProvider.GetFileInfo(Path.GetFileName(path)).LastModified; } catch (Exception e) { _logger.LastModifiedTimeError(path, e); } - return DateTime.MinValue; + return DateTimeOffset.MinValue; } private void OnChange(string path) @@ -172,6 +181,9 @@ private void OnChange(string path) return; } + // Existence implied by the fact that we're tracking the file + var dirMetadata = _metadataForDirectory[Path.GetDirectoryName(path)!]; + // We ignore file changes that don't advance the last modified time. // For example, if we lose access to the network share the file is // stored on, we don't notify our listeners because no one wants @@ -180,14 +192,14 @@ private void OnChange(string path) // before a new cert is introduced with the old name. // This also helps us in scenarios where the underlying file system // reports more than one change for a single logical operation. - var lastModifiedTime = GetLastModifiedTimeOrMinimum(path); + var lastModifiedTime = GetLastModifiedTimeOrMinimum(path, dirMetadata.FileProvider); if (lastModifiedTime > fileMetadata.LastModifiedTime) { fileMetadata.LastModifiedTime = lastModifiedTime; } else { - if (lastModifiedTime == DateTime.MinValue) + if (lastModifiedTime == DateTimeOffset.MinValue) { _logger.EventWithoutLastModifiedTime(path); } @@ -299,19 +311,19 @@ void IDisposable.Dispose() GC.SuppressFinalize(this); } - private sealed class DirectoryWatchMetadata(PhysicalFileProvider fileProvider) : IDisposable + private sealed class DirectoryWatchMetadata(IFileProvider fileProvider) : IDisposable { - public readonly PhysicalFileProvider FileProvider = fileProvider; + public readonly IFileProvider FileProvider = fileProvider; public int FileWatchCount; - public void Dispose() => FileProvider.Dispose(); + public void Dispose() => (FileProvider as IDisposable)?.Dispose(); } private sealed class FileWatchMetadata(IDisposable disposable) : IDisposable { public readonly IDisposable Disposable = disposable; public readonly HashSet Configs = new(ReferenceEqualityComparer.Instance); - public DateTime LastModifiedTime = DateTime.MinValue; + public DateTimeOffset LastModifiedTime = DateTimeOffset.MinValue; public void Dispose() => Disposable.Dispose(); } diff --git a/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs b/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs index a623e6df648e..b4be5350f81c 100644 --- a/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs +++ b/src/Servers/Kestrel/Core/test/CertificatePathWatcherTests.cs @@ -3,85 +3,64 @@ using Microsoft.AspNetCore.Server.Kestrel.Core.Internal; using Microsoft.AspNetCore.Testing; -using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.FileProviders; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Testing; -using Moq; +using Microsoft.Extensions.Primitives; namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests; public class CertificatePathWatcherTests : LoggedTest { - private static readonly TimeSpan FileChangeTimeout = TimeSpan.FromSeconds(10); - [Theory] - [InlineData(true, true)] - [InlineData(true, false)] - [InlineData(false, true)] - [InlineData(false, false)] - public void AddAndRemoveWatch(bool fileExists, bool absoluteFilePath) + [InlineData(true)] + [InlineData(false)] + public void AddAndRemoveWatch(bool absoluteFilePath) { - var dirInfo = Directory.CreateTempSubdirectory(); - try - { - var dir = dirInfo.FullName; - var fileName = Path.GetRandomFileName(); - var filePath = Path.Combine(dir, fileName); - - var hostEnvironment = new Mock(); - hostEnvironment.SetupGet(h => h.ContentRootPath).Returns(dir); + var dir = Directory.GetCurrentDirectory(); + var fileName = Path.GetRandomFileName(); + var filePath = Path.Combine(dir, fileName); - var logger = LoggerFactory.CreateLogger(); - - using var watcher = new CertificatePathWatcher(hostEnvironment.Object, logger); - - var changeToken = watcher.GetChangeToken(); + var logger = LoggerFactory.CreateLogger(); - var certificateConfig = new CertificateConfig - { - Path = absoluteFilePath ? filePath : fileName, - }; + using var watcher = new CertificatePathWatcher(dir, logger, _ => NoChangeFileProvider.Instance); - if (fileExists) - { - ChangeFile(filePath); - } + var changeToken = watcher.GetChangeToken(); - Assert.Equal(fileExists, File.Exists(filePath)); + var certificateConfig = new CertificateConfig + { + Path = absoluteFilePath ? filePath : fileName, + }; - IDictionary messageProps; + IDictionary messageProps; - watcher.AddWatchUnsynchronized(certificateConfig); + watcher.AddWatchUnsynchronized(certificateConfig); - messageProps = GetLogMessageProperties(TestSink, "CreatedDirectoryWatcher"); - Assert.Equal(dir, messageProps["Directory"]); + messageProps = GetLogMessageProperties(TestSink, "CreatedDirectoryWatcher"); + Assert.Equal(dir, messageProps["Directory"]); - messageProps = GetLogMessageProperties(TestSink, "CreatedFileWatcher"); - Assert.Equal(filePath, messageProps["Path"]); + messageProps = GetLogMessageProperties(TestSink, "CreatedFileWatcher"); + Assert.Equal(filePath, messageProps["Path"]); - Assert.Equal(1, watcher.TestGetDirectoryWatchCountUnsynchronized()); - Assert.Equal(1, watcher.TestGetFileWatchCountUnsynchronized(dir)); - Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); + Assert.Equal(1, watcher.TestGetDirectoryWatchCountUnsynchronized()); + Assert.Equal(1, watcher.TestGetFileWatchCountUnsynchronized(dir)); + Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); - watcher.RemoveWatchUnsynchronized(certificateConfig); + watcher.RemoveWatchUnsynchronized(certificateConfig); - messageProps = GetLogMessageProperties(TestSink, "RemovedFileWatcher"); - Assert.Equal(filePath, messageProps["Path"]); + messageProps = GetLogMessageProperties(TestSink, "RemovedFileWatcher"); + Assert.Equal(filePath, messageProps["Path"]); - messageProps = GetLogMessageProperties(TestSink, "RemovedDirectoryWatcher"); - Assert.Equal(dir, messageProps["Directory"]); + messageProps = GetLogMessageProperties(TestSink, "RemovedDirectoryWatcher"); + Assert.Equal(dir, messageProps["Directory"]); - Assert.Equal(0, watcher.TestGetDirectoryWatchCountUnsynchronized()); - Assert.Equal(0, watcher.TestGetFileWatchCountUnsynchronized(dir)); - Assert.Equal(0, watcher.TestGetObserverCountUnsynchronized(filePath)); + Assert.Equal(0, watcher.TestGetDirectoryWatchCountUnsynchronized()); + Assert.Equal(0, watcher.TestGetFileWatchCountUnsynchronized(dir)); + Assert.Equal(0, watcher.TestGetObserverCountUnsynchronized(filePath)); - Assert.Same(changeToken, watcher.GetChangeToken()); - Assert.False(changeToken.HasChanged); - } - finally - { - dirInfo.Delete(recursive: true); - } + Assert.Same(changeToken, watcher.GetChangeToken()); + Assert.False(changeToken.HasChanged); } [Theory] @@ -90,59 +69,47 @@ public void AddAndRemoveWatch(bool fileExists, bool absoluteFilePath) [InlineData(5, 13)] public void WatchMultipleDirectories(int dirCount, int fileCount) { - var rootDirInfo = Directory.CreateTempSubdirectory(); - try - { - var rootDir = rootDirInfo.FullName; - var dirs = new string[dirCount]; + var rootDir = Directory.GetCurrentDirectory(); + var dirs = new string[dirCount]; - var hostEnvironment = new Mock(); - hostEnvironment.SetupGet(h => h.ContentRootPath).Returns(rootDir); - - var logger = LoggerFactory.CreateLogger(); - - for (int i = 0; i < dirCount; i++) - { - dirs[i] = Path.Combine(rootDir, $"dir{i}"); - Directory.CreateDirectory(dirs[i]); - } + var logger = LoggerFactory.CreateLogger(); - using var watcher = new CertificatePathWatcher(hostEnvironment.Object, logger); + for (int i = 0; i < dirCount; i++) + { + dirs[i] = Path.Combine(rootDir, $"dir{i}"); + } - var certificateConfigs = new CertificateConfig[fileCount]; - var filesInDir = new int[dirCount]; - for (int i = 0; i < fileCount; i++) - { - certificateConfigs[i] = new CertificateConfig - { - Path = $"dir{i % dirCount}/file{i % fileCount}", - }; - filesInDir[i % dirCount]++; - } + using var watcher = new CertificatePathWatcher(rootDir, logger, _ => NoChangeFileProvider.Instance); - foreach (var certificateConfig in certificateConfigs) + var certificateConfigs = new CertificateConfig[fileCount]; + var filesInDir = new int[dirCount]; + for (int i = 0; i < fileCount; i++) + { + certificateConfigs[i] = new CertificateConfig { - watcher.AddWatchUnsynchronized(certificateConfig); - } - - Assert.Equal(Math.Min(dirCount, fileCount), watcher.TestGetDirectoryWatchCountUnsynchronized()); + Path = $"dir{i % dirCount}/file{i % fileCount}", + }; + filesInDir[i % dirCount]++; + } - for (int i = 0; i < dirCount; i++) - { - Assert.Equal(filesInDir[i], watcher.TestGetFileWatchCountUnsynchronized(dirs[i])); - } + foreach (var certificateConfig in certificateConfigs) + { + watcher.AddWatchUnsynchronized(certificateConfig); + } - foreach (var certificateConfig in certificateConfigs) - { - watcher.RemoveWatchUnsynchronized(certificateConfig); - } + Assert.Equal(Math.Min(dirCount, fileCount), watcher.TestGetDirectoryWatchCountUnsynchronized()); - Assert.Equal(0, watcher.TestGetDirectoryWatchCountUnsynchronized()); + for (int i = 0; i < dirCount; i++) + { + Assert.Equal(filesInDir[i], watcher.TestGetFileWatchCountUnsynchronized(dirs[i])); } - finally + + foreach (var certificateConfig in certificateConfigs) { - rootDirInfo.Delete(recursive: true); + watcher.RemoveWatchUnsynchronized(certificateConfig); } + + Assert.Equal(0, watcher.TestGetDirectoryWatchCountUnsynchronized()); } [Theory] @@ -151,58 +118,98 @@ public void WatchMultipleDirectories(int dirCount, int fileCount) [InlineData(4)] public async Task FileChanged(int observerCount) { - var dirInfo = Directory.CreateTempSubdirectory(); - try + var dir = Directory.GetCurrentDirectory(); + var fileName = Path.GetRandomFileName(); + var filePath = Path.Combine(dir, fileName); + + var logger = LoggerFactory.CreateLogger(); + + var fileProvider = new MockFileProvider(); + var fileLastModifiedTime = DateTimeOffset.UtcNow; + fileProvider.SetLastModifiedTime(fileName, fileLastModifiedTime); + + using var watcher = new CertificatePathWatcher(dir, logger, _ => fileProvider); + + var signalTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + var oldChangeToken = watcher.GetChangeToken(); + oldChangeToken.RegisterChangeCallback(_ => signalTcs.SetResult(), state: null); + + var certificateConfigs = new CertificateConfig[observerCount]; + for (int i = 0; i < observerCount; i++) { - var dir = dirInfo.FullName; - var fileName = Path.GetRandomFileName(); - var filePath = Path.Combine(dir, fileName); + certificateConfigs[i] = new CertificateConfig + { + Path = filePath, + }; - ChangeFile(filePath); + watcher.AddWatchUnsynchronized(certificateConfigs[i]); + } - var hostEnvironment = new Mock(); - hostEnvironment.SetupGet(h => h.ContentRootPath).Returns(dir); + Assert.Equal(1, watcher.TestGetDirectoryWatchCountUnsynchronized()); + Assert.Equal(1, watcher.TestGetFileWatchCountUnsynchronized(dir)); + Assert.Equal(observerCount, watcher.TestGetObserverCountUnsynchronized(filePath)); - var logger = LoggerFactory.CreateLogger(); + // Simulate file change on disk + fileProvider.SetLastModifiedTime(fileName, fileLastModifiedTime.AddSeconds(1)); + fileProvider.FireChangeToken(fileName); - using var watcher = new CertificatePathWatcher(hostEnvironment.Object, logger); + await signalTcs.Task.DefaultTimeout(); - var signalTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var newChangeToken = watcher.GetChangeToken(); - var oldChangeToken = watcher.GetChangeToken(); - oldChangeToken.RegisterChangeCallback(_ => signalTcs.SetResult(), state: null); + Assert.NotSame(oldChangeToken, newChangeToken); + Assert.True(oldChangeToken.HasChanged); + Assert.False(newChangeToken.HasChanged); - var certificateConfigs = new CertificateConfig[observerCount]; - for (int i = 0; i < observerCount; i++) - { - certificateConfigs[i] = new CertificateConfig - { - Path = filePath, - }; + Assert.All(certificateConfigs, cc => Assert.True(cc.FileHasChanged)); + } - watcher.AddWatchUnsynchronized(certificateConfigs[i]); - } + [Fact] + public async Task OutOfOrderLastModifiedTime() + { + var dir = Directory.GetCurrentDirectory(); + var fileName = Path.GetRandomFileName(); + var filePath = Path.Combine(dir, fileName); - Assert.Equal(1, watcher.TestGetDirectoryWatchCountUnsynchronized()); - Assert.Equal(1, watcher.TestGetFileWatchCountUnsynchronized(dir)); - Assert.Equal(observerCount, watcher.TestGetObserverCountUnsynchronized(filePath)); + var logger = LoggerFactory.CreateLogger(); - ChangeFile(filePath); + var fileProvider = new MockFileProvider(); + var fileLastModifiedTime = DateTimeOffset.UtcNow; + fileProvider.SetLastModifiedTime(fileName, fileLastModifiedTime); - await signalTcs.Task.TimeoutAfter(FileChangeTimeout); + using var watcher = new CertificatePathWatcher(dir, logger, _ => fileProvider); - var newChangeToken = watcher.GetChangeToken(); + var certificateConfig = new CertificateConfig + { + Path = filePath, + }; - Assert.NotSame(oldChangeToken, newChangeToken); - Assert.True(oldChangeToken.HasChanged); - Assert.False(newChangeToken.HasChanged); + watcher.AddWatchUnsynchronized(certificateConfig); - Assert.All(certificateConfigs, cc => Assert.True(cc.FileHasChanged)); - } - finally + var logTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + TestSink.MessageLogged += writeContext => { - dirInfo.Delete(recursive: true); - } + if (writeContext.EventId.Name == "OutOfOrderEvent") + { + logTcs.SetResult(); + } + }; + + var oldChangeToken = watcher.GetChangeToken(); + + Assert.Equal(1, watcher.TestGetDirectoryWatchCountUnsynchronized()); + Assert.Equal(1, watcher.TestGetFileWatchCountUnsynchronized(dir)); + Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); + + // Simulate file change on disk + fileProvider.SetLastModifiedTime(fileName, fileLastModifiedTime.AddSeconds(-1)); + fileProvider.FireChangeToken(fileName); + + await logTcs.Task.DefaultTimeout(); + + Assert.False(oldChangeToken.HasChanged); } [Fact] @@ -212,12 +219,10 @@ public void DirectoryDoesNotExist() Assert.False(Directory.Exists(dir)); - var hostEnvironment = new Mock(); - hostEnvironment.SetupGet(h => h.ContentRootPath).Returns(dir); - var logger = LoggerFactory.CreateLogger(); - using var watcher = new CertificatePathWatcher(hostEnvironment.Object, logger); + // Returning null indicates that the directory does not exist + using var watcher = new CertificatePathWatcher(dir, logger, _ => null); var certificateConfig = new CertificateConfig { @@ -237,46 +242,33 @@ public void DirectoryDoesNotExist() [InlineData(false)] public void RemoveUnknownFileWatch(bool previouslyAdded) { - var dirInfo = Directory.CreateTempSubdirectory(); - try - { - var dir = dirInfo.FullName; - var fileName = Path.GetRandomFileName(); - var filePath = Path.Combine(dir, fileName); - - ChangeFile(filePath); - - var hostEnvironment = new Mock(); - hostEnvironment.SetupGet(h => h.ContentRootPath).Returns(dir); + var dir = Directory.GetCurrentDirectory(); + var fileName = Path.GetRandomFileName(); + var filePath = Path.Combine(dir, fileName); - var logger = LoggerFactory.CreateLogger(); + var logger = LoggerFactory.CreateLogger(); - using var watcher = new CertificatePathWatcher(hostEnvironment.Object, logger); + using var watcher = new CertificatePathWatcher(dir, logger, _ => NoChangeFileProvider.Instance); - var certificateConfig = new CertificateConfig - { - Path = filePath, - }; + var certificateConfig = new CertificateConfig + { + Path = filePath, + }; - if (previouslyAdded) - { - watcher.AddWatchUnsynchronized(certificateConfig); - watcher.RemoveWatchUnsynchronized(certificateConfig); - } + if (previouslyAdded) + { + watcher.AddWatchUnsynchronized(certificateConfig); + watcher.RemoveWatchUnsynchronized(certificateConfig); + } - Assert.Equal(0, watcher.TestGetObserverCountUnsynchronized(filePath)); + Assert.Equal(0, watcher.TestGetObserverCountUnsynchronized(filePath)); - watcher.RemoveWatchUnsynchronized(certificateConfig); + watcher.RemoveWatchUnsynchronized(certificateConfig); - var messageProps = GetLogMessageProperties(TestSink, "UnknownFile"); - Assert.Equal(filePath, messageProps["Path"]); + var messageProps = GetLogMessageProperties(TestSink, "UnknownFile"); + Assert.Equal(filePath, messageProps["Path"]); - Assert.Equal(0, watcher.TestGetObserverCountUnsynchronized(filePath)); - } - finally - { - dirInfo.Delete(recursive: true); - } + Assert.Equal(0, watcher.TestGetObserverCountUnsynchronized(filePath)); } [Theory] @@ -284,290 +276,327 @@ public void RemoveUnknownFileWatch(bool previouslyAdded) [InlineData(false)] public void RemoveUnknownFileObserver(bool previouslyAdded) { - var dirInfo = Directory.CreateTempSubdirectory(); - try - { - var dir = dirInfo.FullName; - var fileName = Path.GetRandomFileName(); - var filePath = Path.Combine(dir, fileName); - - ChangeFile(filePath); - - var hostEnvironment = new Mock(); - hostEnvironment.SetupGet(h => h.ContentRootPath).Returns(dir); + var dir = Directory.GetCurrentDirectory(); + var fileName = Path.GetRandomFileName(); + var filePath = Path.Combine(dir, fileName); - var logger = LoggerFactory.CreateLogger(); + var logger = LoggerFactory.CreateLogger(); - using var watcher = new CertificatePathWatcher(hostEnvironment.Object, logger); + using var watcher = new CertificatePathWatcher(dir, logger, _ => NoChangeFileProvider.Instance); - var certificateConfig1 = new CertificateConfig - { - Path = filePath, - }; + var certificateConfig1 = new CertificateConfig + { + Path = filePath, + }; - var certificateConfig2 = new CertificateConfig - { - Path = filePath, - }; + var certificateConfig2 = new CertificateConfig + { + Path = filePath, + }; - watcher.AddWatchUnsynchronized(certificateConfig1); + watcher.AddWatchUnsynchronized(certificateConfig1); - if (previouslyAdded) - { - watcher.AddWatchUnsynchronized(certificateConfig2); - watcher.RemoveWatchUnsynchronized(certificateConfig2); - } + if (previouslyAdded) + { + watcher.AddWatchUnsynchronized(certificateConfig2); + watcher.RemoveWatchUnsynchronized(certificateConfig2); + } - Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); + Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); - watcher.RemoveWatchUnsynchronized(certificateConfig2); + watcher.RemoveWatchUnsynchronized(certificateConfig2); - var messageProps = GetLogMessageProperties(TestSink, "UnknownObserver"); - Assert.Equal(filePath, messageProps["Path"]); + var messageProps = GetLogMessageProperties(TestSink, "UnknownObserver"); + Assert.Equal(filePath, messageProps["Path"]); - Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); - } - finally - { - dirInfo.Delete(recursive: true); - } + Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); } [Fact] [LogLevel(LogLevel.Trace)] public void ReuseFileObserver() { - var dirInfo = Directory.CreateTempSubdirectory(); - try - { - var dir = dirInfo.FullName; - var fileName = Path.GetRandomFileName(); - var filePath = Path.Combine(dir, fileName); - - ChangeFile(filePath); - - var hostEnvironment = new Mock(); - hostEnvironment.SetupGet(h => h.ContentRootPath).Returns(dir); + var dir = Directory.GetCurrentDirectory(); + var fileName = Path.GetRandomFileName(); + var filePath = Path.Combine(dir, fileName); - var logger = LoggerFactory.CreateLogger(); + var logger = LoggerFactory.CreateLogger(); - using var watcher = new CertificatePathWatcher(hostEnvironment.Object, logger); + using var watcher = new CertificatePathWatcher(dir, logger, _ => NoChangeFileProvider.Instance); - var certificateConfig = new CertificateConfig - { - Path = filePath, - }; + var certificateConfig = new CertificateConfig + { + Path = filePath, + }; - watcher.AddWatchUnsynchronized(certificateConfig); + watcher.AddWatchUnsynchronized(certificateConfig); - Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); + Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); - watcher.AddWatchUnsynchronized(certificateConfig); + watcher.AddWatchUnsynchronized(certificateConfig); - var messageProps = GetLogMessageProperties(TestSink, "ReusedObserver"); - Assert.Equal(filePath, messageProps["Path"]); + var messageProps = GetLogMessageProperties(TestSink, "ReusedObserver"); + Assert.Equal(filePath, messageProps["Path"]); - Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); - } - finally - { - dirInfo.Delete(recursive: true); - } + Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); } [Theory] - [InlineData(true)] - [InlineData(false)] + [InlineData(true, true)] + [InlineData(true, false)] + [InlineData(false, true)] + [InlineData(false, false)] [LogLevel(LogLevel.Trace)] - public async Task IgnoreDeletion(bool deleteDirectory) + public async Task IgnoreDeletion(bool seeChangeForDeletion, bool restoredWithNewerLastModifiedTime) { - var dirInfo = Directory.CreateTempSubdirectory(); - try - { - var dir = dirInfo.FullName; - var fileName = Path.GetRandomFileName(); - var filePath = Path.Combine(dir, fileName); - - ChangeFile(filePath); + var dir = Directory.GetCurrentDirectory(); + var fileName = Path.GetRandomFileName(); + var filePath = Path.Combine(dir, fileName); - var hostEnvironment = new Mock(); - hostEnvironment.SetupGet(h => h.ContentRootPath).Returns(dir); - - var logger = LoggerFactory.CreateLogger(); + var logger = LoggerFactory.CreateLogger(); - using var watcher = new CertificatePathWatcher(hostEnvironment.Object, logger); + var fileProvider = new MockFileProvider(); + var fileLastModifiedTime = DateTimeOffset.UtcNow; + fileProvider.SetLastModifiedTime(fileName, fileLastModifiedTime); - var certificateConfig = new CertificateConfig - { - Path = filePath, - }; + using var watcher = new CertificatePathWatcher(dir, logger, _ => fileProvider); - watcher.AddWatchUnsynchronized(certificateConfig); + var certificateConfig = new CertificateConfig + { + Path = filePath, + }; - Assert.Equal(1, watcher.TestGetDirectoryWatchCountUnsynchronized()); - Assert.Equal(1, watcher.TestGetFileWatchCountUnsynchronized(dir)); - Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); + watcher.AddWatchUnsynchronized(certificateConfig); - var changeTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + Assert.Equal(1, watcher.TestGetDirectoryWatchCountUnsynchronized()); + Assert.Equal(1, watcher.TestGetFileWatchCountUnsynchronized(dir)); + Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); - watcher.GetChangeToken().RegisterChangeCallback(_ => changeTcs.SetResult(), state: null); + var changeTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); - var logTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + watcher.GetChangeToken().RegisterChangeCallback(_ => changeTcs.SetResult(), state: null); - TestSink.MessageLogged += writeContext => - { - if (writeContext.EventId.Name == "EventWithoutLastModifiedTime") - { - logTcs.SetResult(); - } - }; + var logNoLastModifiedTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var logSameLastModifiedTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); - // "Delete" the file or directory - if (deleteDirectory) + TestSink.MessageLogged += writeContext => + { + if (writeContext.EventId.Name == "EventWithoutLastModifiedTime") { - dirInfo.MoveTo(dir + ".bak"); + logNoLastModifiedTcs.SetResult(); } - else + else if (writeContext.EventId.Name == "RedundantEvent") { - File.Move(filePath, filePath + ".bak"); + logSameLastModifiedTcs.SetResult(); } + }; - Assert.False(File.Exists(filePath)); - - try - { - await logTcs.Task.TimeoutAfter(FileChangeTimeout); - } - catch (TimeoutException) - { - // In some scenarios, the directory deletion won't trigger an event. - // For example, a `FileSystemWatcher` on Windows won't fire one, - // whereas the polling watcher will. - Assert.True(deleteDirectory); - } + // Simulate file deletion + fileProvider.SetLastModifiedTime(fileName, null); - Assert.Equal(1, watcher.TestGetDirectoryWatchCountUnsynchronized()); - Assert.Equal(1, watcher.TestGetFileWatchCountUnsynchronized(dir)); - Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); + // In some file systems and watch modes, there's no event when (e.g.) the directory containing the watched file is deleted + if (seeChangeForDeletion) + { + fileProvider.FireChangeToken(fileName); - Assert.False(changeTcs.Task.IsCompleted); + await logNoLastModifiedTcs.Task.DefaultTimeout(); + } - // Restore the file or directory - if (deleteDirectory) - { - dirInfo.MoveTo(dir); - } - else - { - File.Move(filePath + ".bak", filePath); - } + Assert.Equal(1, watcher.TestGetDirectoryWatchCountUnsynchronized()); + Assert.Equal(1, watcher.TestGetFileWatchCountUnsynchronized(dir)); + Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); - Assert.True(File.Exists(filePath)); + Assert.False(changeTcs.Task.IsCompleted); - ChangeFile(filePath); // In some scenarios, renaming the file back doesn't change the last modified time + // Restore the file + fileProvider.SetLastModifiedTime(fileName, restoredWithNewerLastModifiedTime ? fileLastModifiedTime.AddSeconds(1) : fileLastModifiedTime); + fileProvider.FireChangeToken(fileName); - await changeTcs.Task.TimeoutAfter(FileChangeTimeout); + if (restoredWithNewerLastModifiedTime) + { + await changeTcs.Task.DefaultTimeout(); + Assert.False(logSameLastModifiedTcs.Task.IsCompleted); } - finally + else { - dirInfo.Delete(recursive: true); + await logSameLastModifiedTcs.Task.DefaultTimeout(); + Assert.False(changeTcs.Task.IsCompleted); } } [Fact] public void UpdateWatches() { - var dirInfo = Directory.CreateTempSubdirectory(); - try + var dir = Directory.GetCurrentDirectory(); + var fileName = Path.GetRandomFileName(); + var filePath = Path.Combine(dir, fileName); + + var logger = LoggerFactory.CreateLogger(); + + using var watcher = new CertificatePathWatcher(dir, logger, _ => NoChangeFileProvider.Instance); + + var changeToken = watcher.GetChangeToken(); + + var certificateConfig1 = new CertificateConfig { - var dir = dirInfo.FullName; - var fileName = Path.GetRandomFileName(); - var filePath = Path.Combine(dir, fileName); + Path = filePath, + }; - var hostEnvironment = new Mock(); - hostEnvironment.SetupGet(h => h.ContentRootPath).Returns(dir); + var certificateConfig2 = new CertificateConfig + { + Path = filePath, + }; - var logger = LoggerFactory.CreateLogger(); + var certificateConfig3 = new CertificateConfig + { + Path = filePath, + }; - using var watcher = new CertificatePathWatcher(hostEnvironment.Object, logger); + // Add certificateConfig1 + watcher.UpdateWatches(new List { }, new List { certificateConfig1 }); - var changeToken = watcher.GetChangeToken(); + Assert.Equal(1, watcher.TestGetDirectoryWatchCountUnsynchronized()); + Assert.Equal(1, watcher.TestGetFileWatchCountUnsynchronized(dir)); + Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); - var certificateConfig1 = new CertificateConfig - { - Path = filePath, - }; + // Remove certificateConfig1 + watcher.UpdateWatches(new List { certificateConfig1 }, new List { }); - var certificateConfig2 = new CertificateConfig - { - Path = filePath, - }; + Assert.Equal(0, watcher.TestGetDirectoryWatchCountUnsynchronized()); + Assert.Equal(0, watcher.TestGetFileWatchCountUnsynchronized(dir)); + Assert.Equal(0, watcher.TestGetObserverCountUnsynchronized(filePath)); - var certificateConfig3 = new CertificateConfig + // Re-add certificateConfig1 + watcher.UpdateWatches(new List { }, new List { certificateConfig1 }); + + Assert.Equal(1, watcher.TestGetDirectoryWatchCountUnsynchronized()); + Assert.Equal(1, watcher.TestGetFileWatchCountUnsynchronized(dir)); + Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); + + watcher.UpdateWatches( + new List { - Path = filePath, - }; + certificateConfig1, // Delete something present + certificateConfig1, // Delete it again + certificateConfig2, // Delete something never added + certificateConfig2, // Delete it again + }, + new List + { + certificateConfig1, // Re-add something removed above + certificateConfig1, // Re-add it again + certificateConfig2, // Add something vacuously removed above + certificateConfig2, // Add it again + certificateConfig3, // Add something new + certificateConfig3, // Add it again + }); + + Assert.Equal(1, watcher.TestGetDirectoryWatchCountUnsynchronized()); + Assert.Equal(1, watcher.TestGetFileWatchCountUnsynchronized(dir)); + Assert.Equal(3, watcher.TestGetObserverCountUnsynchronized(filePath)); + } - // Add certificateConfig1 - watcher.UpdateWatches(new List { }, new List { certificateConfig1 }); - - Assert.Equal(1, watcher.TestGetDirectoryWatchCountUnsynchronized()); - Assert.Equal(1, watcher.TestGetFileWatchCountUnsynchronized(dir)); - Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); - - // Remove certificateConfig1 - watcher.UpdateWatches(new List { certificateConfig1 }, new List { }); - - Assert.Equal(0, watcher.TestGetDirectoryWatchCountUnsynchronized()); - Assert.Equal(0, watcher.TestGetFileWatchCountUnsynchronized(dir)); - Assert.Equal(0, watcher.TestGetObserverCountUnsynchronized(filePath)); - - // Re-add certificateConfig1 - watcher.UpdateWatches(new List { }, new List { certificateConfig1 }); - - Assert.Equal(1, watcher.TestGetDirectoryWatchCountUnsynchronized()); - Assert.Equal(1, watcher.TestGetFileWatchCountUnsynchronized(dir)); - Assert.Equal(1, watcher.TestGetObserverCountUnsynchronized(filePath)); - - watcher.UpdateWatches( - new List - { - certificateConfig1, // Delete something present - certificateConfig1, // Delete it again - certificateConfig2, // Delete something never added - certificateConfig2, // Delete it again - }, - new List - { - certificateConfig1, // Re-add something removed above - certificateConfig1, // Re-add it again - certificateConfig2, // Add something vacuously removed above - certificateConfig2, // Add it again - certificateConfig3, // Add something new - certificateConfig3, // Add it again - }); - - Assert.Equal(1, watcher.TestGetDirectoryWatchCountUnsynchronized()); - Assert.Equal(1, watcher.TestGetFileWatchCountUnsynchronized(dir)); - Assert.Equal(3, watcher.TestGetObserverCountUnsynchronized(filePath)); + private static IDictionary GetLogMessageProperties(ITestSink testSink, string eventName) + { + var writeContext = Assert.Single(testSink.Writes.Where(wc => wc.EventId.Name == eventName)); + var pairs = (IReadOnlyList>)writeContext.State; + var dict = new Dictionary(pairs); + return dict; + } + + private sealed class NoChangeFileProvider : IFileProvider + { + public static readonly IFileProvider Instance = new NoChangeFileProvider(); + + private NoChangeFileProvider() + { } - finally + + IDirectoryContents IFileProvider.GetDirectoryContents(string subpath) => throw new NotSupportedException(); + IFileInfo IFileProvider.GetFileInfo(string subpath) => throw new NotSupportedException(); + IChangeToken IFileProvider.Watch(string filter) => NoChangeChangeToken.Instance; + + private sealed class NoChangeChangeToken : IChangeToken { - dirInfo.Delete(recursive: true); + public static readonly IChangeToken Instance = new NoChangeChangeToken(); + + private NoChangeChangeToken() + { + } + + bool IChangeToken.HasChanged => false; + bool IChangeToken.ActiveChangeCallbacks => true; + IDisposable IChangeToken.RegisterChangeCallback(Action callback, object state) => DummyDisposable.Instance; } } - private static void ChangeFile(string filePath) + private sealed class DummyDisposable : IDisposable { - File.WriteAllText(filePath, DateTime.UtcNow.ToLongTimeString()); - File.SetLastWriteTimeUtc(filePath, DateTime.UtcNow); + public static readonly IDisposable Instance = new DummyDisposable(); + + private DummyDisposable() + { + } + + void IDisposable.Dispose() + { + } } - private static IDictionary GetLogMessageProperties(ITestSink testSink, string eventName) + private sealed class MockFileProvider : IFileProvider { - var writeContext = Assert.Single(testSink.Writes.Where(wc => wc.EventId.Name == eventName)); - var pairs = (IReadOnlyList>)writeContext.State; - var dict = new Dictionary(pairs); - return dict; + private readonly Dictionary _changeTokens = new(); + private readonly Dictionary _lastModifiedTimes = new(); + + public void FireChangeToken(string path) + { + var oldChangeToken = _changeTokens[path]; + _changeTokens[path] = new ConfigurationReloadToken(); + oldChangeToken.OnReload(); + } + + public void SetLastModifiedTime(string path, DateTimeOffset? lastModifiedTime) + { + _lastModifiedTimes[path] = lastModifiedTime; + } + + IDirectoryContents IFileProvider.GetDirectoryContents(string subpath) + { + throw new NotSupportedException(); + } + + IFileInfo IFileProvider.GetFileInfo(string subpath) + { + return new MockFileInfo(_lastModifiedTimes[subpath]); + } + + IChangeToken IFileProvider.Watch(string path) + { + if (!_changeTokens.TryGetValue(path, out var changeToken)) + { + _changeTokens[path] = changeToken = new ConfigurationReloadToken(); + } + + return changeToken; + } + + private sealed class MockFileInfo : IFileInfo + { + private readonly DateTimeOffset? _lastModifiedTime; + + public MockFileInfo(DateTimeOffset? lastModifiedTime) + { + _lastModifiedTime = lastModifiedTime; + } + + bool IFileInfo.Exists => _lastModifiedTime.HasValue; + DateTimeOffset IFileInfo.LastModified => _lastModifiedTime.GetValueOrDefault(); + + long IFileInfo.Length => throw new NotSupportedException(); + string IFileInfo.PhysicalPath => throw new NotSupportedException(); + string IFileInfo.Name => throw new NotSupportedException(); + bool IFileInfo.IsDirectory => throw new NotSupportedException(); + Stream IFileInfo.CreateReadStream() => throw new NotSupportedException(); + } } } From e6ccd626bc5a0d95a30115d715996e3fd11f2789 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 11 Aug 2023 12:43:02 -0700 Subject: [PATCH 15/17] Clarify use of delays in KestrelConfigurationLoaderTests.CertificateChangedOnDisk --- .../test/KestrelConfigurationLoaderTests.cs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs b/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs index d05144c3ea48..e6d1f9527753 100644 --- a/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs +++ b/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs @@ -841,8 +841,8 @@ public void ConfigureEndpoint_DoesNotThrowWhen_HttpsConfigIsDeclaredInEndpointDe } [Theory] - [InlineData(true)] - [InlineData(false)] + [InlineData(true)] // This might be flaky + [InlineData(false)] // This will be slow (5 seconds) public async Task CertificateChangedOnDisk(bool reloadOnChange) { var certificatePath = GetCertificatePath(); @@ -892,14 +892,15 @@ public async Task CertificateChangedOnDisk(bool reloadOnChange) } File.WriteAllBytes(certificatePath, newCertificateBytes); - try + if (reloadOnChange) { - await fileTcs.Task.TimeoutAfter(TimeSpan.FromSeconds(10)); - Assert.True(reloadOnChange, "Received an unexpected change event"); + await fileTcs.Task.DefaultTimeout(); } - catch (TimeoutException) + else { - Assert.False(reloadOnChange, "Timed out even though responding to file changes was enabled"); + // We can't just check immediately that the callback hasn't fired - we might preempt it + await Task.Delay(Testing.TaskExtensions.DefaultTimeoutDuration); + Assert.False(fileTcs.Task.IsCompleted); } Assert.Equal(1, endpointConfigurationCallCount); From c731b28499332c18333c734648d932f31d9f5532 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 11 Aug 2023 14:27:52 -0700 Subject: [PATCH 16/17] Address PR feedback --- .../Kestrel/Core/src/Internal/CertificatePathWatcher.cs | 2 -- .../Kestrel/Core/src/Internal/ConfigurationReader.cs | 1 + src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs | 2 +- .../Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs | 6 +++--- 4 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcher.cs b/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcher.cs index 38194ca4309c..7cb7234ac0a8 100644 --- a/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcher.cs +++ b/src/Servers/Kestrel/Core/src/Internal/CertificatePathWatcher.cs @@ -307,8 +307,6 @@ void IDisposable.Dispose() { fileMetadata.Dispose(); } - - GC.SuppressFinalize(this); } private sealed class DirectoryWatchMetadata(IFileProvider fileProvider) : IDisposable diff --git a/src/Servers/Kestrel/Core/src/Internal/ConfigurationReader.cs b/src/Servers/Kestrel/Core/src/Internal/ConfigurationReader.cs index 97c8ed55c595..7b27228e3cae 100644 --- a/src/Servers/Kestrel/Core/src/Internal/ConfigurationReader.cs +++ b/src/Servers/Kestrel/Core/src/Internal/ConfigurationReader.cs @@ -401,6 +401,7 @@ internal CertificateConfig() /// /// Vacuously false if this isn't a file cert. + /// Used for change tracking - not actually part of configuring the certificate. /// public bool FileHasChanged { get; internal set; } diff --git a/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs b/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs index 99982a3e98e1..9b71c5127fcd 100644 --- a/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs +++ b/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs @@ -47,7 +47,7 @@ internal KestrelConfigurationLoader( _httpsConfigurationService = httpsConfigurationService; _certificatePathWatcher = certificatePathWatcher; - Debug.Assert(!!reloadOnChange || (certificatePathWatcher is null), "If reloadOnChange is false, then certificatePathWatcher should be null"); + Debug.Assert(reloadOnChange || (certificatePathWatcher is null), "If reloadOnChange is false, then certificatePathWatcher should be null"); } /// diff --git a/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs b/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs index e6d1f9527753..197128b72907 100644 --- a/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs +++ b/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs @@ -841,8 +841,8 @@ public void ConfigureEndpoint_DoesNotThrowWhen_HttpsConfigIsDeclaredInEndpointDe } [Theory] - [InlineData(true)] // This might be flaky - [InlineData(false)] // This will be slow (5 seconds) + [InlineData(true)] // This might be flaky, since it depends on file system events (or polling) + [InlineData(false)] // This will be slow (1 seconds) public async Task CertificateChangedOnDisk(bool reloadOnChange) { var certificatePath = GetCertificatePath(); @@ -899,7 +899,7 @@ public async Task CertificateChangedOnDisk(bool reloadOnChange) else { // We can't just check immediately that the callback hasn't fired - we might preempt it - await Task.Delay(Testing.TaskExtensions.DefaultTimeoutDuration); + await Task.Delay(TimeSpan.FromSeconds(1)); Assert.False(fileTcs.Task.IsCompleted); } From ca372a7dd1dec01ddd9e0c87dfcfccd380917070 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 11 Aug 2023 16:08:50 -0700 Subject: [PATCH 17/17] Preemptively add CertificateChangedOnDisk to the helix retry list --- eng/test-configuration.json | 1 + 1 file changed, 1 insertion(+) diff --git a/eng/test-configuration.json b/eng/test-configuration.json index 1da9b61de6c6..3f5af7a1b766 100644 --- a/eng/test-configuration.json +++ b/eng/test-configuration.json @@ -21,6 +21,7 @@ {"testName": {"contains": "InjectedStartup_DefaultApplicationNameIsEntryAssembly"}}, {"testName": {"contains": "HEADERS_Received_SecondRequest_ConnectProtocolReset"}}, {"testName": {"contains": "ClientUsingOldCallWithNewProtocol"}}, + {"testName": {"contains": "CertificateChangedOnDisk"}}, {"testAssembly": {"contains": "IIS"}}, {"testAssembly": {"contains": "Template"}}, {"failureMessage": {"contains":"(Site is started but no worker process found)"}},