diff options
12 files changed, 576 insertions, 81 deletions
diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 99f24e3a14..a97d335170 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -240,6 +240,7 @@ - [Florin-Popescu](https://github.com/Florin-Popescu) - [m0g3r](https://github.com/m0g3r) - [martin-77](https://github.com/martin-77) + - [Oggeb1](https://github.com/Oggeb1) # Emby Contributors diff --git a/Emby.Server.Implementations/EntryPoints/LibraryChangedNotifier.cs b/Emby.Server.Implementations/EntryPoints/LibraryChangedNotifier.cs index 933cfc8cbe..02b104756e 100644 --- a/Emby.Server.Implementations/EntryPoints/LibraryChangedNotifier.cs +++ b/Emby.Server.Implementations/EntryPoints/LibraryChangedNotifier.cs @@ -27,6 +27,11 @@ namespace Emby.Server.Implementations.EntryPoints; /// </summary> public sealed class LibraryChangedNotifier : IHostedService, IDisposable { + // A batch holds a live reference to every item it names, so it has to stay small enough that a + // library scan - which changes items faster than any batch window closes - cannot grow it without + // bound. Reached only by a scan; interactive use closes a batch on the window long before this. + internal const int MaxBatchSize = 2000; + private readonly ILibraryManager _libraryManager; private readonly IServerConfigurationManager _configurationManager; private readonly IProviderManager _providerManager; @@ -35,11 +40,11 @@ public sealed class LibraryChangedNotifier : IHostedService, IDisposable private readonly ILogger<LibraryChangedNotifier> _logger; private readonly Lock _libraryChangedSyncLock = new(); - private readonly List<Folder> _foldersAddedTo = new(); - private readonly List<Folder> _foldersRemovedFrom = new(); - private readonly List<BaseItem> _itemsAdded = new(); - private readonly List<BaseItem> _itemsRemoved = new(); - private readonly List<BaseItem> _itemsUpdated = new(); + private readonly Dictionary<Guid, Folder> _foldersAddedTo = []; + private readonly Dictionary<Guid, Folder> _foldersRemovedFrom = []; + private readonly Dictionary<Guid, BaseItem> _itemsAdded = []; + private readonly Dictionary<Guid, BaseItem> _itemsRemoved = []; + private readonly Dictionary<Guid, BaseItem> _itemsUpdated = []; private readonly ConcurrentDictionary<Guid, DateTime> _lastProgressMessageTimes = new(); private Timer? _libraryUpdateTimer; @@ -173,7 +178,7 @@ public sealed class LibraryChangedNotifier : IHostedService, IDisposable private void OnLibraryItemRemoved(object? sender, ItemChangeEventArgs e) => OnLibraryChange(e.Item, e.Parent, _itemsRemoved, _foldersRemovedFrom); - private void OnLibraryChange(BaseItem item, BaseItem parent, List<BaseItem> itemsList, List<Folder>? foldersList) + private void OnLibraryChange(BaseItem item, BaseItem parent, Dictionary<Guid, BaseItem> itemsList, Dictionary<Guid, Folder>? foldersList) { if (!FilterItem(item)) { @@ -182,23 +187,28 @@ public sealed class LibraryChangedNotifier : IHostedService, IDisposable lock (_libraryChangedSyncLock) { - var updateDuration = TimeSpan.FromSeconds(_configurationManager.Configuration.LibraryUpdateDuration); - + // The window runs from the first change of a batch and is never extended. Extending it on + // every change would keep a library scan's batch open for the whole scan, and the batch + // holds the items it names alive, so it would grow to the size of the library. if (_libraryUpdateTimer is null) { + var updateDuration = TimeSpan.FromSeconds(_configurationManager.Configuration.LibraryUpdateDuration); _libraryUpdateTimer = new Timer(LibraryUpdateTimerCallback, null, updateDuration, Timeout.InfiniteTimeSpan); } - else - { - _libraryUpdateTimer.Change(updateDuration, Timeout.InfiniteTimeSpan); - } if (foldersList is not null && parent is Folder folder) { - foldersList.Add(folder); + foldersList[folder.Id] = folder; } - itemsList.Add(item); + itemsList[item.Id] = item; + + // A window long enough to cover a burst still has to give way once the batch is large + // enough to be worth sending on its own. + if (_itemsAdded.Count + _itemsRemoved.Count + _itemsUpdated.Count >= MaxBatchSize) + { + _libraryUpdateTimer.Change(TimeSpan.Zero, Timeout.InfiniteTimeSpan); + } } } @@ -211,22 +221,16 @@ public sealed class LibraryChangedNotifier : IHostedService, IDisposable List<BaseItem> itemsRemoved; lock (_libraryChangedSyncLock) { - // Remove dupes in case some were saved multiple times - foldersAddedTo = _foldersAddedTo - .DistinctBy(x => x.Id) - .ToList(); - - foldersRemovedFrom = _foldersRemovedFrom - .DistinctBy(x => x.Id) - .ToList(); + foldersAddedTo = _foldersAddedTo.Values.ToList(); + foldersRemovedFrom = _foldersRemovedFrom.Values.ToList(); itemsUpdated = _itemsUpdated - .Where(i => !_itemsAdded.Contains(i)) - .DistinctBy(x => x.Id) + .Where(e => !_itemsAdded.ContainsKey(e.Key)) + .Select(e => e.Value) .ToList(); - itemsAdded = _itemsAdded.ToList(); - itemsRemoved = _itemsRemoved.ToList(); + itemsAdded = _itemsAdded.Values.ToList(); + itemsRemoved = _itemsRemoved.Values.ToList(); if (_libraryUpdateTimer is not null) { @@ -241,6 +245,15 @@ public sealed class LibraryChangedNotifier : IHostedService, IDisposable _foldersRemovedFrom.Clear(); } + if (itemsAdded.Count == 0 + && itemsUpdated.Count == 0 + && itemsRemoved.Count == 0 + && foldersAddedTo.Count == 0 + && foldersRemovedFrom.Count == 0) + { + return; + } + await SendChangeNotifications(itemsAdded, itemsUpdated, itemsRemoved, foldersAddedTo, foldersRemovedFrom, CancellationToken.None).ConfigureAwait(false); } diff --git a/Emby.Server.Implementations/EntryPoints/UserDataChangeNotifier.cs b/Emby.Server.Implementations/EntryPoints/UserDataChangeNotifier.cs index fc174b7c14..b182e5837b 100644 --- a/Emby.Server.Implementations/EntryPoints/UserDataChangeNotifier.cs +++ b/Emby.Server.Implementations/EntryPoints/UserDataChangeNotifier.cs @@ -18,15 +18,17 @@ namespace Emby.Server.Implementations.EntryPoints public sealed class UserDataChangeNotifier : IHostedService, IDisposable { private const int UpdateDuration = 500; + internal const int MaxBatchSize = 2000; private readonly ISessionManager _sessionManager; private readonly IUserDataManager _userDataManager; private readonly IUserManager _userManager; - private readonly Dictionary<Guid, List<BaseItem>> _changedItems = new(); + private readonly Dictionary<Guid, Dictionary<Guid, BaseItem>> _changedItems = []; private readonly Lock _syncLock = new(); private Timer? _updateTimer; + private int _changedItemCount; /// <summary> /// Initializes a new instance of the <see cref="UserDataChangeNotifier"/> class. @@ -69,50 +71,64 @@ namespace Emby.Server.Implementations.EntryPoints lock (_syncLock) { - if (_updateTimer is null) + // The window runs from the first change of a batch and is never extended, so a stream + // of changes that never pauses - a library scan - still closes its batches instead of + // holding every item it touched alive until the stream stops. + _updateTimer ??= new Timer( + UpdateTimerCallback, + null, + UpdateDuration, + Timeout.Infinite); + + if (!_changedItems.TryGetValue(e.UserId, out Dictionary<Guid, BaseItem>? keys)) { - _updateTimer = new Timer( - UpdateTimerCallback, - null, - UpdateDuration, - Timeout.Infinite); - } - else - { - _updateTimer.Change(UpdateDuration, Timeout.Infinite); - } - - if (!_changedItems.TryGetValue(e.UserId, out List<BaseItem>? keys)) - { - keys = new List<BaseItem>(); + keys = []; _changedItems[e.UserId] = keys; } - keys.Add(e.Item); - var baseItem = e.Item; // Go up one level for indicators if (baseItem is not null) { + Track(keys, baseItem); + var parent = baseItem.GetOwner() ?? baseItem.GetParent(); if (parent is not null) { - keys.Add(parent); + Track(keys, parent); } } + + // A window long enough to cover a burst still has to give way once the batch is + // large enough to be worth sending on its own. + if (_changedItemCount >= MaxBatchSize) + { + _updateTimer.Change(0, Timeout.Infinite); + } + } + } + + private void Track(Dictionary<Guid, BaseItem> keys, BaseItem item) + { + var before = keys.Count; + keys[item.Id] = item; + + if (keys.Count != before) + { + _changedItemCount++; } } private async void UpdateTimerCallback(object? state) { - List<KeyValuePair<Guid, List<BaseItem>>> changes; + List<KeyValuePair<Guid, Dictionary<Guid, BaseItem>>> changes; lock (_syncLock) { - // Remove dupes in case some were saved multiple times changes = _changedItems.ToList(); _changedItems.Clear(); + _changedItemCount = 0; if (_updateTimer is not null) { @@ -121,17 +137,22 @@ namespace Emby.Server.Implementations.EntryPoints } } + if (changes.Count == 0) + { + return; + } + foreach (var (userId, changedItems) in changes) { await _sessionManager.SendMessageToUserSessions( [userId], SessionMessageType.UserDataChanged, - () => GetUserDataChangeInfo(userId, changedItems), + () => GetUserDataChangeInfo(userId, changedItems.Values), default).ConfigureAwait(false); } } - private UserDataChangeInfo GetUserDataChangeInfo(Guid userId, List<BaseItem> changedItems) + private UserDataChangeInfo GetUserDataChangeInfo(Guid userId, IEnumerable<BaseItem> changedItems) { var user = _userManager.GetUserById(userId) ?? throw new ArgumentException("Invalid user ID", nameof(userId)); @@ -140,7 +161,6 @@ namespace Emby.Server.Implementations.EntryPoints { UserId = userId, UserDataList = changedItems - .DistinctBy(x => x.Id) .Select(i => { var dto = _userDataManager.GetUserDataDto(i, user); diff --git a/Jellyfin.Server.Implementations/Item/ItemPersistenceService.cs b/Jellyfin.Server.Implementations/Item/ItemPersistenceService.cs index efff3457a3..c8672e189b 100644 --- a/Jellyfin.Server.Implementations/Item/ItemPersistenceService.cs +++ b/Jellyfin.Server.Implementations/Item/ItemPersistenceService.cs @@ -176,14 +176,6 @@ public class ItemPersistenceService : IItemPersistenceService var context = await _dbProvider.CreateDbContextAsync(cancellationToken).ConfigureAwait(false); await using (context.ConfigureAwait(false)) { - if (!await context.BaseItems - .AnyAsync(bi => bi.Id == item.Id, cancellationToken) - .ConfigureAwait(false)) - { - _logger.LogWarning("Unable to save ImageInfo for non existing BaseItem"); - return; - } - await context.BaseItemImageInfos .Where(e => e.ItemId == item.Id) .ExecuteDeleteAsync(cancellationToken) @@ -193,7 +185,26 @@ public class ItemPersistenceService : IItemPersistenceService .AddRangeAsync(images, cancellationToken) .ConfigureAwait(false); - await context.SaveChangesAsync(cancellationToken).ConfigureAwait(false); + try + { + await context.SaveChangesAsync(cancellationToken).ConfigureAwait(false); + } + catch (DbUpdateException) + { + // Checking that the item exists before writing leaves a gap a scan can delete it + // through, turning the insert into a foreign key violation that fails the whole + // refresh instead of the no-op intended here. Let the insert be the check: it is the + // only point at which the answer cannot go stale. Nothing is orphaned by the delete + // above, because deleting the item cascades to its images anyway. + if (await context.BaseItems + .AnyAsync(bi => bi.Id == item.Id, cancellationToken) + .ConfigureAwait(false)) + { + throw; + } + + _logger.LogWarning("Unable to save ImageInfo for non existing BaseItem {ItemId}", item.Id); + } } } diff --git a/MediaBrowser.Controller/MediaEncoding/EncodingHelper.cs b/MediaBrowser.Controller/MediaEncoding/EncodingHelper.cs index 274e82d823..9952c10c3a 100644 --- a/MediaBrowser.Controller/MediaEncoding/EncodingHelper.cs +++ b/MediaBrowser.Controller/MediaEncoding/EncodingHelper.cs @@ -6315,7 +6315,7 @@ namespace MediaBrowser.Controller.MediaEncoding string.Join(',', overlayFilters)); var mapPrefix = Convert.ToInt32(state.SubtitleStream.IsExternal); - var subtitleStreamIndex = FindIndex(state.MediaSource.MediaStreams, state.SubtitleStream); + var subtitleStreamIndex = GetSubtitleStreamIndexForFfmpeg(state.MediaSource, state.SubtitleStream); var videoStreamIndex = FindIndex(state.MediaSource.MediaStreams, state.VideoStream); if (hasSubs) @@ -7947,6 +7947,24 @@ namespace MediaBrowser.Controller.MediaEncoding return -1; } + public static int GetSubtitleStreamIndexForFfmpeg(MediaSourceInfo mediaSource, MediaStream subtitleStream) + { + var index = FindIndex(mediaSource.MediaStreams, subtitleStream); + if (index == -1 || subtitleStream.IsExternal || mediaSource.VideoType != VideoType.BluRay) + { + return index; + } + + var hiddenStreamsBefore = mediaSource.MediaStreams.Count(s => + s.Type == MediaStreamType.Audio + && !s.IsExternal + && (string.Equals(s.Codec, "truehd", StringComparison.OrdinalIgnoreCase) + || string.Equals(s.Codec, "atmos", StringComparison.OrdinalIgnoreCase)) + && s.Index < subtitleStream.Index); + + return index + hiddenStreamsBefore; + } + public static bool IsCopyCodec(string codec) { return string.Equals(codec, "copy", StringComparison.OrdinalIgnoreCase); diff --git a/MediaBrowser.MediaEncoding/Encoder/MediaEncoder.cs b/MediaBrowser.MediaEncoding/Encoder/MediaEncoder.cs index 0ddd378352..f64fd73763 100644 --- a/MediaBrowser.MediaEncoding/Encoder/MediaEncoder.cs +++ b/MediaBrowser.MediaEncoding/Encoder/MediaEncoder.cs @@ -1152,6 +1152,11 @@ namespace MediaBrowser.MediaEncoding.Encoder { process.Process.PriorityClass = ProcessPriorityClass.BelowNormal; } + catch (InvalidOperationException) + { + // The process finished before its priority could be lowered. That says nothing + // about whether the platform allows it, so keep the capability for the next one. + } catch (Exception ex) { _canSetProcessPriority = false; @@ -1361,12 +1366,20 @@ namespace MediaBrowser.MediaEncoding.Encoder return _configurationManager.GetEncodingOptions().EnableSubtitleExtraction; } - private sealed class ProcessWrapper : IDisposable + internal sealed class ProcessWrapper : IDisposable { private readonly MediaEncoder _mediaEncoder; + // The exit event is raised on the thread pool, so it writes the state below while the + // caller that started the process is reading it. + private readonly Lock _exitLock = new(); + private bool _disposed = false; + private bool _hasExited; + + private int? _exitCode; + public ProcessWrapper(Process process, MediaEncoder mediaEncoder) { Process = process; @@ -1376,49 +1389,84 @@ namespace MediaBrowser.MediaEncoding.Encoder public Process Process { get; } - public bool HasExited { get; private set; } + // The exit event can lag behind the wait that returned, so ask the process rather than + // report one that has exited as still running. + public bool HasExited => ReadExitState().HasExited; + + // As above: rather than report no exit code for a process that has one. + public int? ExitCode => ReadExitState().ExitCode; - public int? ExitCode { get; private set; } + private (bool HasExited, int? ExitCode) ReadExitState() + { + lock (_exitLock) + { + if (!_hasExited && !_disposed) + { + try + { + if (Process.HasExited) + { + _hasExited = true; + _exitCode = Process.ExitCode; + } + } + catch (InvalidOperationException) + { + // No process is associated with this object, or it was disposed from + // under us - ObjectDisposedException derives from this one. + } + } + + return (_hasExited, _exitCode); + } + } private void OnProcessExited(object sender, EventArgs e) { var process = (Process)sender; - HasExited = true; - - try - { - ExitCode = process.ExitCode; - } - catch + lock (_exitLock) { + _hasExited = true; + + try + { + _exitCode = process.ExitCode; + } + catch + { + } } - DisposeProcess(process); + // Only stop tracking it. The caller that started the process still holds it to read + // its output and its exit code, so disposing it here handed whoever was quickest to + // exit - an ffprobe on a file it rejects outright - an ObjectDisposedException. + Untrack(); } - private void DisposeProcess(Process process) + private void Untrack() { lock (_mediaEncoder._runningProcessesLock) { _mediaEncoder._runningProcesses.Remove(this); } - - process.Dispose(); } public void Dispose() { - if (!_disposed) + lock (_exitLock) { - if (Process is not null) + if (_disposed) { - Process.Exited -= OnProcessExited; - DisposeProcess(Process); + return; } + + _disposed = true; } - _disposed = true; + Process.Exited -= OnProcessExited; + Untrack(); + Process.Dispose(); } } } diff --git a/MediaBrowser.MediaEncoding/Subtitles/SubtitleEncoder.cs b/MediaBrowser.MediaEncoding/Subtitles/SubtitleEncoder.cs index e8c636e7fb..fba644b74a 100644 --- a/MediaBrowser.MediaEncoding/Subtitles/SubtitleEncoder.cs +++ b/MediaBrowser.MediaEncoding/Subtitles/SubtitleEncoder.cs @@ -649,7 +649,7 @@ namespace MediaBrowser.MediaEncoding.Subtitles List<MediaStream> subtitleStreams, CancellationToken cancellationToken) { - var inputPath = _mediaEncoder.GetInputArgument(mediaSource.Path, mediaSource); + var inputPath = _mediaEncoder.GetInputPathArgument(mediaSource.Path, mediaSource); var outputPaths = new List<string>(); var args = string.Format( CultureInfo.InvariantCulture, @@ -673,7 +673,7 @@ namespace MediaBrowser.MediaEncoding.Subtitles var outputCodec = IsCodecCopyable(subtitleStream.Codec) ? "copy" : "srt"; // FFmpeg does not provide an .idx/.sub muxer, so VobSub streams must be written as MKS files. var outputFormatOption = MediaStream.IsVobSubFormat(subtitleStream.Codec) ? " -f matroska" : string.Empty; - var streamIndex = EncodingHelper.FindIndex(mediaSource.MediaStreams, subtitleStream); + var streamIndex = EncodingHelper.GetSubtitleStreamIndexForFfmpeg(mediaSource, subtitleStream); if (streamIndex == -1) { diff --git a/tests/Jellyfin.MediaEncoding.Tests/Encoder/ProcessWrapperTests.cs b/tests/Jellyfin.MediaEncoding.Tests/Encoder/ProcessWrapperTests.cs new file mode 100644 index 0000000000..141164815c --- /dev/null +++ b/tests/Jellyfin.MediaEncoding.Tests/Encoder/ProcessWrapperTests.cs @@ -0,0 +1,103 @@ +using System; +using System.Diagnostics; +using System.Threading; +using System.Threading.Tasks; +using MediaBrowser.Controller.Configuration; +using MediaBrowser.MediaEncoding.Encoder; +using MediaBrowser.Model.Globalization; +using MediaBrowser.Model.IO; +using MediaBrowser.Model.MediaInfo; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Logging; +using Moq; +using Xunit; + +namespace Jellyfin.MediaEncoding.Tests.Encoder; + +public class ProcessWrapperTests +{ + [Fact] + public async Task ExitedProcess_StaysUsableForTheCallerThatStartedIt() + { + using var process = CreateProcess(); + using var exitHandled = new ManualResetEventSlim(false); + + using (var wrapper = new MediaEncoder.ProcessWrapper(process, CreateEncoder())) + { + // Subscribed after the wrapper, so by the time this is set the wrapper's own handler has + // already run: whatever it does to the process has happened. + process.Exited += (_, _) => exitHandled.Set(); + + process.Start(); + await process.WaitForExitAsync(TestContext.Current.CancellationToken).ConfigureAwait(true); + + Assert.True(exitHandled.Wait(TimeSpan.FromSeconds(15), TestContext.Current.CancellationToken), "The process never raised Exited."); + + // The caller still owns the process here. Disposing it from the exit handler handed + // whoever exited quickest an ObjectDisposedException out of these three lines. + var output = await process.StandardOutput.ReadToEndAsync(TestContext.Current.CancellationToken).ConfigureAwait(true); + Assert.Equal("jellyfin", output.Trim()); + + Assert.True(wrapper.HasExited); + Assert.Equal(3, wrapper.ExitCode); + } + } + + [Fact] + public async Task ExitState_IsReadableBeforeTheExitEventArrives() + { + using var process = CreateProcess(); + + using (var wrapper = new MediaEncoder.ProcessWrapper(process, CreateEncoder())) + { + process.Start(); + await process.WaitForExitAsync(TestContext.Current.CancellationToken).ConfigureAwait(true); + + // The exit event is raised on the thread pool and can lag behind the wait that just + // returned, so neither of these may depend on it having arrived. + Assert.True(wrapper.HasExited); + Assert.Equal(3, wrapper.ExitCode); + } + } + + [Fact] + public async Task ExitCode_SurvivesDisposal() + { + using var process = CreateProcess(); + var wrapper = new MediaEncoder.ProcessWrapper(process, CreateEncoder()); + + process.Start(); + await process.WaitForExitAsync(TestContext.Current.CancellationToken).ConfigureAwait(true); + + var exitCode = wrapper.ExitCode; + wrapper.Dispose(); + + Assert.Equal(exitCode, wrapper.ExitCode); + Assert.True(wrapper.HasExited); + } + + private static MediaEncoder CreateEncoder() + => new( + Mock.Of<ILogger<MediaEncoder>>(), + Mock.Of<IServerConfigurationManager>(), + Mock.Of<IFileSystem>(), + Mock.Of<IBlurayExaminer>(), + Mock.Of<ILocalizationManager>(), + new ConfigurationBuilder().Build(), + Mock.Of<IServerConfigurationManager>()); + + // Writes to stdout and exits immediately with a non-zero code, standing in for the ffprobe that + // rejects a file outright - the process that used to win the race against its own caller. + private static Process CreateProcess() + { + var startInfo = OperatingSystem.IsWindows() + ? new ProcessStartInfo("cmd.exe", "/c echo jellyfin & exit 3") + : new ProcessStartInfo("/bin/sh", "-c \"printf 'jellyfin\\n'; exit 3\""); + + startInfo.CreateNoWindow = true; + startInfo.UseShellExecute = false; + startInfo.RedirectStandardOutput = true; + + return new Process { StartInfo = startInfo, EnableRaisingEvents = true }; + } +} diff --git a/tests/Jellyfin.Server.Implementations.Tests/EntryPoints/LibraryChangedNotifierTests.cs b/tests/Jellyfin.Server.Implementations.Tests/EntryPoints/LibraryChangedNotifierTests.cs new file mode 100644 index 0000000000..cdb261de8d --- /dev/null +++ b/tests/Jellyfin.Server.Implementations.Tests/EntryPoints/LibraryChangedNotifierTests.cs @@ -0,0 +1,123 @@ +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.Threading; +using System.Threading.Tasks; +using Emby.Server.Implementations.EntryPoints; +using MediaBrowser.Controller.Configuration; +using MediaBrowser.Controller.Entities; +using MediaBrowser.Controller.Library; +using MediaBrowser.Controller.Providers; +using MediaBrowser.Controller.Session; +using MediaBrowser.Model.Configuration; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; +using Xunit; + +namespace Jellyfin.Server.Implementations.Tests.EntryPoints; + +public class LibraryChangedNotifierTests +{ + // How long a test waits for the notifier's timer callback to run. Generous: the assertions are + // about a batch being sent at all, not about how promptly. + private static readonly TimeSpan _flushTimeout = TimeSpan.FromSeconds(15); + + private readonly Mock<ILibraryManager> _libraryManager = new(); + private readonly Mock<IServerConfigurationManager> _configurationManager = new(); + private readonly Mock<ISessionManager> _sessionManager = new(); + private readonly Mock<IUserManager> _userManager = new(); + private readonly Mock<IProviderManager> _providerManager = new(); + private readonly ServerConfiguration _configuration = new(); + + private int _flushCount; + + public LibraryChangedNotifierTests() + { + _configurationManager.SetupGet(e => e.Configuration).Returns(_configuration); + + // Reading the session list is the first thing a flush does, so it stands in for "a batch was + // sent" without having to mock a whole user library behind it. + _sessionManager.SetupGet(e => e.Sessions) + .Returns(() => + { + Interlocked.Increment(ref _flushCount); + return []; + }); + } + + [Fact] + public async Task OnLibraryItemUpdated_BatchSizeCapReached_SendsWithoutWaitingForWindow() + { + // Long enough that only the size cap can close the batch. + _configuration.LibraryUpdateDuration = 3600; + + var notifier = CreateNotifier(); + await notifier.StartAsync(TestContext.Current.CancellationToken); + + for (var i = 0; i < LibraryChangedNotifier.MaxBatchSize; i++) + { + RaiseItemUpdated(); + } + + Assert.True(await WaitForFlushAsync(1), "The batch was not sent once it hit the size cap."); + + await notifier.StopAsync(TestContext.Current.CancellationToken); + notifier.Dispose(); + } + + [Fact] + public async Task OnLibraryItemUpdated_ChangesNeverPause_StillSendsOnTheWindow() + { + // A scan changes items continuously. The window must run from the first change of a batch, or + // the batch never closes and holds every item it named alive for the length of the scan. + _configuration.LibraryUpdateDuration = 1; + + var notifier = CreateNotifier(); + await notifier.StartAsync(TestContext.Current.CancellationToken); + + var stopwatch = Stopwatch.StartNew(); + while (stopwatch.Elapsed < _flushTimeout && Volatile.Read(ref _flushCount) == 0) + { + // Well below the window, and well below the size cap over the whole loop. + RaiseItemUpdated(); + await Task.Delay(25, TestContext.Current.CancellationToken); + } + + Assert.True(Volatile.Read(ref _flushCount) > 0, "The batch was never sent while changes kept arriving."); + + await notifier.StopAsync(TestContext.Current.CancellationToken); + notifier.Dispose(); + } + + private LibraryChangedNotifier CreateNotifier() + => new( + _libraryManager.Object, + _configurationManager.Object, + _sessionManager.Object, + _userManager.Object, + NullLogger<LibraryChangedNotifier>.Instance, + _providerManager.Object); + + // A folder passes the notifier's item filter without needing any of BaseItem's static services. + private void RaiseItemUpdated() + => _libraryManager.Raise( + e => e.ItemUpdated += null, + _libraryManager.Object, + new ItemChangeEventArgs { Item = new Folder { Id = Guid.NewGuid() } }); + + private async Task<bool> WaitForFlushAsync(int expected) + { + var stopwatch = Stopwatch.StartNew(); + while (stopwatch.Elapsed < _flushTimeout) + { + if (Volatile.Read(ref _flushCount) >= expected) + { + return true; + } + + await Task.Delay(25, TestContext.Current.CancellationToken); + } + + return false; + } +} diff --git a/tests/Jellyfin.Server.Implementations.Tests/EntryPoints/UserDataChangeNotifierTests.cs b/tests/Jellyfin.Server.Implementations.Tests/EntryPoints/UserDataChangeNotifierTests.cs new file mode 100644 index 0000000000..0274398f89 --- /dev/null +++ b/tests/Jellyfin.Server.Implementations.Tests/EntryPoints/UserDataChangeNotifierTests.cs @@ -0,0 +1,78 @@ +using System; +using System.Diagnostics; +using System.Threading; +using System.Threading.Tasks; +using Emby.Server.Implementations.EntryPoints; +using MediaBrowser.Controller.Entities; +using MediaBrowser.Controller.Library; +using MediaBrowser.Controller.Session; +using MediaBrowser.Model.Entities; +using MediaBrowser.Model.Session; +using Moq; +using Xunit; + +namespace Jellyfin.Server.Implementations.Tests.EntryPoints; + +public class UserDataChangeNotifierTests +{ + // How long a test waits for the notifier's timer callback to run. Generous: the assertions are + // about a batch being sent at all, not about how promptly. + private static readonly TimeSpan _flushTimeout = TimeSpan.FromSeconds(15); + + private readonly Mock<IUserDataManager> _userDataManager = new(); + private readonly Mock<ISessionManager> _sessionManager = new(); + private readonly Mock<IUserManager> _userManager = new(); + + private int _flushCount; + + public UserDataChangeNotifierTests() + { + _sessionManager + .Setup(e => e.SendMessageToUserSessions( + It.IsAny<System.Collections.Generic.List<Guid>>(), + SessionMessageType.UserDataChanged, + It.IsAny<Func<UserDataChangeInfo>>(), + It.IsAny<CancellationToken>())) + .Callback(() => Interlocked.Increment(ref _flushCount)) + .Returns(Task.CompletedTask); + } + + [Fact] + public async Task OnUserDataSaved_ChangesNeverPause_StillSendsOnTheWindow() + { + // A scan changes user data continuously. The window must run from the first change of a batch, + // or the batch never closes and holds every item it named alive for the length of the scan. + var notifier = CreateNotifier(); + await notifier.StartAsync(TestContext.Current.CancellationToken); + + var userId = Guid.NewGuid(); + var stopwatch = Stopwatch.StartNew(); + while (stopwatch.Elapsed < _flushTimeout && Volatile.Read(ref _flushCount) == 0) + { + // Well below the window, and well below the size cap over the whole loop. + RaiseUserDataSaved(userId); + await Task.Delay(25, TestContext.Current.CancellationToken); + } + + Assert.True(Volatile.Read(ref _flushCount) > 0, "The batch was never sent while changes kept arriving."); + + await notifier.StopAsync(TestContext.Current.CancellationToken); + notifier.Dispose(); + } + + private UserDataChangeNotifier CreateNotifier() + => new(_userDataManager.Object, _sessionManager.Object, _userManager.Object); + + // A folder needs none of BaseItem's static services, and PlaybackProgress is the one reason the + // notifier ignores outright. + private void RaiseUserDataSaved(Guid userId) + => _userDataManager.Raise( + e => e.UserDataSaved += null, + _userDataManager.Object, + new UserDataSaveEventArgs + { + UserId = userId, + SaveReason = UserDataSaveReason.UpdateUserRating, + Item = new Folder { Id = Guid.NewGuid() } + }); +} diff --git a/tests/Jellyfin.Server.Implementations.Tests/Item/ItemPersistenceServiceSaveImagesTests.cs b/tests/Jellyfin.Server.Implementations.Tests/Item/ItemPersistenceServiceSaveImagesTests.cs new file mode 100644 index 0000000000..7997c6d771 --- /dev/null +++ b/tests/Jellyfin.Server.Implementations.Tests/Item/ItemPersistenceServiceSaveImagesTests.cs @@ -0,0 +1,77 @@ +using System; +using System.Linq; +using System.Threading.Tasks; +using Jellyfin.Database.Implementations.Entities; +using Jellyfin.Server.Implementations.Item; +using MediaBrowser.Controller; +using MediaBrowser.Controller.Entities; +using MediaBrowser.Model.Entities; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; +using Xunit; + +namespace Jellyfin.Server.Implementations.Tests.Item; + +public class ItemPersistenceServiceSaveImagesTests : SqliteDbTestFixture +{ + private readonly ItemPersistenceService _service; + + public ItemPersistenceServiceSaveImagesTests() + { + _service = new ItemPersistenceService( + CreateDbContextFactory(), + Mock.Of<IServerApplicationHost>(), + NullLogger<ItemPersistenceService>.Instance); + } + + [Fact] + public async Task SaveImagesAsync_ReplacesThePreviousImages() + { + var itemId = Guid.NewGuid(); + Seed(itemId); + + await _service.SaveImagesAsync(CreateItem(itemId, "/first.jpg"), TestContext.Current.CancellationToken); + await _service.SaveImagesAsync(CreateItem(itemId, "/second.jpg"), TestContext.Current.CancellationToken); + + using var context = CreateDbContext(); + var paths = context.BaseItemImageInfos + .Where(e => e.ItemId.Equals(itemId)) + .Select(e => e.Path) + .ToList(); + + Assert.Equal(["/second.jpg"], paths); + } + + [Fact] + public async Task SaveImagesAsync_ItemDeletedFromUnderIt_IsANoOp() + { + // A scan can delete the item between the refresh reading it and the images being written. That + // must not fail the whole refresh, and must not leave the images of an item that is gone. + var itemId = Guid.NewGuid(); + + await _service.SaveImagesAsync(CreateItem(itemId, "/gone.jpg"), TestContext.Current.CancellationToken); + + using var context = CreateDbContext(); + Assert.Empty(context.BaseItemImageInfos.Where(e => e.ItemId.Equals(itemId))); + } + + private static BaseItem CreateItem(Guid itemId, string imagePath) + => new Folder + { + Id = itemId, + ImageInfos = [new ItemImageInfo { Path = imagePath, Type = ImageType.Primary }] + }; + + private void Seed(Guid itemId) + { + using var context = CreateDbContext(); + context.BaseItems.Add(new BaseItemEntity + { + Id = itemId, + Type = "Folder", + IsFolder = true + }); + context.SaveChanges(); + } +} diff --git a/tests/Jellyfin.Server.Implementations.Tests/Item/SqliteDbTestFixture.cs b/tests/Jellyfin.Server.Implementations.Tests/Item/SqliteDbTestFixture.cs index 87efa8fea5..cfc9c9496c 100644 --- a/tests/Jellyfin.Server.Implementations.Tests/Item/SqliteDbTestFixture.cs +++ b/tests/Jellyfin.Server.Implementations.Tests/Item/SqliteDbTestFixture.cs @@ -1,4 +1,5 @@ using System; +using System.Threading; using Emby.Server.Implementations.Data; using Jellyfin.Database.Implementations; using Jellyfin.Database.Implementations.Locking; @@ -58,6 +59,8 @@ public abstract class SqliteDbTestFixture : IDisposable { var factory = new Mock<IDbContextFactory<JellyfinDbContext>>(); factory.Setup(f => f.CreateDbContext()).Returns(CreateDbContext); + factory.Setup(f => f.CreateDbContextAsync(It.IsAny<CancellationToken>())) + .ReturnsAsync(CreateDbContext); return factory.Object; } |