diff options
14 files changed, 877 insertions, 83 deletions
diff --git a/Emby.Server.Implementations/IO/LibraryMonitor.cs b/Emby.Server.Implementations/IO/LibraryMonitor.cs index 1bf0f8c76c..0f92e2f03e 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; /// <summary> @@ -47,6 +49,7 @@ namespace Emby.Server.Implementations.IO /// <param name="libraryManager">The library manager.</param> /// <param name="configurationManager">The configuration manager.</param> /// <param name="fileSystem">The filesystem.</param> + /// <param name="directoryService">The directory service.</param> /// <param name="appLifetime">The <see cref="IHostApplicationLifetime"/>.</param> /// <param name="dotIgnoreIgnoreRule">The .ignore rule handler.</param> 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,8 @@ namespace Emby.Server.Implementations.IO return; } + _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 634cb8044c..e6fa94fbef 100644 --- a/Emby.Server.Implementations/Library/LibraryManager.cs +++ b/Emby.Server.Implementations/Library/LibraryManager.cs @@ -86,6 +86,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<Guid, BaseItem> _cache; private readonly DotIgnoreIgnoreRule _dotIgnoreIgnoreRule; private readonly IMediaStreamRepository _mediaStreamRepository; @@ -184,6 +185,7 @@ namespace Emby.Server.Implementations.Library _pathManager = pathManager; _dotIgnoreIgnoreRule = dotIgnoreIgnoreRule; _localization = localization; + _directoryService = directoryService; _extraResolver = new ExtraResolver(loggerFactory.CreateLogger<ExtraResolver>(), namingOptions, directoryService); _configurationManager.ConfigurationUpdated += ConfigurationUpdated; @@ -3774,6 +3776,10 @@ namespace Emby.Server.Implementations.Library AddMediaPathInternal(name, path, false); } } + + // 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 { @@ -3956,6 +3962,7 @@ namespace Emby.Server.Implementations.Library try { Directory.Delete(path, true); + _directoryService.Invalidate(path); } finally { @@ -4025,6 +4032,7 @@ namespace Emby.Server.Implementations.Library if (!string.IsNullOrEmpty(shortcut)) { _fileSystem.DeleteFile(shortcut); + _directoryService.Invalidate(shortcut); } var libraryOptions = CollectionFolder.GetLibraryOptions(virtualFolderPath); @@ -4068,6 +4076,7 @@ namespace Emby.Server.Implementations.Library } _fileSystem.CreateShortcut(lnk, _appHost.ReverseVirtualPath(path)); + _directoryService.Invalidate(lnk); RemoveContentTypeOverrides(path); } diff --git a/Jellyfin.Api/Controllers/LibraryStructureController.cs b/Jellyfin.Api/Controllers/LibraryStructureController.cs index 5c596c21b9..65bfe25d21 100644 --- a/Jellyfin.Api/Controllers/LibraryStructureController.cs +++ b/Jellyfin.Api/Controllers/LibraryStructureController.cs @@ -16,6 +16,7 @@ using MediaBrowser.Controller.Configuration; using MediaBrowser.Controller.Entities; using MediaBrowser.Controller.IO; using MediaBrowser.Controller.Library; +using MediaBrowser.Controller.Providers; using MediaBrowser.Model.Configuration; using MediaBrowser.Model.Entities; using Microsoft.AspNetCore.Authorization; @@ -34,6 +35,7 @@ public class LibraryStructureController : BaseJellyfinApiController private readonly IServerApplicationPaths _appPaths; private readonly ILibraryManager _libraryManager; private readonly ILibraryMonitor _libraryMonitor; + private readonly IDirectoryService _directoryService; /// <summary> /// Initializes a new instance of the <see cref="LibraryStructureController"/> class. @@ -41,14 +43,17 @@ public class LibraryStructureController : BaseJellyfinApiController /// <param name="serverConfigurationManager">Instance of <see cref="IServerConfigurationManager"/> interface.</param> /// <param name="libraryManager">Instance of <see cref="ILibraryManager"/> interface.</param> /// <param name="libraryMonitor">Instance of <see cref="ILibraryMonitor"/> interface.</param> + /// <param name="directoryService">Instance of <see cref="IDirectoryService"/> interface.</param> public LibraryStructureController( IServerConfigurationManager serverConfigurationManager, ILibraryManager libraryManager, - ILibraryMonitor libraryMonitor) + ILibraryMonitor libraryMonitor, + IDirectoryService directoryService) { _appPaths = serverConfigurationManager.ApplicationPaths; _libraryManager = libraryManager; _libraryMonitor = libraryMonitor; + _directoryService = directoryService; } /// <summary> @@ -178,11 +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); + _directoryService.Move(currentPath, newPath); } finally { diff --git a/MediaBrowser.Controller/LibraryTaskScheduler/LimitedConcurrencyLibraryScheduler.cs b/MediaBrowser.Controller/LibraryTaskScheduler/LimitedConcurrencyLibraryScheduler.cs index 6da398129a..be75117b6f 100644 --- a/MediaBrowser.Controller/LibraryTaskScheduler/LimitedConcurrencyLibraryScheduler.cs +++ b/MediaBrowser.Controller/LibraryTaskScheduler/LimitedConcurrencyLibraryScheduler.cs @@ -17,7 +17,8 @@ namespace MediaBrowser.Controller.LibraryTaskScheduler; /// </summary> public sealed class LimitedConcurrencyLibraryScheduler : ILimitedConcurrencyLibraryScheduler, IAsyncDisposable { - private const int CleanupGracePeriod = 60; + private static readonly TimeSpan _cleanupGracePeriod = TimeSpan.FromSeconds(60); + private readonly IHostApplicationLifetime _hostApplicationLifetime; private readonly ILogger<LimitedConcurrencyLibraryScheduler> _logger; private readonly IServerConfigurationManager _serverConfigurationManager; @@ -31,6 +32,8 @@ public sealed class LimitedConcurrencyLibraryScheduler : ILimitedConcurrencyLibr private readonly Lock _taskLock = new(); private readonly Channel<TaskQueueItem> _tasks = Channel.CreateUnbounded<TaskQueueItem>(); + private readonly CancellationTokenSource _disposeTokenSource = new(); + private readonly TimeSpan _gracePeriod; private volatile int _workCounter; private Task? _cleanupTask; @@ -46,10 +49,34 @@ public sealed class LimitedConcurrencyLibraryScheduler : ILimitedConcurrencyLibr IHostApplicationLifetime hostApplicationLifetime, ILogger<LimitedConcurrencyLibraryScheduler> logger, IServerConfigurationManager serverConfigurationManager) + : this(hostApplicationLifetime, logger, serverConfigurationManager, _cleanupGracePeriod) + { + } + + internal LimitedConcurrencyLibraryScheduler( + IHostApplicationLifetime hostApplicationLifetime, + ILogger<LimitedConcurrencyLibraryScheduler> logger, + IServerConfigurationManager serverConfigurationManager, + TimeSpan gracePeriod) { _hostApplicationLifetime = hostApplicationLifetime; _logger = logger; _serverConfigurationManager = serverConfigurationManager; + _gracePeriod = gracePeriod; + } + + /// <summary> + /// Gets the number of runners the scheduler currently keeps alive. + /// </summary> + internal int ActiveRunnerCount + { + get + { + lock (_taskLock) + { + return _taskRunners.Count; + } + } } private void ScheduleTaskCleanup() @@ -68,31 +95,65 @@ public sealed class LimitedConcurrencyLibraryScheduler : ILimitedConcurrencyLibr async Task RunCleanupTask() { - _logger.LogDebug("Schedule cleanup task in {CleanupGracePerioid} sec.", CleanupGracePeriod); - await Task.Delay(TimeSpan.FromSeconds(CleanupGracePeriod)).ConfigureAwait(false); - if (_disposed) + while (true) { - _logger.LogDebug("Abort cleaning up, already disposed."); - return; - } + _logger.LogDebug("Schedule cleanup task in {CleanupGracePeriod}.", _gracePeriod); + try + { + await Task.Delay(_gracePeriod, _disposeTokenSource.Token).ConfigureAwait(false); + } + catch (OperationCanceledException) + { + _logger.LogDebug("Abort cleaning up, already disposed."); + return; + } - lock (_taskLock) - { - if (_tasks.Reader.Count > 0 || _workCounter > 0) + if (_disposed) { - _logger.LogDebug("Delay cleanup task, operations still running."); - // tasks are still there so its still in use. Reschedule cleanup task. - // we cannot just exit here and rely on the other invoker because there is a considerable timeframe where it could have already ended. - _cleanupTask = RunCleanupTask(); + _logger.LogDebug("Abort cleaning up, already disposed."); return; } + + CancellationTokenSource[] runners; + lock (_taskLock) + { + if (_tasks.Reader.Count > 0 || _workCounter > 0) + { + _logger.LogDebug("Delay cleanup task, operations still running."); + // tasks are still there so its still in use. Wait another grace period. + // we cannot just exit here and rely on the other invoker because there is a considerable timeframe where it could have already ended. + continue; + } + + runners = [.. _taskRunners.Keys]; + + // Retire the runners before they are told to stop: an operation starting while + // they wind down must spawn its own instead of counting these towards the fanout. + _taskRunners.Clear(); + + // Hand the next operation the ability to schedule a cleanup again. Without this + // the very first cleanup would be the only one that ever runs. + _cleanupTask = null; + } + + _logger.LogDebug("Cleanup runners."); + await StopRunners(runners).ConfigureAwait(false); + return; } + } + } - _logger.LogDebug("Cleanup runners."); - foreach (var item in _taskRunners.ToArray()) + private static async Task StopRunners(CancellationTokenSource[] runners) + { + foreach (var runner in runners) + { + try { - await item.Key.CancelAsync().ConfigureAwait(false); - _taskRunners.Remove(item.Key); + await runner.CancelAsync().ConfigureAwait(false); + } + catch (ObjectDisposedException) + { + // The runner already stopped on its own and disposed its stop source. } } } @@ -127,12 +188,17 @@ public sealed class LimitedConcurrencyLibraryScheduler : ILimitedConcurrencyLibr { var stopToken = new CancellationTokenSource(); var combinedSource = CancellationTokenSource.CreateLinkedTokenSource(stopToken.Token, _hostApplicationLifetime.ApplicationStopping); + + // Keyed on its own stop source, because cancelling that is what reaches the linked + // source the runner waits on. Cancellation does not travel the other way. + // Started without the runner's own token: a task cancelled before it is scheduled + // never runs its body, so it would never take itself out of _taskRunners again. _taskRunners.Add( - combinedSource, + stopToken, Task.Factory.StartNew( ItemWorker, - (combinedSource, stopToken), - combinedSource.Token, + (stopToken, combinedSource), + CancellationToken.None, TaskCreationOptions.PreferFairness, TaskScheduler.Default)); } @@ -145,7 +211,7 @@ public sealed class LimitedConcurrencyLibraryScheduler : ILimitedConcurrencyLibr _deadlockDetector.Value = stopToken.TaskStop; try { - while (!stopToken.GlobalStop.Token.IsCancellationRequested) + while (!stopToken.GlobalStop.IsCancellationRequested) { var item = await _tasks.Reader.ReadAsync(stopToken.GlobalStop.Token).ConfigureAwait(false); try @@ -162,15 +228,24 @@ public sealed class LimitedConcurrencyLibraryScheduler : ILimitedConcurrencyLibr } } } - catch (OperationCanceledException) when (stopToken.TaskStop.IsCancellationRequested) + catch (OperationCanceledException) when (stopToken.GlobalStop.IsCancellationRequested) { // thats how you do it, interupt the waiter thread. There is nothing to do here when it was on purpose. } + catch (ChannelClosedException) + { + // the scheduler was disposed and will not hand out any more work. + } finally { _logger.LogDebug("Cleanup Runner'."); _deadlockDetector.Value = default!; - _taskRunners.Remove(stopToken.TaskStop); + + lock (_taskLock) + { + _taskRunners.Remove(stopToken.TaskStop); + } + stopToken.GlobalStop.Dispose(); stopToken.TaskStop.Dispose(); } @@ -195,7 +270,7 @@ public sealed class LimitedConcurrencyLibraryScheduler : ILimitedConcurrencyLibr finally { item.Progress.Report(100); - item.Done.SetResult(); + item.Done.TrySetResult(); } } @@ -285,16 +360,33 @@ public sealed class LimitedConcurrencyLibraryScheduler : ILimitedConcurrencyLibr _disposed = true; _tasks.Writer.Complete(); - foreach (var item in _taskRunners) + + // Nobody is left to run these, so release whoever is waiting on them. + while (_tasks.Reader.TryRead(out var item)) { - await item.Key.CancelAsync().ConfigureAwait(false); + item.Done.TrySetResult(); } - if (_cleanupTask is not null) + CancellationTokenSource[] runners; + Task? cleanupTask; + lock (_taskLock) { - await _cleanupTask.ConfigureAwait(false); - _cleanupTask?.Dispose(); + runners = [.. _taskRunners.Keys]; + _taskRunners.Clear(); + cleanupTask = _cleanupTask; } + + await StopRunners(runners).ConfigureAwait(false); + + // Cuts the grace period short instead of holding up shutdown for the rest of it. + await _disposeTokenSource.CancelAsync().ConfigureAwait(false); + + if (cleanupTask is not null) + { + await cleanupTask.ConfigureAwait(false); + } + + _disposeTokenSource.Dispose(); } private class TaskQueueItem 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 @@ </PropertyGroup> <ItemGroup> - <PackageReference Include="BitFaster.Caching" /> <PackageReference Include="Microsoft.Extensions.Configuration.Binder" /> </ItemGroup> diff --git a/MediaBrowser.Controller/Providers/DirectoryService.cs b/MediaBrowser.Controller/Providers/DirectoryService.cs index 6060d051a5..f8e0bf4ed9 100644 --- a/MediaBrowser.Controller/Providers/DirectoryService.cs +++ b/MediaBrowser.Controller/Providers/DirectoryService.cs @@ -5,13 +5,19 @@ using System.Collections.Concurrent; using System.Collections.Generic; using System.IO; using System.Linq; +using System.Threading; using MediaBrowser.Model.IO; namespace MediaBrowser.Controller.Providers { public class DirectoryService : IDirectoryService { - // TODO make static and switch to FastConcurrentLru. + // TODO replace with one shared bounded cache. + private const int MaxCachedRecords = 100_000; + private const int AccessIntervalMs = 1_000; + // Timeout cache if no access for 5 minutes. + private const int IdleTimeoutMs = 5 * 60 * 1_000; + private readonly ConcurrentDictionary<string, FileSystemMetadata[]> _cache = new(StringComparer.Ordinal); private readonly ConcurrentDictionary<string, FileSystemMetadata> _fileCache = new(StringComparer.Ordinal); @@ -20,6 +26,12 @@ namespace MediaBrowser.Controller.Providers private readonly IFileSystem _fileSystem; + // ConcurrentDictionary.Count locks the dictionary, so keep an estimated counter. + // Concurrent factory runs can overcount and a clear racing an add can undercount, + // it only has to be roughly right. + private int _recordCount; + private long _lastAccess = Environment.TickCount64; + public DirectoryService(IFileSystem fileSystem) { _fileSystem = fileSystem; @@ -27,20 +39,26 @@ namespace MediaBrowser.Controller.Providers public FileSystemMetadata[] GetFileSystemEntries(string path) { + DropCacheIfIdleOrFull(); + return _cache.GetOrAdd( path, - static (p, fileSystem) => + static (p, state) => { + FileSystemMetadata[] entries; try { - return fileSystem.GetFileSystemEntries(p).ToArray(); + entries = state.FileSystem.GetFileSystemEntries(p).ToArray(); } catch (DirectoryNotFoundException) { - return []; + entries = []; } + + Interlocked.Add(ref state.Service._recordCount, entries.Length + 1); + return entries; }, - _fileSystem); + (FileSystem: _fileSystem, Service: this)); } public List<FileSystemMetadata> GetDirectories(string path) @@ -89,13 +107,18 @@ namespace MediaBrowser.Controller.Providers public FileSystemMetadata? GetFileSystemEntry(string path) { + DropCacheIfIdleOrFull(); + if (!_fileCache.TryGetValue(path, out var result)) { var file = _fileSystem.GetFileSystemInfo(path); if (file?.Exists ?? false) { result = file; - _fileCache.TryAdd(path, result); + if (_fileCache.TryAdd(path, result)) + { + Interlocked.Increment(ref _recordCount); + } } } @@ -107,32 +130,96 @@ namespace MediaBrowser.Controller.Providers public IReadOnlyList<string> GetFilePaths(string path, bool clearCache) { - if (clearCache) + if (clearCache && _filePathCache.TryRemove(path, out var cached)) { - _filePathCache.TryRemove(path, out _); + Interlocked.Add(ref _recordCount, -(cached.Count + 1)); } + DropCacheIfIdleOrFull(); + var filePaths = _filePathCache.GetOrAdd( path, - static (p, fileSystem) => + static (p, state) => { + List<string> filePaths; try { - return fileSystem.GetFilePaths(p).OrderBy(x => x).ToList(); + filePaths = state.FileSystem.GetFilePaths(p).OrderBy(x => x).ToList(); } catch (DirectoryNotFoundException) { - return []; + filePaths = []; } + + Interlocked.Add(ref state.Service._recordCount, filePaths.Count + 1); + return filePaths; }, - _fileSystem); + (FileSystem: _fileSystem, Service: this)); return filePaths; } + public void Invalidate(string path) + { + Forget(path); + + var parent = Path.GetDirectoryName(path); + if (!string.IsNullOrEmpty(parent)) + { + Forget(parent); + } + } + + 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(); } + + private void DropCacheIfIdleOrFull() + { + var nowMs = Environment.TickCount64; + var idleMs = nowMs - _lastAccess; + + if (idleMs >= IdleTimeoutMs || _recordCount >= MaxCachedRecords) + { + _cache.Clear(); + _fileCache.Clear(); + _filePathCache.Clear(); + _recordCount = 0; + _lastAccess = nowMs; + return; + } + + if (idleMs >= AccessIntervalMs) + { + _lastAccess = nowMs; + } + } + + private void Forget(string path) + { + if (_cache.TryRemove(path, out var entries)) + { + Interlocked.Add(ref _recordCount, -(entries.Length + 1)); + } + + if (_fileCache.TryRemove(path, out _)) + { + Interlocked.Decrement(ref _recordCount); + } + + if (_filePathCache.TryRemove(path, out var filePaths)) + { + Interlocked.Add(ref _recordCount, -(filePaths.Count + 1)); + } + } } } diff --git a/MediaBrowser.Controller/Providers/IDirectoryService.cs b/MediaBrowser.Controller/Providers/IDirectoryService.cs index 8a3fa33da3..3a943d5f0c 100644 --- a/MediaBrowser.Controller/Providers/IDirectoryService.cs +++ b/MediaBrowser.Controller/Providers/IDirectoryService.cs @@ -23,6 +23,19 @@ namespace MediaBrowser.Controller.Providers IReadOnlyList<string> GetFilePaths(string path, bool clearCache); + /// <summary> + /// Forgets what is cached about a path and about the directory containing it. + /// </summary> + /// <param name="path">The file or directory path that changed.</param> + void Invalidate(string path); + + /// <summary> + /// Moves a directory and forgets what is cached about both paths. + /// </summary> + /// <param name="source">The directory to move.</param> + /// <param name="destination">The path to move the directory to.</param> + 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 af31e373ef..a19262c3a7 100644 --- a/MediaBrowser.Providers/Lyric/LyricManager.cs +++ b/MediaBrowser.Providers/Lyric/LyricManager.cs @@ -32,6 +32,7 @@ public class LyricManager : ILyricManager private readonly IFileSystem _fileSystem; private readonly ILibraryMonitor _libraryMonitor; private readonly IMediaSourceManager _mediaSourceManager; + private readonly IDirectoryService _directoryService; private readonly ILyricProvider[] _lyricProviders; private readonly ILyricParser[] _lyricParsers; @@ -43,6 +44,7 @@ public class LyricManager : ILyricManager /// <param name="fileSystem">Instance of the <see cref="IFileSystem"/> interface.</param> /// <param name="libraryMonitor">Instance of the <see cref="ILibraryMonitor"/> interface.</param> /// <param name="mediaSourceManager">Instance of the <see cref="IMediaSourceManager"/> interface.</param> + /// <param name="directoryService">Instance of the <see cref="IDirectoryService"/> interface.</param> /// <param name="lyricProviders">The list of <see cref="ILyricProvider"/>.</param> /// <param name="lyricParsers">The list of <see cref="ILyricParser"/>.</param> public LyricManager( @@ -50,6 +52,7 @@ public class LyricManager : ILyricManager IFileSystem fileSystem, ILibraryMonitor libraryMonitor, IMediaSourceManager mediaSourceManager, + IDirectoryService directoryService, IEnumerable<ILyricProvider> lyricProviders, IEnumerable<ILyricParser> lyricParsers) { @@ -57,6 +60,7 @@ public class LyricManager : ILyricManager _fileSystem = fileSystem; _libraryMonitor = libraryMonitor; _mediaSourceManager = mediaSourceManager; + _directoryService = directoryService; _lyricProviders = lyricProviders .OrderBy(i => i is IHasOrder hasOrder ? hasOrder.Order : 0) .ToArray(); @@ -250,6 +254,8 @@ public class LyricManager : ILyricManager { _libraryMonitor.ReportFileSystemChangeComplete(path, false); } + + _directoryService.Invalidate(path); } return audio.RefreshMetadata(CancellationToken.None); @@ -446,6 +452,8 @@ public class LyricManager : ILyricManager await stream.CopyToAsync(fs).ConfigureAwait(false); } + _directoryService.Invalidate(savePath); + return; } catch (Exception ex) diff --git a/MediaBrowser.Providers/Manager/ProviderManager.cs b/MediaBrowser.Providers/Manager/ProviderManager.cs index fbd9e5435e..e7b15305b3 100644 --- a/MediaBrowser.Providers/Manager/ProviderManager.cs +++ b/MediaBrowser.Providers/Manager/ProviderManager.cs @@ -1143,16 +1143,21 @@ namespace MediaBrowser.Providers.Manager return; } - _refreshQueue.Enqueue((itemId, options), priority); - + // PriorityQueue is not thread safe and the processor dequeues concurrently, so every + // touch of the queue takes the lock. lock (_refreshQueueLock) { - if (!_isProcessingRefreshQueue) + _refreshQueue.Enqueue((itemId, options), priority); + + if (_isProcessingRefreshQueue) { - _isProcessingRefreshQueue = true; - Task.Run(StartProcessingRefreshQueue); + return; } + + _isProcessingRefreshQueue = true; } + + Task.Run(StartProcessingRefreshQueue); } private async Task StartProcessingRefreshQueue() @@ -1161,17 +1166,33 @@ namespace MediaBrowser.Providers.Manager if (_disposed) { + lock (_refreshQueueLock) + { + _isProcessingRefreshQueue = false; + } + return; } var cancellationToken = _disposeCancellationTokenSource.Token; libraryManager.ClearIgnoreRuleCache(); - while (_refreshQueue.TryDequeue(out var refreshItem, out _)) + + while (true) { - if (_disposed) + (Guid ItemId, MetadataRefreshOptions RefreshOptions) refreshItem; + + // 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) { - return; + if (_disposed + || cancellationToken.IsCancellationRequested + || !_refreshQueue.TryDequeue(out refreshItem, out _)) + { + _isProcessingRefreshQueue = false; + break; + } } try @@ -1188,19 +1209,21 @@ namespace MediaBrowser.Providers.Manager await task.ConfigureAwait(false); } - catch (OperationCanceledException) + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { - break; + // Shutting down: the next pass sees the token and stands the processor down. + continue; } catch (Exception ex) { + // 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"); } } - lock (_refreshQueueLock) + if (!_disposed) { - _isProcessingRefreshQueue = false; libraryManager.ClearIgnoreRuleCache(); } } diff --git a/MediaBrowser.Providers/Subtitles/SubtitleManager.cs b/MediaBrowser.Providers/Subtitles/SubtitleManager.cs index c3458d4b2a..cd9dda21a0 100644 --- a/MediaBrowser.Providers/Subtitles/SubtitleManager.cs +++ b/MediaBrowser.Providers/Subtitles/SubtitleManager.cs @@ -33,6 +33,7 @@ namespace MediaBrowser.Providers.Subtitles private readonly ILibraryMonitor _monitor; private readonly IMediaSourceManager _mediaSourceManager; private readonly ILocalizationManager _localization; + private readonly IDirectoryService _directoryService; private readonly HashSet<string> _allowedSubtitleFormats; private readonly ISubtitleProvider[] _subtitleProviders; @@ -43,6 +44,7 @@ namespace MediaBrowser.Providers.Subtitles ILibraryMonitor monitor, IMediaSourceManager mediaSourceManager, ILocalizationManager localizationManager, + IDirectoryService directoryService, IEnumerable<ISubtitleProvider> subtitleProviders, NamingOptions namingOptions) { @@ -51,6 +53,7 @@ namespace MediaBrowser.Providers.Subtitles _monitor = monitor; _mediaSourceManager = mediaSourceManager; _localization = localizationManager; + _directoryService = directoryService; _subtitleProviders = subtitleProviders .OrderBy(i => i is IHasOrder hasOrder ? hasOrder.Order : 0) .ToArray(); @@ -281,6 +284,8 @@ namespace MediaBrowser.Providers.Subtitles await stream.CopyToAsync(fs).ConfigureAwait(false); } + _directoryService.Invalidate(path); + return; } else @@ -395,6 +400,8 @@ namespace MediaBrowser.Providers.Subtitles _monitor.ReportFileSystemChangeComplete(path, false); } + _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 1f59908a86..e57fbfe473 100644 --- a/tests/Jellyfin.Controller.Tests/DirectoryServiceTests.cs +++ b/tests/Jellyfin.Controller.Tests/DirectoryServiceTests.cs @@ -1,3 +1,5 @@ +using System.Globalization; +using System.IO; using System.Linq; using MediaBrowser.Controller.Providers; using MediaBrowser.Model.IO; @@ -8,29 +10,31 @@ namespace Jellyfin.Controller.Tests { public class DirectoryServiceTests { - private const string LowerCasePath = "/music/someartist"; - private const string UpperCasePath = "/music/SOMEARTIST"; + // Path.GetDirectoryName, which Invalidate uses to find the parent, normalizes the + // separators, so cache keys only match the parent it returns when they use the platform's. + private static readonly string _lowerCasePath = LocalPath("/music/someartist"); + private static readonly string _upperCasePath = LocalPath("/music/SOMEARTIST"); private static readonly FileSystemMetadata[] _lowerCaseFileSystemMetadata = { new() { - FullName = LowerCasePath + "/Artwork", + FullName = Path.Combine(_lowerCasePath, "Artwork"), IsDirectory = true }, new() { - FullName = LowerCasePath + "/Some Other Folder", + FullName = Path.Combine(_lowerCasePath, "Some Other Folder"), IsDirectory = true }, new() { - FullName = LowerCasePath + "/Song 2.mp3", + FullName = Path.Combine(_lowerCasePath, "Song 2.mp3"), IsDirectory = false }, new() { - FullName = LowerCasePath + "/Song 3.mp3", + FullName = Path.Combine(_lowerCasePath, "Song 3.mp3"), IsDirectory = false } }; @@ -39,12 +43,12 @@ namespace Jellyfin.Controller.Tests { new() { - FullName = UpperCasePath + "/Lyrics", + FullName = Path.Combine(_upperCasePath, "Lyrics"), IsDirectory = true }, new() { - FullName = UpperCasePath + "/Song 1.mp3", + FullName = Path.Combine(_upperCasePath, "Song 1.mp3"), IsDirectory = false } }; @@ -53,12 +57,12 @@ namespace Jellyfin.Controller.Tests public void GetFileSystemEntries_GivenPathsWithDifferentCasing_CachesAll() { var fileSystemMock = new Mock<IFileSystem>(); - fileSystemMock.Setup(f => f.GetFileSystemEntries(It.Is<string>(x => x == UpperCasePath), false)).Returns(_upperCaseFileSystemMetadata); - fileSystemMock.Setup(f => f.GetFileSystemEntries(It.Is<string>(x => x == LowerCasePath), false)).Returns(_lowerCaseFileSystemMetadata); + fileSystemMock.Setup(f => f.GetFileSystemEntries(It.Is<string>(x => x == _upperCasePath), false)).Returns(_upperCaseFileSystemMetadata); + fileSystemMock.Setup(f => f.GetFileSystemEntries(It.Is<string>(x => x == _lowerCasePath), false)).Returns(_lowerCaseFileSystemMetadata); var directoryService = new DirectoryService(fileSystemMock.Object); - var upperCaseResult = directoryService.GetFileSystemEntries(UpperCasePath); - var lowerCaseResult = directoryService.GetFileSystemEntries(LowerCasePath); + var upperCaseResult = directoryService.GetFileSystemEntries(_upperCasePath); + var lowerCaseResult = directoryService.GetFileSystemEntries(_lowerCasePath); Assert.Equal(_upperCaseFileSystemMetadata, upperCaseResult); Assert.Equal(_lowerCaseFileSystemMetadata, lowerCaseResult); @@ -68,12 +72,12 @@ namespace Jellyfin.Controller.Tests public void GetFiles_GivenPathsWithDifferentCasing_ReturnsCorrectFiles() { var fileSystemMock = new Mock<IFileSystem>(); - fileSystemMock.Setup(f => f.GetFileSystemEntries(It.Is<string>(x => x == UpperCasePath), false)).Returns(_upperCaseFileSystemMetadata); - fileSystemMock.Setup(f => f.GetFileSystemEntries(It.Is<string>(x => x == LowerCasePath), false)).Returns(_lowerCaseFileSystemMetadata); + fileSystemMock.Setup(f => f.GetFileSystemEntries(It.Is<string>(x => x == _upperCasePath), false)).Returns(_upperCaseFileSystemMetadata); + fileSystemMock.Setup(f => f.GetFileSystemEntries(It.Is<string>(x => x == _lowerCasePath), false)).Returns(_lowerCaseFileSystemMetadata); var directoryService = new DirectoryService(fileSystemMock.Object); - var upperCaseResult = directoryService.GetFiles(UpperCasePath); - var lowerCaseResult = directoryService.GetFiles(LowerCasePath); + var upperCaseResult = directoryService.GetFiles(_upperCasePath); + var lowerCaseResult = directoryService.GetFiles(_lowerCasePath); Assert.Equal(_upperCaseFileSystemMetadata.Where(f => !f.IsDirectory), upperCaseResult); Assert.Equal(_lowerCaseFileSystemMetadata.Where(f => !f.IsDirectory), lowerCaseResult); @@ -83,12 +87,12 @@ namespace Jellyfin.Controller.Tests public void GetDirectories_GivenPathsWithDifferentCasing_ReturnsCorrectDirectories() { var fileSystemMock = new Mock<IFileSystem>(); - fileSystemMock.Setup(f => f.GetFileSystemEntries(It.Is<string>(x => x == UpperCasePath), false)).Returns(_upperCaseFileSystemMetadata); - fileSystemMock.Setup(f => f.GetFileSystemEntries(It.Is<string>(x => x == LowerCasePath), false)).Returns(_lowerCaseFileSystemMetadata); + fileSystemMock.Setup(f => f.GetFileSystemEntries(It.Is<string>(x => x == _upperCasePath), false)).Returns(_upperCaseFileSystemMetadata); + fileSystemMock.Setup(f => f.GetFileSystemEntries(It.Is<string>(x => x == _lowerCasePath), false)).Returns(_lowerCaseFileSystemMetadata); var directoryService = new DirectoryService(fileSystemMock.Object); - var upperCaseResult = directoryService.GetDirectories(UpperCasePath); - var lowerCaseResult = directoryService.GetDirectories(LowerCasePath); + var upperCaseResult = directoryService.GetDirectories(_upperCasePath); + var lowerCaseResult = directoryService.GetDirectories(_lowerCasePath); Assert.Equal(_upperCaseFileSystemMetadata.Where(f => f.IsDirectory), upperCaseResult); Assert.Equal(_lowerCaseFileSystemMetadata.Where(f => f.IsDirectory), lowerCaseResult); @@ -248,5 +252,171 @@ namespace Jellyfin.Controller.Tests Assert.Equal(cachedPaths, result); Assert.Equal(newPaths, secondResult); } + + [Fact] + public void GetFileSystemEntries_RepeatedPath_ReadsTheFileSystemOnce() + { + var fileSystemMock = new Mock<IFileSystem>(MockBehavior.Strict); + fileSystemMock.Setup(f => f.GetFileSystemEntries(_lowerCasePath)) + .Returns(_lowerCaseFileSystemMetadata); + + var directoryService = new DirectoryService(fileSystemMock.Object); + + directoryService.GetFileSystemEntries(_lowerCasePath); + directoryService.GetFileSystemEntries(_lowerCasePath); + + fileSystemMock.Verify(f => f.GetFileSystemEntries(_lowerCasePath), Times.Once); + } + + [Fact] + public void Invalidate_GivenADirectory_DropsBothTheListingAndTheFilePaths() + { + var fileSystemMock = new Mock<IFileSystem>(); + fileSystemMock.SetupSequence(f => f.GetFileSystemEntries(_lowerCasePath)) + .Returns(_lowerCaseFileSystemMetadata) + .Returns(_upperCaseFileSystemMetadata); + fileSystemMock.SetupSequence(f => f.GetFilePaths(_lowerCasePath, false)) + .Returns(new[] { Path.Combine(_lowerCasePath, "Song 2.mp3") }) + .Returns(new[] { Path.Combine(_lowerCasePath, "Song 2.mp3"), Path.Combine(_lowerCasePath, "Song 2.srt") }); + + var directoryService = new DirectoryService(fileSystemMock.Object); + directoryService.GetFileSystemEntries(_lowerCasePath); + directoryService.GetFilePaths(_lowerCasePath); + + directoryService.Invalidate(_lowerCasePath); + + Assert.Equal(_upperCaseFileSystemMetadata, directoryService.GetFileSystemEntries(_lowerCasePath)); + Assert.Equal(2, directoryService.GetFilePaths(_lowerCasePath).Count); + } + + [Fact] + public void Invalidate_GivenAFile_DropsTheListingOfTheDirectoryHoldingIt() + { + var newFile = Path.Combine(_lowerCasePath, "Song 2.srt"); + + var fileSystemMock = new Mock<IFileSystem>(); + fileSystemMock.SetupSequence(f => f.GetFileSystemEntries(_lowerCasePath)) + .Returns(_lowerCaseFileSystemMetadata) + .Returns(_upperCaseFileSystemMetadata); + + var directoryService = new DirectoryService(fileSystemMock.Object); + directoryService.GetFileSystemEntries(_lowerCasePath); + + directoryService.Invalidate(newFile); + + Assert.Equal(_upperCaseFileSystemMetadata, directoryService.GetFileSystemEntries(_lowerCasePath)); + } + + [Fact] + public void GetFilePaths_ClearingTheCache_KeepsTheParentDirectory() + { + var parentPath = LocalPath("/music"); + + var fileSystemMock = new Mock<IFileSystem>(); + fileSystemMock.Setup(f => f.GetFilePaths(_lowerCasePath)) + .Returns(new[] { Path.Combine(_lowerCasePath, "Song 2.mp3") }); + fileSystemMock.Setup(f => f.GetFileSystemEntries(parentPath)) + .Returns(_lowerCaseFileSystemMetadata); + + var directoryService = new DirectoryService(fileSystemMock.Object); + directoryService.GetFileSystemEntries(parentPath); + + directoryService.GetFilePaths(_lowerCasePath, true); + + directoryService.GetFileSystemEntries(parentPath); + fileSystemMock.Verify(f => f.GetFileSystemEntries(parentPath), Times.Once); + } + + [Fact] + public void GetFileSystemEntries_MoreRecordsThanTheCeiling_DropsCache() + { + // Charged by the files in a listing, not the number of listings, so a few big folders + // reach the limit where a lot of small ones would not. + const int FolderCount = 60; + var bigListing = new FileSystemMetadata[5000]; + for (var i = 0; i < bigListing.Length; i++) + { + bigListing[i] = new FileSystemMetadata + { + FullName = "/music/track" + i.ToString(CultureInfo.InvariantCulture), + IsDirectory = false + }; + } + + var fileSystemMock = new Mock<IFileSystem>(); + fileSystemMock.Setup(f => f.GetFileSystemEntries(It.IsAny<string>())) + .Returns(bigListing); + + var directoryService = new DirectoryService(fileSystemMock.Object); + + const string FirstPath = "/music/artist0"; + directoryService.GetFileSystemEntries(FirstPath); + + for (var i = 1; i < FolderCount; 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_RepeatedlyInvalidatedFolder_KeepsUnrelatedEntriesCached() + { + // Invalidating gives the records back, so churning one folder must not add up to the + // ceiling and drop everything else with it. + const int ChurnCount = 50; + var bigListing = new FileSystemMetadata[5000]; + for (var i = 0; i < bigListing.Length; i++) + { + bigListing[i] = new FileSystemMetadata + { + FullName = "/music/track" + i.ToString(CultureInfo.InvariantCulture), + IsDirectory = false + }; + } + + var fileSystemMock = new Mock<IFileSystem>(); + fileSystemMock.Setup(f => f.GetFileSystemEntries(It.IsAny<string>())) + .Returns(bigListing); + + var directoryService = new DirectoryService(fileSystemMock.Object); + + const string ChurnedPath = "/music/watched"; + const string StablePath = "/music/untouched"; + directoryService.GetFileSystemEntries(StablePath); + + for (var i = 0; i < ChurnCount; i++) + { + directoryService.GetFileSystemEntries(ChurnedPath); + directoryService.Invalidate(ChurnedPath); + } + + directoryService.GetFileSystemEntries(StablePath); + + fileSystemMock.Verify(f => f.GetFileSystemEntries(StablePath), Times.Once); + } + + [Fact] + public void GetFileSystemEntry_MissingPath_IsNotRemembered() + { + const string MissingPath = "/music/not-here"; + + var fileSystemMock = new Mock<IFileSystem>(); + fileSystemMock.SetupSequence(f => f.GetFileSystemInfo(MissingPath)) + .Returns(new FileSystemMetadata { FullName = MissingPath, Exists = false }) + .Returns(new FileSystemMetadata { FullName = MissingPath, Exists = true }); + + var directoryService = new DirectoryService(fileSystemMock.Object); + + Assert.Null(directoryService.GetFileSystemEntry(MissingPath)); + + Assert.NotNull(directoryService.GetFileSystemEntry(MissingPath)); + } + + private static string LocalPath(string path) + => path.Replace('/', Path.DirectorySeparatorChar); } } diff --git a/tests/Jellyfin.Controller.Tests/LibraryTaskScheduler/LimitedConcurrencyLibrarySchedulerTests.cs b/tests/Jellyfin.Controller.Tests/LibraryTaskScheduler/LimitedConcurrencyLibrarySchedulerTests.cs new file mode 100644 index 0000000000..686d839f4f --- /dev/null +++ b/tests/Jellyfin.Controller.Tests/LibraryTaskScheduler/LimitedConcurrencyLibrarySchedulerTests.cs @@ -0,0 +1,216 @@ +using System; +using System.Collections.Concurrent; +using System.Diagnostics; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using MediaBrowser.Controller.Configuration; +using MediaBrowser.Controller.LibraryTaskScheduler; +using MediaBrowser.Model.Configuration; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; +using Xunit; + +namespace Jellyfin.Controller.Tests.LibraryTaskScheduler +{ + public class LimitedConcurrencyLibrarySchedulerTests + { + private static readonly TimeSpan _shortGracePeriod = TimeSpan.FromMilliseconds(50); + + // Generous, because these only ever wait for something that should already have happened. + private static readonly TimeSpan _timeout = TimeSpan.FromSeconds(10); + + [Fact] + public async Task Enqueue_ProcessesEveryItem() + { + using var appStopping = new CancellationTokenSource(); + var scheduler = CreateScheduler(appStopping); + await using (scheduler) + { + var data = Enumerable.Range(0, 100).ToArray(); + var processed = new ConcurrentBag<int>(); + + await scheduler.Enqueue( + data, + (item, _) => + { + processed.Add(item); + return Task.CompletedTask; + }, + new Progress<double>(), + CancellationToken.None); + + Assert.Equal(data, processed.Order()); + } + } + + [Fact] + public async Task Enqueue_WithFailingWorker_StillCompletes() + { + using var appStopping = new CancellationTokenSource(); + var scheduler = CreateScheduler(appStopping); + await using (scheduler) + { + await scheduler.Enqueue( + Enumerable.Range(0, 20).ToArray(), + (item, _) => item % 2 == 0 ? throw new InvalidOperationException("boom") : Task.CompletedTask, + new Progress<double>(), + CancellationToken.None); + } + } + + /// <summary> + /// The runners wait on a source linked to <see cref="IHostApplicationLifetime.ApplicationStopping"/>, + /// so a shutdown has to reach them. It does not travel from the linked source back to the one + /// the cleanup cancels, which is what made them immortal. + /// </summary> + /// <returns>A <see cref="Task"/> representing the asynchronous unit test.</returns> + [Fact] + public async Task ApplicationStopping_RetiresRunners() + { + using var appStopping = new CancellationTokenSource(); + + // Long enough that the cleanup cannot be what retires them. + var scheduler = CreateScheduler(appStopping, gracePeriod: TimeSpan.FromMinutes(5)); + await using (scheduler) + { + await RunOneOperation(scheduler); + Assert.True(scheduler.ActiveRunnerCount > 0); + + await appStopping.CancelAsync(); + + await WaitForAsync(() => scheduler.ActiveRunnerCount == 0); + } + } + + /// <summary> + /// The cleanup used to be a one shot: it never released the scheduling slot it took, so + /// every runner spawned after the first pass stayed around for the lifetime of the server. + /// </summary> + /// <returns>A <see cref="Task"/> representing the asynchronous unit test.</returns> + [Fact] + public async Task Enqueue_RetiresIdleRunnersAfterEveryOperation() + { + using var appStopping = new CancellationTokenSource(); + var scheduler = CreateScheduler(appStopping); + await using (scheduler) + { + for (var round = 0; round < 3; round++) + { + await RunOneOperation(scheduler); + Assert.True(scheduler.ActiveRunnerCount > 0, $"no runner spawned in round {round}"); + + await WaitForAsync(() => scheduler.ActiveRunnerCount == 0); + } + } + } + + /// <summary> + /// Disposing used to sit out the rest of the cleanup grace period, holding up shutdown for + /// up to a minute. + /// </summary> + /// <returns>A <see cref="Task"/> representing the asynchronous unit test.</returns> + [Fact] + public async Task DisposeAsync_DoesNotWaitOutTheGracePeriod() + { + using var appStopping = new CancellationTokenSource(); + var scheduler = CreateScheduler(appStopping, gracePeriod: TimeSpan.FromMinutes(5)); + + await RunOneOperation(scheduler); + + var stopwatch = Stopwatch.StartNew(); + await scheduler.DisposeAsync(); + + Assert.True(stopwatch.Elapsed < _timeout, $"disposing took {stopwatch.Elapsed}"); + } + + [Fact] + public async Task Enqueue_AfterDispose_DoesNothing() + { + using var appStopping = new CancellationTokenSource(); + var scheduler = CreateScheduler(appStopping); + await scheduler.DisposeAsync(); + + var processed = 0; + await scheduler.Enqueue( + Enumerable.Range(0, 10).ToArray(), + (_, _) => + { + Interlocked.Increment(ref processed); + return Task.CompletedTask; + }, + new Progress<double>(), + CancellationToken.None); + + Assert.Equal(0, processed); + } + + [Theory] + [InlineData(1)] + [InlineData(4)] + public async Task Enqueue_FromWithinAWorker_DoesNotDeadlock(int fanout) + { + using var appStopping = new CancellationTokenSource(); + var scheduler = CreateScheduler(appStopping, fanout: fanout); + await using (scheduler) + { + var inner = 0; + + var outer = scheduler.Enqueue( + Enumerable.Range(0, 8).ToArray(), + (_, _) => scheduler.Enqueue( + Enumerable.Range(0, 4).ToArray(), + (_, _) => + { + Interlocked.Increment(ref inner); + return Task.CompletedTask; + }, + new Progress<double>(), + CancellationToken.None), + new Progress<double>(), + CancellationToken.None); + + await outer.WaitAsync(_timeout, TestContext.Current.CancellationToken); + + Assert.Equal(32, inner); + } + } + + private static LimitedConcurrencyLibraryScheduler CreateScheduler( + CancellationTokenSource appStopping, + int fanout = 4, + TimeSpan? gracePeriod = null) + { + var lifetime = new Mock<IHostApplicationLifetime>(); + lifetime.SetupGet(x => x.ApplicationStopping).Returns(() => appStopping.Token); + + var configurationManager = new Mock<IServerConfigurationManager>(); + configurationManager.SetupGet(x => x.Configuration) + .Returns(new ServerConfiguration { LibraryScanFanoutConcurrency = fanout }); + + return new LimitedConcurrencyLibraryScheduler( + lifetime.Object, + NullLogger<LimitedConcurrencyLibraryScheduler>.Instance, + configurationManager.Object, + gracePeriod ?? _shortGracePeriod); + } + + private static Task RunOneOperation(LimitedConcurrencyLibraryScheduler scheduler) + => scheduler.Enqueue( + Enumerable.Range(0, 8).ToArray(), + (_, _) => Task.CompletedTask, + new Progress<double>(), + CancellationToken.None); + + private static async Task WaitForAsync(Func<bool> condition) + { + var stopwatch = Stopwatch.StartNew(); + while (!condition()) + { + Assert.True(stopwatch.Elapsed < _timeout, "timed out waiting for the scheduler to settle"); + await Task.Delay(20, TestContext.Current.CancellationToken); + } + } + } +} diff --git a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs index 5749944fcd..248b236df8 100644 --- a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs +++ b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Linq; using System.Net.Http; @@ -377,6 +378,116 @@ namespace Jellyfin.Providers.Tests.Manager GetMetadataProviders_CanRefreshMetadata_Tester(providerType, expected, ownedItem: true); } + [Fact] + public async Task QueueRefresh_ManyItemsQueuedFromManyThreads_ProcessesEveryOne() + { + const int ItemCount = 2000; + + var queued = Enumerable.Range(0, ItemCount).Select(_ => Guid.NewGuid()).ToArray(); + var processed = new ConcurrentBag<Guid>(); + var allProcessed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + var libraryManager = new Mock<ILibraryManager>(); + libraryManager.Setup(i => i.GetItemById(It.IsAny<Guid>())) + .Returns((Guid id) => + { + // Returning null drains the entry without the whole refresh machinery. + processed.Add(id); + if (processed.Count == ItemCount) + { + allProcessed.TrySetResult(); + } + + return null; + }); + + using var providerManager = GetProviderManager(libraryManager: libraryManager.Object); + + await Parallel.ForEachAsync( + queued, + TestContext.Current.CancellationToken, + (id, _) => + { + providerManager.QueueRefresh(id, new MetadataRefreshOptions(Mock.Of<IDirectoryService>()), RefreshPriority.Normal); + return ValueTask.CompletedTask; + }); + + using var timeout = CancellationTokenSource.CreateLinkedTokenSource(TestContext.Current.CancellationToken); + timeout.CancelAfter(TimeSpan.FromSeconds(30)); + + try + { + await allProcessed.Task.WaitAsync(timeout.Token); + } + catch (OperationCanceledException) + { + // Fall through so the assertions report what was lost. + } + + Assert.Empty(providerManager.GetRefreshQueue()); + Assert.Equal(queued.Order().ToArray(), processed.Order().ToArray()); + } + + [Fact] + public async Task QueueRefresh_RefreshCancelsForItsOwnReasons_KeepsDrainingTheQueue() + { + // 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(); + var processed = new ConcurrentBag<Guid>(); + var allProcessed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + using var allQueued = new ManualResetEventSlim(false); + var cancelledOnce = false; + + var libraryManager = new Mock<ILibraryManager>(); + libraryManager.Setup(i => i.GetItemById(It.IsAny<Guid>())) + .Returns((Guid id) => + { + if (!cancelledOnce) + { + cancelledOnce = true; + + // Hold the first entry until the whole batch is queued. + allQueued.Wait(TimeSpan.FromSeconds(30)); + throw new OperationCanceledException("provider timed out"); + } + + processed.Add(id); + if (processed.Count == ItemCount - 1) + { + allProcessed.TrySetResult(); + } + + return null; + }); + + using var providerManager = GetProviderManager(libraryManager: libraryManager.Object); + + foreach (var id in queued) + { + providerManager.QueueRefresh(id, new MetadataRefreshOptions(Mock.Of<IDirectoryService>()), RefreshPriority.Normal); + } + + allQueued.Set(); + + using var timeout = CancellationTokenSource.CreateLinkedTokenSource(TestContext.Current.CancellationToken); + timeout.CancelAfter(TimeSpan.FromSeconds(30)); + + try + { + await allProcessed.Task.WaitAsync(timeout.Token); + } + catch (OperationCanceledException) + { + // Fall through so the assertions report what was stranded. + } + + Assert.Empty(providerManager.GetRefreshQueue()); + Assert.Equal(ItemCount - 1, processed.Count); + } + private static void GetMetadataProviders_CanRefreshMetadata_Tester( string providerType, bool expected, @@ -554,15 +665,20 @@ namespace Jellyfin.Providers.Tests.Manager private static ProviderManager GetProviderManager( ServerConfiguration? serverConfiguration = null, LibraryOptions? libraryOptions = null, - IBaseItemManager? baseItemManager = null) + IBaseItemManager? baseItemManager = null, + ILibraryManager? libraryManager = null) { var serverConfigurationManager = new Mock<IServerConfigurationManager>(MockBehavior.Strict); serverConfigurationManager.Setup(i => i.Configuration) .Returns(serverConfiguration ?? new ServerConfiguration()); - var libraryManager = new Mock<ILibraryManager>(MockBehavior.Strict); - libraryManager.Setup(i => i.GetLibraryOptions(It.IsAny<BaseItem>())) - .Returns(libraryOptions ?? new LibraryOptions()); + if (libraryManager is null) + { + var libraryManagerMock = new Mock<ILibraryManager>(MockBehavior.Strict); + libraryManagerMock.Setup(i => i.GetLibraryOptions(It.IsAny<BaseItem>())) + .Returns(libraryOptions ?? new LibraryOptions()); + libraryManager = libraryManagerMock.Object; + } var providerManager = new ProviderManager( Mock.Of<IHttpClientFactory>(), @@ -572,7 +688,7 @@ namespace Jellyfin.Providers.Tests.Manager _logger, Mock.Of<IFileSystem>(), Mock.Of<IServerApplicationPaths>(), - libraryManager.Object, + libraryManager, baseItemManager!, Mock.Of<ILyricManager>(), Mock.Of<IMemoryCache>(), 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; @@ -26,6 +29,45 @@ public sealed class LibraryStructureControllerTests : IClassFixture<JellyfinAppl } [Fact] + [Priority(-3)] + public async Task AddVirtualFolder_WithWarmDirectoryServiceCache_InvalidatesTheParentListing() + { + const string Name = "stale-cache-test"; + + var client = _factory.CreateClient(); + client.DefaultRequestHeaders.AddAuthHeader(_accessToken ??= await AuthHelper.CompleteStartupAsync(client)); + + var directoryService = _factory.Services.GetRequiredService<IDirectoryService>(); + var rootFolderPath = _factory.Services.GetRequiredService<IServerApplicationPaths>().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() { |