Reduce comments

9 files changed, 28 insertions, 63 deletions

diff --git a/Emby.Server.Implementations/IO/LibraryMonitor.cs b/Emby.Server.Implementations/IO/LibraryMonitor.cs
index bc76c99451..e6a33d9dd3 100644
--- a/Emby.Server.Implementations/IO/LibraryMonitor.cs
+++ b/Emby.Server.Implementations/IO/LibraryMonitor.cs
@@ -368,10 +368,8 @@ namespace Emby.Server.Implementations.IO
                 return;
             }
 
-            // Something changed on disk that the server did not necessarily do itself, so whatever is
-            // cached about that path and its folder is now a guess. This sits above the checks below
-            // because a change we deliberately do not refresh for still has to be read correctly the
-            // next time somebody looks at that folder.
+            // Invalidate before the checks below: a change we deliberately do not refresh for still
+            // has to be read correctly the next time somebody looks at that folder.
             _directoryService.Invalidate(path);
 
             // Ignore certain files, If the parent of an ignored path has a change event, ignore that too
diff --git a/Emby.Server.Implementations/Library/LibraryManager.cs b/Emby.Server.Implementations/Library/LibraryManager.cs
index 92a87d9f50..0ced650587 100644
--- a/Emby.Server.Implementations/Library/LibraryManager.cs
+++ b/Emby.Server.Implementations/Library/LibraryManager.cs
@@ -3723,8 +3723,8 @@ namespace Emby.Server.Implementations.Library
                     }
                 }
 
-                // The directory caches are shared, so the validation below would otherwise resolve
-                // the libraries root from a listing taken before this folder was created.
+                // The validation below would otherwise resolve the libraries root from a listing
+                // taken before this folder was created.
                 _directoryService.Invalidate(virtualFolderPath);
             }
             finally
diff --git a/Jellyfin.Api/Controllers/LibraryStructureController.cs b/Jellyfin.Api/Controllers/LibraryStructureController.cs
index 5cd6dada19..83684f1b91 100644
--- a/Jellyfin.Api/Controllers/LibraryStructureController.cs
+++ b/Jellyfin.Api/Controllers/LibraryStructureController.cs
@@ -189,8 +189,8 @@ public class LibraryStructureController : BaseJellyfinApiController
 
             Directory.Move(currentPath, newPath);
 
-            // The directory caches are shared, so the validation below would otherwise resolve the
-            // libraries root from a listing taken before the folder was moved.
+            // The validation below would otherwise resolve the libraries root from a listing taken
+            // before the folder was moved.
             _directoryService.Invalidate(currentPath);
             _directoryService.Invalidate(newPath);
         }
