From d73e3d964e3ce8197ec86bdaad447072305bc0d6 Mon Sep 17 00:00:00 2001 From: Shadowghost Date: Wed, 2 Sep 2026 07:06:08 +0200 Subject: Optimize Caches Co-Authored-By: Cody Robibero --- Emby.Server.Implementations/IO/LibraryMonitor.cs | 11 +++++++++++ Emby.Server.Implementations/Library/LibraryManager.cs | 9 +++++++++ 2 files changed, 20 insertions(+) (limited to 'Emby.Server.Implementations') diff --git a/Emby.Server.Implementations/IO/LibraryMonitor.cs b/Emby.Server.Implementations/IO/LibraryMonitor.cs index 1bf0f8c76c..bc76c99451 100644 --- a/Emby.Server.Implementations/IO/LibraryMonitor.cs +++ b/Emby.Server.Implementations/IO/LibraryMonitor.cs @@ -8,6 +8,7 @@ using Emby.Server.Implementations.Library; using MediaBrowser.Controller.Configuration; using MediaBrowser.Controller.Entities; using MediaBrowser.Controller.Library; +using MediaBrowser.Controller.Providers; using MediaBrowser.Model.IO; using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Logging; @@ -21,6 +22,7 @@ namespace Emby.Server.Implementations.IO private readonly ILibraryManager _libraryManager; private readonly IServerConfigurationManager _configurationManager; private readonly IFileSystem _fileSystem; + private readonly IDirectoryService _directoryService; private readonly DotIgnoreIgnoreRule _dotIgnoreIgnoreRule; /// @@ -47,6 +49,7 @@ namespace Emby.Server.Implementations.IO /// The library manager. /// The configuration manager. /// The filesystem. + /// The directory service. /// The . /// The .ignore rule handler. public LibraryMonitor( @@ -54,6 +57,7 @@ namespace Emby.Server.Implementations.IO ILibraryManager libraryManager, IServerConfigurationManager configurationManager, IFileSystem fileSystem, + IDirectoryService directoryService, IHostApplicationLifetime appLifetime, DotIgnoreIgnoreRule dotIgnoreIgnoreRule) { @@ -61,6 +65,7 @@ namespace Emby.Server.Implementations.IO _logger = logger; _configurationManager = configurationManager; _fileSystem = fileSystem; + _directoryService = directoryService; _dotIgnoreIgnoreRule = dotIgnoreIgnoreRule; appLifetime.ApplicationStarted.Register(Start); @@ -363,6 +368,12 @@ 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. + _directoryService.Invalidate(path); + // Ignore certain files, If the parent of an ignored path has a change event, ignore that too foreach (var i in _tempIgnoredPaths.Keys) { diff --git a/Emby.Server.Implementations/Library/LibraryManager.cs b/Emby.Server.Implementations/Library/LibraryManager.cs index 3db8265f6e..92a87d9f50 100644 --- a/Emby.Server.Implementations/Library/LibraryManager.cs +++ b/Emby.Server.Implementations/Library/LibraryManager.cs @@ -88,6 +88,7 @@ namespace Emby.Server.Implementations.Library private readonly ExtraResolver _extraResolver; private readonly IPathManager _pathManager; private readonly ILocalizationManager _localization; + private readonly IDirectoryService _directoryService; private readonly FastConcurrentLru _cache; private readonly DotIgnoreIgnoreRule _dotIgnoreIgnoreRule; private readonly IMediaStreamRepository _mediaStreamRepository; @@ -189,6 +190,7 @@ namespace Emby.Server.Implementations.Library _pathManager = pathManager; _dotIgnoreIgnoreRule = dotIgnoreIgnoreRule; _localization = localization; + _directoryService = directoryService; _extraResolver = new ExtraResolver(loggerFactory.CreateLogger(), namingOptions, directoryService); _configurationManager.ConfigurationUpdated += ConfigurationUpdated; @@ -3720,6 +3722,10 @@ namespace Emby.Server.Implementations.Library AddMediaPathInternal(name, path, false); } } + + // The directory caches are shared, so the validation below would otherwise resolve + // the libraries root from a listing taken before this folder was created. + _directoryService.Invalidate(virtualFolderPath); } finally { @@ -3920,6 +3926,7 @@ namespace Emby.Server.Implementations.Library try { Directory.Delete(path, true); + _directoryService.Invalidate(path); } finally { @@ -3989,6 +3996,7 @@ namespace Emby.Server.Implementations.Library if (!string.IsNullOrEmpty(shortcut)) { _fileSystem.DeleteFile(shortcut); + _directoryService.Invalidate(shortcut); } var libraryOptions = CollectionFolder.GetLibraryOptions(virtualFolderPath); @@ -4032,6 +4040,7 @@ namespace Emby.Server.Implementations.Library } _fileSystem.CreateShortcut(lnk, _appHost.ReverseVirtualPath(path)); + _directoryService.Invalidate(lnk); RemoveContentTypeOverrides(path); } -- cgit v1.2.3 From b724e57458e60e967bf05e943c5cf6b1a41f4044 Mon Sep 17 00:00:00 2001 From: Shadowghost Date: Thu, 3 Sep 2026 11:51:02 +0200 Subject: Reduce comments --- Emby.Server.Implementations/IO/LibraryMonitor.cs | 6 ++---- Emby.Server.Implementations/Library/LibraryManager.cs | 4 ++-- Jellyfin.Api/Controllers/LibraryStructureController.cs | 4 ++-- MediaBrowser.Controller/Providers/DirectoryService.cs | 14 ++++---------- MediaBrowser.Providers/Lyric/LyricManager.cs | 7 ++----- MediaBrowser.Providers/Manager/ProviderManager.cs | 17 +++++++---------- MediaBrowser.Providers/Subtitles/SubtitleManager.cs | 7 ++----- .../Jellyfin.Controller.Tests/DirectoryServiceTests.cs | 14 +------------- .../Manager/ProviderManagerTests.cs | 18 ++++++------------ 9 files changed, 28 insertions(+), 63 deletions(-) (limited to 'Emby.Server.Implementations') 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 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(); @@ -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(); 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(); 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(); @@ -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(); 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(); @@ -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())) .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()); -- cgit v1.2.3 From 46dd7d8e99ff7167d2d175878896294ed823927f Mon Sep 17 00:00:00 2001 From: Shadowghost Date: Fri, 4 Sep 2026 19:28:59 +0200 Subject: Invalidate the singleton DirectoryService cache on filesystem changes --- Emby.Server.Implementations/IO/LibraryMonitor.cs | 4 +- .../Library/LibraryManager.cs | 3 +- .../Controllers/LibraryStructureController.cs | 3 +- .../MediaBrowser.Controller.csproj | 1 - .../Providers/DirectoryService.cs | 52 +++++---------- MediaBrowser.Providers/Lyric/LyricManager.cs | 4 +- .../Subtitles/SubtitleManager.cs | 4 +- .../DirectoryServiceTests.cs | 73 ---------------------- 8 files changed, 25 insertions(+), 119 deletions(-) (limited to 'Emby.Server.Implementations') diff --git a/Emby.Server.Implementations/IO/LibraryMonitor.cs b/Emby.Server.Implementations/IO/LibraryMonitor.cs index e6a33d9dd3..d5735aed27 100644 --- a/Emby.Server.Implementations/IO/LibraryMonitor.cs +++ b/Emby.Server.Implementations/IO/LibraryMonitor.cs @@ -368,8 +368,8 @@ namespace Emby.Server.Implementations.IO return; } - // 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. + // The injected service is a singleton, so drop the path before the checks below: + // a change we deliberately do not refresh for still has to read correctly later. _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 0ced650587..80e89b4305 100644 --- a/Emby.Server.Implementations/Library/LibraryManager.cs +++ b/Emby.Server.Implementations/Library/LibraryManager.cs @@ -3723,8 +3723,7 @@ namespace Emby.Server.Implementations.Library } } - // The validation below would otherwise resolve the libraries root from a listing - // taken before this folder was created. + // The injected service is a singleton, so its listing predates this folder. _directoryService.Invalidate(virtualFolderPath); } finally diff --git a/Jellyfin.Api/Controllers/LibraryStructureController.cs b/Jellyfin.Api/Controllers/LibraryStructureController.cs index 83684f1b91..e4833c77dd 100644 --- a/Jellyfin.Api/Controllers/LibraryStructureController.cs +++ b/Jellyfin.Api/Controllers/LibraryStructureController.cs @@ -189,8 +189,7 @@ public class LibraryStructureController : BaseJellyfinApiController Directory.Move(currentPath, newPath); - // The validation below would otherwise resolve the libraries root from a listing taken - // before the folder was moved. + // The injected service is a singleton, so its listings of both paths are now stale. _directoryService.Invalidate(currentPath); _directoryService.Invalidate(newPath); } diff --git a/MediaBrowser.Controller/MediaBrowser.Controller.csproj b/MediaBrowser.Controller/MediaBrowser.Controller.csproj index 06188ad511..73cdf18e91 100644 --- a/MediaBrowser.Controller/MediaBrowser.Controller.csproj +++ b/MediaBrowser.Controller/MediaBrowser.Controller.csproj @@ -18,7 +18,6 @@ - diff --git a/MediaBrowser.Controller/Providers/DirectoryService.cs b/MediaBrowser.Controller/Providers/DirectoryService.cs index 613433391b..684e247f86 100644 --- a/MediaBrowser.Controller/Providers/DirectoryService.cs +++ b/MediaBrowser.Controller/Providers/DirectoryService.cs @@ -1,31 +1,33 @@ #pragma warning disable CS1591 using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.IO; using System.Linq; -using System.Runtime.CompilerServices; -using BitFaster.Caching.Lru; using MediaBrowser.Model.IO; namespace MediaBrowser.Controller.Providers { public class DirectoryService : IDirectoryService { - private static readonly ConditionalWeakTable _caches = []; + // TODO make static and switch to FastConcurrentLru. + private readonly ConcurrentDictionary _cache = new(StringComparer.Ordinal); + + private readonly ConcurrentDictionary _fileCache = new(StringComparer.Ordinal); + + private readonly ConcurrentDictionary> _filePathCache = new(StringComparer.Ordinal); private readonly IFileSystem _fileSystem; - private readonly DirectoryCache _cache; public DirectoryService(IFileSystem fileSystem) { _fileSystem = fileSystem; - _cache = _caches.GetValue(fileSystem, static _ => new DirectoryCache()); } public FileSystemMetadata[] GetFileSystemEntries(string path) { - return _cache.Entries.GetOrAdd( + return _cache.GetOrAdd( path, static (p, fileSystem) => { @@ -87,15 +89,13 @@ namespace MediaBrowser.Controller.Providers public FileSystemMetadata? GetFileSystemEntry(string path) { - if (!_cache.Files.TryGet(path, out var result)) + if (!_fileCache.TryGetValue(path, out var result)) { var file = _fileSystem.GetFileSystemInfo(path); - - // Only cache hits: a missing file can turn up later. if (file?.Exists ?? false) { result = file; - _cache.Files.AddOrUpdate(path, result); + _fileCache.TryAdd(path, result); } } @@ -109,11 +109,10 @@ namespace MediaBrowser.Controller.Providers { if (clearCache) { - // Not Invalidate(), which would also drop the parent listing for no reason here. - Forget(path); + _filePathCache.TryRemove(path, out _); } - return _cache.FilePaths.GetOrAdd( + var filePaths = _filePathCache.GetOrAdd( path, static (p, fileSystem) => { @@ -127,6 +126,8 @@ namespace MediaBrowser.Controller.Providers } }, _fileSystem); + + return filePaths; } public void Invalidate(string path) @@ -147,28 +148,9 @@ namespace MediaBrowser.Controller.Providers private void Forget(string path) { - _cache.Entries.TryRemove(path, out _); - _cache.Files.TryRemove(path, out _); - _cache.FilePaths.TryRemove(path, out _); - } - - private sealed class DirectoryCache - { - private const int DirectoryCacheSize = 2048; - private const int FileCacheSize = 8192; - - // 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 Entries { get; } - = new(Environment.ProcessorCount, DirectoryCacheSize, StringComparer.Ordinal, _entryLifetime); - - public ConcurrentTLru Files { get; } - = new(Environment.ProcessorCount, FileCacheSize, StringComparer.Ordinal, _entryLifetime); - - public ConcurrentTLru> FilePaths { get; } - = new(Environment.ProcessorCount, DirectoryCacheSize, StringComparer.Ordinal, _entryLifetime); + _cache.TryRemove(path, out _); + _fileCache.TryRemove(path, out _); + _filePathCache.TryRemove(path, out _); } } } diff --git a/MediaBrowser.Providers/Lyric/LyricManager.cs b/MediaBrowser.Providers/Lyric/LyricManager.cs index 57d98af1da..dfa7bfde2f 100644 --- a/MediaBrowser.Providers/Lyric/LyricManager.cs +++ b/MediaBrowser.Providers/Lyric/LyricManager.cs @@ -255,7 +255,7 @@ public class LyricManager : ILyricManager _libraryMonitor.ReportFileSystemChangeComplete(path, false); } - // The refresh below would otherwise find the deleted file in a cached listing. + // The injected service is a singleton, so its listing would keep the deleted file. _directoryService.Invalidate(path); } @@ -453,7 +453,7 @@ public class LyricManager : ILyricManager await stream.CopyToAsync(fs).ConfigureAwait(false); } - // The refresh that follows would otherwise not see the new file. + // The injected service is a singleton, so its listing of the folder is now stale. _directoryService.Invalidate(savePath); return; diff --git a/MediaBrowser.Providers/Subtitles/SubtitleManager.cs b/MediaBrowser.Providers/Subtitles/SubtitleManager.cs index 37d261a95e..aa363c425f 100644 --- a/MediaBrowser.Providers/Subtitles/SubtitleManager.cs +++ b/MediaBrowser.Providers/Subtitles/SubtitleManager.cs @@ -284,7 +284,7 @@ namespace MediaBrowser.Providers.Subtitles await stream.CopyToAsync(fs).ConfigureAwait(false); } - // The refresh that follows would otherwise not see the new file. + // The injected service is a singleton, so its listing of the folder is now stale. _directoryService.Invalidate(path); return; @@ -401,7 +401,7 @@ namespace MediaBrowser.Providers.Subtitles _monitor.ReportFileSystemChangeComplete(path, false); } - // The refresh below would otherwise find the deleted file in a cached listing. + // The injected service is a singleton, so its listing would keep the deleted file. _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 14ef604d34..7c275b78cc 100644 --- a/tests/Jellyfin.Controller.Tests/DirectoryServiceTests.cs +++ b/tests/Jellyfin.Controller.Tests/DirectoryServiceTests.cs @@ -1,4 +1,3 @@ -using System.Globalization; using System.IO; using System.Linq; using MediaBrowser.Controller.Providers; @@ -268,62 +267,6 @@ namespace Jellyfin.Controller.Tests fileSystemMock.Verify(f => f.GetFileSystemEntries(_lowerCasePath), Times.Once); } - [Fact] - public void GetFileSystemEntries_FarMorePathsThanTheCacheHolds_EvictsInsteadOfGrowing() - { - // Eviction of the first path shows up as the file system being read for it twice. - const int PathCount = 8192; - - var fileSystemMock = new Mock(); - fileSystemMock.Setup(f => f.GetFileSystemEntries(It.IsAny())) - .Returns(_lowerCaseFileSystemMetadata); - - var directoryService = new DirectoryService(fileSystemMock.Object); - - const string FirstPath = "/music/artist0"; - directoryService.GetFileSystemEntries(FirstPath); - - for (var i = 1; i < PathCount; i++) - { - directoryService.GetFileSystemEntries("/music/artist" + i.ToString(CultureInfo.InvariantCulture)); - } - - directoryService.GetFileSystemEntries(FirstPath); - - fileSystemMock.Verify(f => f.GetFileSystemEntries(FirstPath), Times.Exactly(2)); - } - - [Fact] - public void GetFileSystemEntries_SecondServiceOverSameFileSystem_ReusesTheFirstAnswer() - { - var fileSystemMock = new Mock(); - fileSystemMock.Setup(f => f.GetFileSystemEntries(_lowerCasePath)) - .Returns(_lowerCaseFileSystemMetadata); - - new DirectoryService(fileSystemMock.Object).GetFileSystemEntries(_lowerCasePath); - var result = new DirectoryService(fileSystemMock.Object).GetFileSystemEntries(_lowerCasePath); - - Assert.Equal(_lowerCaseFileSystemMetadata, result); - fileSystemMock.Verify(f => f.GetFileSystemEntries(_lowerCasePath), Times.Once); - } - - [Fact] - public void GetFileSystemEntries_SeparateFileSystems_DoNotShareAnswers() - { - var firstFileSystem = new Mock(); - firstFileSystem.Setup(f => f.GetFileSystemEntries(_lowerCasePath)) - .Returns(_lowerCaseFileSystemMetadata); - var secondFileSystem = new Mock(); - secondFileSystem.Setup(f => f.GetFileSystemEntries(_lowerCasePath)) - .Returns(_upperCaseFileSystemMetadata); - - var firstResult = new DirectoryService(firstFileSystem.Object).GetFileSystemEntries(_lowerCasePath); - var secondResult = new DirectoryService(secondFileSystem.Object).GetFileSystemEntries(_lowerCasePath); - - Assert.Equal(_lowerCaseFileSystemMetadata, firstResult); - Assert.Equal(_upperCaseFileSystemMetadata, secondResult); - } - [Fact] public void Invalidate_GivenADirectory_DropsBothTheListingAndTheFilePaths() { @@ -363,22 +306,6 @@ namespace Jellyfin.Controller.Tests Assert.Equal(_upperCaseFileSystemMetadata, directoryService.GetFileSystemEntries(_lowerCasePath)); } - [Fact] - public void Invalidate_OnOneService_IsSeenByAnotherOverTheSameFileSystem() - { - var fileSystemMock = new Mock(); - fileSystemMock.SetupSequence(f => f.GetFileSystemEntries(_lowerCasePath)) - .Returns(_lowerCaseFileSystemMetadata) - .Returns(_upperCaseFileSystemMetadata); - - new DirectoryService(fileSystemMock.Object).GetFileSystemEntries(_lowerCasePath); - new DirectoryService(fileSystemMock.Object).Invalidate(Path.Combine(_lowerCasePath, "Song 2.srt")); - - var result = new DirectoryService(fileSystemMock.Object).GetFileSystemEntries(_lowerCasePath); - - Assert.Equal(_upperCaseFileSystemMetadata, result); - } - [Fact] public void GetFilePaths_ClearingTheCache_KeepsTheParentDirectory() { -- cgit v1.2.3 From 344a6dcd2c39c6a0a1f34b888f260e297781a2c5 Mon Sep 17 00:00:00 2001 From: Shadowghost Date: Sun, 6 Sep 2026 08:38:43 +0200 Subject: Apply review suggestions --- Emby.Server.Implementations/IO/LibraryMonitor.cs | 2 -- Emby.Server.Implementations/Library/LibraryManager.cs | 1 - Jellyfin.Api/Controllers/LibraryStructureController.cs | 8 ++------ MediaBrowser.Controller/Providers/DirectoryService.cs | 18 +++++++++++++----- MediaBrowser.Controller/Providers/IDirectoryService.cs | 7 +++++++ MediaBrowser.Providers/Lyric/LyricManager.cs | 2 -- MediaBrowser.Providers/Subtitles/SubtitleManager.cs | 2 -- 7 files changed, 22 insertions(+), 18 deletions(-) (limited to 'Emby.Server.Implementations') diff --git a/Emby.Server.Implementations/IO/LibraryMonitor.cs b/Emby.Server.Implementations/IO/LibraryMonitor.cs index d5735aed27..0f92e2f03e 100644 --- a/Emby.Server.Implementations/IO/LibraryMonitor.cs +++ b/Emby.Server.Implementations/IO/LibraryMonitor.cs @@ -368,8 +368,6 @@ namespace Emby.Server.Implementations.IO return; } - // The injected service is a singleton, so drop the path before the checks below: - // a change we deliberately do not refresh for still has to read correctly later. _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 80e89b4305..dc76e1183e 100644 --- a/Emby.Server.Implementations/Library/LibraryManager.cs +++ b/Emby.Server.Implementations/Library/LibraryManager.cs @@ -3723,7 +3723,6 @@ namespace Emby.Server.Implementations.Library } } - // The injected service is a singleton, so its listing predates this folder. _directoryService.Invalidate(virtualFolderPath); } finally diff --git a/Jellyfin.Api/Controllers/LibraryStructureController.cs b/Jellyfin.Api/Controllers/LibraryStructureController.cs index e4833c77dd..65bfe25d21 100644 --- a/Jellyfin.Api/Controllers/LibraryStructureController.cs +++ b/Jellyfin.Api/Controllers/LibraryStructureController.cs @@ -183,15 +183,11 @@ public class LibraryStructureController : BaseJellyfinApiController var tempPath = Path.Combine( rootFolderPath, Guid.NewGuid().ToString("N", CultureInfo.InvariantCulture)); - Directory.Move(currentPath, tempPath); + _directoryService.Move(currentPath, tempPath); currentPath = tempPath; } - Directory.Move(currentPath, newPath); - - // The injected service is a singleton, so its listings of both paths are now stale. - _directoryService.Invalidate(currentPath); - _directoryService.Invalidate(newPath); + _directoryService.Move(currentPath, newPath); } finally { diff --git a/MediaBrowser.Controller/Providers/DirectoryService.cs b/MediaBrowser.Controller/Providers/DirectoryService.cs index 38872d3cbe..f8e0bf4ed9 100644 --- a/MediaBrowser.Controller/Providers/DirectoryService.cs +++ b/MediaBrowser.Controller/Providers/DirectoryService.cs @@ -170,6 +170,14 @@ namespace MediaBrowser.Controller.Providers } } + public void Move(string source, string destination) + { + Directory.Move(source, destination); + + Invalidate(source); + Invalidate(destination); + } + public bool IsAccessible(string path) { return _fileSystem.GetFileSystemEntryPaths(path).Any(); @@ -178,21 +186,21 @@ namespace MediaBrowser.Controller.Providers private void DropCacheIfIdleOrFull() { var nowMs = Environment.TickCount64; - var idleMs = nowMs - Volatile.Read(ref _lastAccess); + var idleMs = nowMs - _lastAccess; - if (idleMs >= IdleTimeoutMs || Volatile.Read(ref _recordCount) >= MaxCachedRecords) + if (idleMs >= IdleTimeoutMs || _recordCount >= MaxCachedRecords) { _cache.Clear(); _fileCache.Clear(); _filePathCache.Clear(); - Volatile.Write(ref _recordCount, 0); - Volatile.Write(ref _lastAccess, nowMs); + _recordCount = 0; + _lastAccess = nowMs; return; } if (idleMs >= AccessIntervalMs) { - Volatile.Write(ref _lastAccess, nowMs); + _lastAccess = nowMs; } } diff --git a/MediaBrowser.Controller/Providers/IDirectoryService.cs b/MediaBrowser.Controller/Providers/IDirectoryService.cs index 609d094254..3a943d5f0c 100644 --- a/MediaBrowser.Controller/Providers/IDirectoryService.cs +++ b/MediaBrowser.Controller/Providers/IDirectoryService.cs @@ -29,6 +29,13 @@ namespace MediaBrowser.Controller.Providers /// The file or directory path that changed. void Invalidate(string path); + /// + /// Moves a directory and forgets what is cached about both paths. + /// + /// The directory to move. + /// The path to move the directory to. + void Move(string source, string destination); + bool IsAccessible(string path); } } diff --git a/MediaBrowser.Providers/Lyric/LyricManager.cs b/MediaBrowser.Providers/Lyric/LyricManager.cs index dfa7bfde2f..a19262c3a7 100644 --- a/MediaBrowser.Providers/Lyric/LyricManager.cs +++ b/MediaBrowser.Providers/Lyric/LyricManager.cs @@ -255,7 +255,6 @@ public class LyricManager : ILyricManager _libraryMonitor.ReportFileSystemChangeComplete(path, false); } - // The injected service is a singleton, so its listing would keep the deleted file. _directoryService.Invalidate(path); } @@ -453,7 +452,6 @@ public class LyricManager : ILyricManager await stream.CopyToAsync(fs).ConfigureAwait(false); } - // The injected service is a singleton, so its listing of the folder is now stale. _directoryService.Invalidate(savePath); return; diff --git a/MediaBrowser.Providers/Subtitles/SubtitleManager.cs b/MediaBrowser.Providers/Subtitles/SubtitleManager.cs index aa363c425f..cd9dda21a0 100644 --- a/MediaBrowser.Providers/Subtitles/SubtitleManager.cs +++ b/MediaBrowser.Providers/Subtitles/SubtitleManager.cs @@ -284,7 +284,6 @@ namespace MediaBrowser.Providers.Subtitles await stream.CopyToAsync(fs).ConfigureAwait(false); } - // The injected service is a singleton, so its listing of the folder is now stale. _directoryService.Invalidate(path); return; @@ -401,7 +400,6 @@ namespace MediaBrowser.Providers.Subtitles _monitor.ReportFileSystemChangeComplete(path, false); } - // The injected service is a singleton, so its listing would keep the deleted file. _directoryService.Invalidate(path); return item.RefreshMetadata(CancellationToken.None); -- cgit v1.2.3 From 7b638a22bb049196376170306d2f984826e9663a Mon Sep 17 00:00:00 2001 From: Shadowghost Date: Sun, 6 Sep 2026 09:07:22 +0200 Subject: Add test --- .../Library/LibraryManager.cs | 2 ++ .../Controllers/LibraryStructureControllerTests.cs | 42 ++++++++++++++++++++++ 2 files changed, 44 insertions(+) (limited to 'Emby.Server.Implementations') diff --git a/Emby.Server.Implementations/Library/LibraryManager.cs b/Emby.Server.Implementations/Library/LibraryManager.cs index dc76e1183e..b698ceb420 100644 --- a/Emby.Server.Implementations/Library/LibraryManager.cs +++ b/Emby.Server.Implementations/Library/LibraryManager.cs @@ -3723,6 +3723,8 @@ namespace Emby.Server.Implementations.Library } } + // The libraries root was listed before this folder existed, so drop that listing: + // anything still reading it resolves the library set without the new folder. _directoryService.Invalidate(virtualFolderPath); } finally diff --git a/tests/Jellyfin.Server.Integration.Tests/Controllers/LibraryStructureControllerTests.cs b/tests/Jellyfin.Server.Integration.Tests/Controllers/LibraryStructureControllerTests.cs index 0a5838c545..79b9d1e2c5 100644 --- a/tests/Jellyfin.Server.Integration.Tests/Controllers/LibraryStructureControllerTests.cs +++ b/tests/Jellyfin.Server.Integration.Tests/Controllers/LibraryStructureControllerTests.cs @@ -6,8 +6,11 @@ using System.Text.Json; using System.Threading.Tasks; using Jellyfin.Api.Models.LibraryStructureDto; using Jellyfin.Extensions.Json; +using MediaBrowser.Controller; +using MediaBrowser.Controller.Providers; using MediaBrowser.Model.Configuration; using MediaBrowser.Model.Entities; +using Microsoft.Extensions.DependencyInjection; using Xunit; using Xunit.v3.Priority; @@ -25,6 +28,45 @@ public sealed class LibraryStructureControllerTests : IClassFixture(); + var rootFolderPath = _factory.Services.GetRequiredService().DefaultUserViewsPath; + + // Cache a listing of the libraries root taken before the new folder exists. Everything + // resolving through this DirectoryService keeps reading that listing until it is dropped, + // so the library stays invisible. Making the caches shared once turned this into a real + // test failure, see UpdateLibraryOptions_Valid_Success. + Assert.DoesNotContain( + directoryService.GetFileSystemEntries(rootFolderPath), + x => string.Equals(x.Name, Name, StringComparison.Ordinal)); + + var body = new AddVirtualFolderDto() + { + LibraryOptions = new LibraryOptions() + { + Enabled = false + } + }; + + using var response = await client.PostAsJsonAsync($"Library/VirtualFolders?name={Name}&refreshLibrary=false", body, _jsonOptions, TestContext.Current.CancellationToken); + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + + Assert.Contains( + directoryService.GetFileSystemEntries(rootFolderPath), + x => string.Equals(x.Name, Name, StringComparison.Ordinal)); + + using var cleanup = await client.DeleteAsync($"Library/VirtualFolders?name={Name}&refreshLibrary=false", TestContext.Current.CancellationToken); + Assert.Equal(HttpStatusCode.NoContent, cleanup.StatusCode); + } + [Fact] [Priority(-1)] public async Task Post_NewVirtualFolder_NotFound() -- cgit v1.2.3