diff --git a/MediaBrowser.Controller/Providers/DirectoryService.cs b/MediaBrowser.Controller/Providers/DirectoryService.cs
index 336e35e293..613433391b 100644
--- a/MediaBrowser.Controller/Providers/DirectoryService.cs
+++ b/MediaBrowser.Controller/Providers/DirectoryService.cs
@@ -91,8 +91,7 @@ namespace MediaBrowser.Controller.Providers
             {
                 var file = _fileSystem.GetFileSystemInfo(path);
 
-                // Only a hit is remembered. A miss is the one answer that changes on its own, when
-                // the file the path names turns up.
+                // Only cache hits: a missing file can turn up later.
                 if (file?.Exists ?? false)
                 {
                     result = file;
@@ -110,8 +109,7 @@ namespace MediaBrowser.Controller.Providers
         {
             if (clearCache)
             {
-                // Only what is remembered about this directory. Invalidate() also drops the parent,
-                // which a write needs but which is needless churn on a shared cache here.
+                // Not Invalidate(), which would also drop the parent listing for no reason here.
                 Forget(path);
             }
 
@@ -133,9 +131,6 @@ namespace MediaBrowser.Controller.Providers
 
         public void Invalidate(string path)
         {
-            // Everything remembered about the path itself, and the listing of the directory holding
-            // it, since writing a file changes what its directory contains. The caches belong to the
-            // file system rather than to this instance, so this is felt by every reader of it.
             Forget(path);
 
             var parent = Path.GetDirectoryName(path);
@@ -162,9 +157,8 @@ namespace MediaBrowser.Controller.Providers
             private const int DirectoryCacheSize = 2048;
             private const int FileCacheSize = 8192;
 
-            // A DirectoryService no longer bounds how long its answers are trusted by dying, so a
-            // lifetime does. This is a staleness bound, not a snapshot: a long refresh can outlive it
-            // and re-read a directory partway through.
+            // The cache outlives the DirectoryService instances reading it, so entries need their
+            // own staleness bound. A long refresh can outlive it and re-read a directory partway.
             private static readonly TimeSpan _entryLifetime = TimeSpan.FromMinutes(1);
 
             public ConcurrentTLru<string, FileSystemMetadata[]> Entries { get; }
diff --git a/MediaBrowser.Providers/Lyric/LyricManager.cs b/MediaBrowser.Providers/Lyric/LyricManager.cs
index bff076b27b..57d98af1da 100644
--- a/MediaBrowser.Providers/Lyric/LyricManager.cs
+++ b/MediaBrowser.Providers/Lyric/LyricManager.cs
@@ -255,8 +255,7 @@ public class LyricManager : ILyricManager
                 _libraryMonitor.ReportFileSystemChangeComplete(path, false);
             }
 
-            // The refresh below reads the containing folder to find external lyrics, and would find
-            // the deleted one again in a cached listing.
+            // The refresh below would otherwise find the deleted file in a cached listing.
             _directoryService.Invalidate(path);
         }
 
@@ -454,9 +453,7 @@ public class LyricManager : ILyricManager
                     await stream.CopyToAsync(fs).ConfigureAwait(false);
                 }
 
-                // The directory caches are shared and outlive this call, so the refresh that follows
-                // would otherwise resolve external lyrics from a listing taken before this file
-                // landed.
+                // The refresh that follows would otherwise not see the new file.
                 _directoryService.Invalidate(savePath);
 
                 return;
diff --git a/MediaBrowser.Providers/Manager/ProviderManager.cs b/MediaBrowser.Providers/Manager/ProviderManager.cs
index 4ff9ab1a52..e7b15305b3 100644
--- a/MediaBrowser.Providers/Manager/ProviderManager.cs
+++ b/MediaBrowser.Providers/Manager/ProviderManager.cs
@@ -1143,8 +1143,8 @@ namespace MediaBrowser.Providers.Manager
                 return;
             }
 
-            // PriorityQueue is not thread safe, and this runs on whichever thread queued the refresh
-            // while the processor dequeues on its own, so every touch of the queue takes the lock.
+            // PriorityQueue is not thread safe and the processor dequeues concurrently, so every
+            // touch of the queue takes the lock.
             lock (_refreshQueueLock)
             {
                 _refreshQueue.Enqueue((itemId, options), priority);
@@ -1182,10 +1182,8 @@ namespace MediaBrowser.Providers.Manager
             {
                 (Guid ItemId, MetadataRefreshOptions RefreshOptions) refreshItem;
 
-                // Standing down and taking the next entry happen under one lock, and this is the
-                // only place the flag is handed back. Releasing it anywhere else would leave a gap
-                // in which a refresh queued just after the queue ran dry sees a processor that has
-                // already stopped, and waits forever.
+                // Dequeueing and standing down happen under one lock, otherwise a refresh queued
+                // just after the queue ran dry would see a processor that has already stopped.
                 lock (_refreshQueueLock)
                 {
                     if (_disposed
@@ -1213,14 +1211,13 @@ namespace MediaBrowser.Providers.Manager
                 }
                 catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested)
                 {
-                    // Shutting down. Whatever is still queued keeps its place; the next pass round
-                    // stands the processor down, so the next refresh queued starts one of its own.
+                    // Shutting down: the next pass sees the token and stands the processor down.
                     continue;
                 }
                 catch (Exception ex)
                 {
-                    // A provider that cancelled for its own reasons lands here too, an HTTP timeout
-                    // above all. One unreachable metadata server must not stop the queue draining.
+                    // Includes a provider that cancelled for its own reasons, such as an HTTP
+                    // timeout, which must not stop the queue draining.
                     _logger.LogError(ex, "Error refreshing item");
                 }
             }
diff --git a/MediaBrowser.Providers/Subtitles/SubtitleManager.cs b/MediaBrowser.Providers/Subtitles/SubtitleManager.cs
index 4a2f58d457..37d261a95e 100644
--- a/MediaBrowser.Providers/Subtitles/SubtitleManager.cs
+++ b/MediaBrowser.Providers/Subtitles/SubtitleManager.cs
@@ -284,9 +284,7 @@ namespace MediaBrowser.Providers.Subtitles
                             await stream.CopyToAsync(fs).ConfigureAwait(false);
                         }
 
-                        // The directory caches are shared and outlive this call, so the refresh that
-                        // follows would otherwise resolve external subtitles from a listing taken
-                        // before this file landed.
+                        // The refresh that follows would otherwise not see the new file.
                         _directoryService.Invalidate(path);
 
                         return;
@@ -403,8 +401,7 @@ namespace MediaBrowser.Providers.Subtitles
                 _monitor.ReportFileSystemChangeComplete(path, false);
             }
 
-            // The refresh below reads the containing folder to find external subtitles, and would
-            // find the deleted one again in a cached listing.
+            // The refresh below would otherwise find the deleted file in a cached listing.
             _directoryService.Invalidate(path);
 
             return item.RefreshMetadata(CancellationToken.None);
diff --git a/tests/Jellyfin.Controller.Tests/DirectoryServiceTests.cs b/tests/Jellyfin.Controller.Tests/DirectoryServiceTests.cs
index a550828783..338ee9c903 100644
--- a/tests/Jellyfin.Controller.Tests/DirectoryServiceTests.cs
+++ b/tests/Jellyfin.Controller.Tests/DirectoryServiceTests.cs
@@ -268,9 +268,7 @@ namespace Jellyfin.Controller.Tests
         [Fact]
         public void GetFileSystemEntries_FarMorePathsThanTheCacheHolds_EvictsInsteadOfGrowing()
         {
-            // The cache outlives every DirectoryService that reads it, so it has to give entries back
-            // rather than hold every path the server ever saw. Asking for more paths than it can hold
-            // must push the first one out, which shows up as the file system being read for it twice.
+            // Eviction of the first path shows up as the file system being read for it twice.
             const int PathCount = 8192;
 
             var fileSystemMock = new Mock<IFileSystem>();
@@ -295,8 +293,6 @@ namespace Jellyfin.Controller.Tests
         [Fact]
         public void GetFileSystemEntries_SecondServiceOverSameFileSystem_ReusesTheFirstAnswer()
         {
-            // The library code news up a DirectoryService per item, so what one of them learned about
-            // a directory has to be worth something to the next one reading the same file system.
             var fileSystemMock = new Mock<IFileSystem>();
             fileSystemMock.Setup(f => f.GetFileSystemEntries(LowerCasePath))
                 .Returns(_lowerCaseFileSystemMetadata);
@@ -328,8 +324,6 @@ namespace Jellyfin.Controller.Tests
         [Fact]
         public void Invalidate_GivenADirectory_DropsBothTheListingAndTheFilePaths()
         {
-            // Clearing only one of the two views of a directory leaves the other one answering from
-            // before whatever was just written there.
             var fileSystemMock = new Mock<IFileSystem>();
             fileSystemMock.SetupSequence(f => f.GetFileSystemEntries(LowerCasePath))
                 .Returns(_lowerCaseFileSystemMetadata)
@@ -351,7 +345,6 @@ namespace Jellyfin.Controller.Tests
         [Fact]
         public void Invalidate_GivenAFile_DropsTheListingOfTheDirectoryHoldingIt()
         {
-            // Downloading a subtitle changes what its folder contains, not just the one path.
             const string NewFile = LowerCasePath + "/Song 2.srt";
 
             var fileSystemMock = new Mock<IFileSystem>();
@@ -370,8 +363,6 @@ namespace Jellyfin.Controller.Tests
         [Fact]
         public void Invalidate_OnOneService_IsSeenByAnotherOverTheSameFileSystem()
         {
-            // Whoever writes the file and whoever refreshes the item hold different services, so
-            // invalidating has to reach the cache both of them read.
             var fileSystemMock = new Mock<IFileSystem>();
             fileSystemMock.SetupSequence(f => f.GetFileSystemEntries(LowerCasePath))
                 .Returns(_lowerCaseFileSystemMetadata)
@@ -388,8 +379,6 @@ namespace Jellyfin.Controller.Tests
         [Fact]
         public void GetFilePaths_ClearingTheCache_KeepsTheParentDirectory()
         {
-            // Re-reading one directory is not a reason to make the server list the library folder
-            // holding it again, which the shared cache would otherwise have to do.
             const string ParentPath = "/music";
 
             var fileSystemMock = new Mock<IFileSystem>();
@@ -421,7 +410,6 @@ namespace Jellyfin.Controller.Tests
 
             Assert.Null(directoryService.GetFileSystemEntry(MissingPath));
 
-            // The one answer that changes on its own: the file turning up has to be visible.
             Assert.NotNull(directoryService.GetFileSystemEntry(MissingPath));
         }
     }
diff --git a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs
index b3f5af76b7..248b236df8 100644
--- a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs
+++ b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs
@@ -381,10 +381,6 @@ namespace Jellyfin.Providers.Tests.Manager
         [Fact]
         public async Task QueueRefresh_ManyItemsQueuedFromManyThreads_ProcessesEveryOne()
         {
-            // The queue is filled from whichever thread wants a refresh and drained by a processor of
-            // its own, so an unsynchronised PriorityQueue can lose entries outright, and a processor
-            // that stands down before releasing its flag leaves whatever was queued in that gap with
-            // nobody to drain it. Either way an item silently never gets refreshed.
             const int ItemCount = 2000;
 
             var queued = Enumerable.Range(0, ItemCount).Select(_ => Guid.NewGuid()).ToArray();
@@ -395,7 +391,7 @@ namespace Jellyfin.Providers.Tests.Manager
             libraryManager.Setup(i => i.GetItemById(It.IsAny<Guid>()))
                 .Returns((Guid id) =>
                 {
-                    // Returning null drains the entry without needing the whole refresh machinery.
+                    // Returning null drains the entry without the whole refresh machinery.
                     processed.Add(id);
                     if (processed.Count == ItemCount)
                     {
@@ -425,7 +421,7 @@ namespace Jellyfin.Providers.Tests.Manager
             }
             catch (OperationCanceledException)
             {
-                // Fall through, so the assertions below name what was lost rather than the wait.
+                // Fall through so the assertions report what was lost.
             }
 
             Assert.Empty(providerManager.GetRefreshQueue());
@@ -435,9 +431,8 @@ namespace Jellyfin.Providers.Tests.Manager
         [Fact]
         public async Task QueueRefresh_RefreshCancelsForItsOwnReasons_KeepsDrainingTheQueue()
         {
-            // MetadataService rethrows OperationCanceledException out of a provider, so an HTTP
-            // timeout against an unreachable metadata server arrives here looking exactly like a
-            // shutdown. Treating it as one stops the processor and strands the rest of the queue.
+            // A provider timeout arrives as an OperationCanceledException, indistinguishable from
+            // a shutdown; treating it as one would strand the rest of the queue.
             const int ItemCount = 200;
 
             var queued = Enumerable.Range(0, ItemCount).Select(_ => Guid.NewGuid()).ToArray();
@@ -454,8 +449,7 @@ namespace Jellyfin.Providers.Tests.Manager
                     {
                         cancelledOnce = true;
 
-                        // Hold the first entry until the whole batch is queued, so everything that
-                        // follows it is already waiting when the cancellation lands.
+                        // Hold the first entry until the whole batch is queued.
                         allQueued.Wait(TimeSpan.FromSeconds(30));
                         throw new OperationCanceledException("provider timed out");
                     }
@@ -487,7 +481,7 @@ namespace Jellyfin.Providers.Tests.Manager
             }
             catch (OperationCanceledException)
             {
-                // Fall through, so the assertions below name what was left stranded.
+                // Fall through so the assertions report what was stranded.
             }
 
             Assert.Empty(providerManager.GetRefreshQueue());