diff --git a/backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryOnDiskTests.cs b/backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryOnDiskTests.cs new file mode 100644 index 0000000000..26ec5100fd --- /dev/null +++ b/backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryOnDiskTests.cs @@ -0,0 +1,75 @@ +using FwDataMiniLcmBridge.LcmUtils; +using FwDataMiniLcmBridge.Tests.Fixtures; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using SIL.LCModel; + +namespace FwDataMiniLcmBridge.Tests; + +public class FwDataFactoryOnDiskTests : IDisposable +{ + private static readonly TimeSpan Timeout = TimeSpan.FromSeconds(30); + private readonly ServiceProvider _services; + private readonly GatedProjectLoader _gatedLoader; + private readonly FwDataProject _project; + + public FwDataFactoryOnDiskTests() + { + _services = new ServiceCollection() + .AddTestFwDataBridge(mockProjectLoader: false) + .PostConfigure(config => config.TemplatesFolder = Path.GetFullPath("Templates")) + .AddSingleton(sp => new GatedProjectLoader(ActivatorUtilities.CreateInstance(sp))) + .AddSingleton(sp => sp.GetRequiredService()) + .BuildServiceProvider(); + _gatedLoader = _services.GetRequiredService(); + var projectsFolder = _services.GetRequiredService>().Value.ProjectsFolder; + Directory.CreateDirectory(projectsFolder); + _project = new FwDataProject($"close-releases-lock_{Guid.NewGuid()}", projectsFolder); + _gatedLoader.NewProject(_project, "en", "en").Dispose(); + } + + public void Dispose() + { + _gatedLoader.Release.Set(); + _services.Dispose(); + if (Directory.Exists(_project.ProjectFolder)) + Directory.Delete(_project.ProjectFolder, true); + } + + [Fact] + public async Task CloseDuringALoadReleasesTheLockFile() + { + var factory = _services.GetRequiredService(); + var load = Task.Run(() => factory.GetFwDataMiniLcmApi(_project, false).Cache); + _gatedLoader.Entered.Wait(Timeout).Should().BeTrue(); + var close = factory.CloseProjectAsync(_project); + _gatedLoader.Release.Set(); + await close.WaitAsync(Timeout); + // The requester may get the disposed cache or the "disposed" error, depending on how it races the close. + try { await load.WaitAsync(Timeout); } + catch (InvalidOperationException) { } + + // LCM's XML backend doesn't hold the .fwdata open; its lock is this file, deleted when the cache is disposed. + _gatedLoader.LockFileWhileLoaded.Should().BeTrue(); + File.Exists(_project.FilePath + ".lock").Should().BeFalse(); + } + + private class GatedProjectLoader(ProjectLoader inner) : IProjectLoader + { + public ManualResetEventSlim Entered { get; } = new(); + public ManualResetEventSlim Release { get; } = new(); + public bool LockFileWhileLoaded { get; private set; } + + public LcmCache LoadCache(FwDataProject project) + { + Entered.Set(); + Release.Wait(Timeout); + var lcmCache = inner.LoadCache(project); + LockFileWhileLoaded = File.Exists(project.FilePath + ".lock"); + return lcmCache; + } + + public LcmCache NewProject(FwDataProject project, string analysisWs, string vernacularWs) => + inner.NewProject(project, analysisWs, vernacularWs); + } +} diff --git a/backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryTests.cs b/backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryTests.cs new file mode 100644 index 0000000000..3930e87251 --- /dev/null +++ b/backend/FwLite/FwDataMiniLcmBridge.Tests/FwDataFactoryTests.cs @@ -0,0 +1,241 @@ +using FwDataMiniLcmBridge.LcmUtils; +using FwDataMiniLcmBridge.Tests.Fixtures; +using Microsoft.Extensions.Caching.Memory; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using SIL.LCModel; + +namespace FwDataMiniLcmBridge.Tests; + +public class FwDataFactoryTests : IDisposable +{ + private static readonly TimeSpan Timeout = TimeSpan.FromSeconds(10); + private readonly ServiceProvider _services; + private readonly FwDataFactory _factory; + private readonly MockFwProjectLoader _mockLoader; + private readonly GatedProjectLoader _gatedLoader; + private readonly IMemoryCache _memoryCache; + private readonly string _projectsFolder; + + public FwDataFactoryTests() + { + _services = new ServiceCollection() + .AddTestFwDataBridge() + .AddSingleton() + .AddSingleton(sp => sp.GetRequiredService()) + .BuildServiceProvider(); + _factory = _services.GetRequiredService(); + _mockLoader = _services.GetRequiredService(); + _gatedLoader = _services.GetRequiredService(); + _memoryCache = _services.GetRequiredService(); + _projectsFolder = _services.GetRequiredService>().Value.ProjectsFolder; + } + + public void Dispose() + { + _gatedLoader.Release.Set(); + _services.Dispose(); + } + + private FwDataProject NewProject(string name) => new($"{name}_{Guid.NewGuid()}", _projectsFolder); + + private LcmCache GetCache(FwDataProject project) => _factory.GetFwDataMiniLcmApi(project, false).Cache; + + // Starts a load and returns once it's blocked inside LoadCache. + private Task StartBlockedLoad(FwDataProject project) + { + var load = Task.Run(() => GetCache(project)); + _gatedLoader.Entered.Wait(Timeout).Should().BeTrue(); + return load; + } + + private static async Task WaitUntil(Func condition) + { + var deadline = DateTime.UtcNow + Timeout; + while (!condition()) + { + if (DateTime.UtcNow > deadline) throw new TimeoutException("Condition not met in time"); + await Task.Delay(20); + } + } + + [Fact] + public async Task ConcurrentRequestsForTheSameProjectShareOneLoad() + { + var project = NewProject("concurrent-load"); + _mockLoader.NewProject(project, "en", "en"); + + var first = StartBlockedLoad(project); + Thread? secondThread = null; + var second = Task.Run(() => + { + secondThread = Thread.CurrentThread; + return GetCache(project); + }); + // The second caller blocks either joining the first load or (if loads aren't shared) inside its own LoadCache. + await WaitUntil(() => secondThread?.ThreadState.HasFlag(ThreadState.WaitSleepJoin) == true); + _gatedLoader.Release.Set(); + var caches = await Task.WhenAll(first, second); + + _gatedLoader.LoadCount.Should().Be(1); + caches[1].Should().BeSameAs(caches[0]); + caches[0].IsDisposed.Should().BeFalse(); + } + + [Fact] + public void FailedLoadIsRetriedOnTheNextRequest() + { + _gatedLoader.Release.Set(); + var project = NewProject("failed-load"); + + var getCache = () => GetCache(project); + getCache.Should().Throw(); + _mockLoader.NewProject(project, "en", "en"); + + getCache().IsDisposed.Should().BeFalse(); + _gatedLoader.LoadCount.Should().Be(2); + } + + [Fact] + public async Task CloseWaitsForAnInFlightLoadAndDisposesIt() + { + var project = NewProject("close-during-load"); + var lcmCache = _mockLoader.NewProject(project, "en", "en"); + var load = StartBlockedLoad(project); + + var close = _factory.CloseProjectAsync(project); + await Task.WhenAny(close, Task.Delay(200)); + close.IsCompleted.Should().BeFalse(); + _gatedLoader.Release.Set(); + await close.WaitAsync(Timeout); + + lcmCache.IsDisposed.Should().BeTrue(); + await IgnoreFailure(load); + } + + [Fact] + public async Task RemovingTheCacheKeyDuringALoadDoesNotLoseTheLoad() + { + var project = NewProject("remove-during-load"); + _mockLoader.NewProject(project, "en", "en"); + var load = StartBlockedLoad(project); + + _memoryCache.Remove(FwDataFactory.CacheKey(project)); + _gatedLoader.Release.Set(); + var lcmCache = await load.WaitAsync(Timeout); + + lcmCache.IsDisposed.Should().BeFalse(); + GetCache(project).Should().BeSameAs(lcmCache); + _gatedLoader.LoadCount.Should().Be(1); + } + + [Fact] + public void ShutdownDisposesAProjectRetriedAfterAFailedLoad() + { + _gatedLoader.Release.Set(); + var project = NewProject("retry-then-shutdown"); + var lcmCache = _mockLoader.NewProject(project, "en", "en"); + _gatedLoader.FailNext = true; + var getCache = () => GetCache(project); + getCache.Should().Throw(); + getCache().Should().BeSameAs(lcmCache); + + _factory.Dispose(); + + lcmCache.IsDisposed.Should().BeTrue(); + } + + [Fact] + public async Task RequestDuringACloseGetsANewCacheAfterIt() + { + _gatedLoader.Release.Set(); + var project = NewProject("request-during-close"); + var firstCache = _mockLoader.NewProject(project, "en", "en"); + GetCache(project).Should().BeSameAs(firstCache); + var secondCache = _mockLoader.NewProject(NewProject("request-during-close-reload"), "en", "en"); + _mockLoader.Projects[project.Name] = secondCache; + + var close = _factory.CloseProjectAsync(project); + // Close takes the entry out of the cache before disposing it; request once it's gone. + await WaitUntil(() => !_memoryCache.TryGetValue(FwDataFactory.CacheKey(project), out _)); + var reloaded = GetCache(project); + await close.WaitAsync(Timeout); + + firstCache.IsDisposed.Should().BeTrue(); + reloaded.Should().BeSameAs(secondCache); + reloaded.IsDisposed.Should().BeFalse(); + _gatedLoader.LoadCount.Should().Be(2); + } + + [Fact] + public async Task CloseRightAfterRemovalDisposesTheOldCache() + { + _gatedLoader.Release.Set(); + for (var i = 0; i < 5; i++) + { + var project = NewProject("close-after-removal"); + var lcmCache = _mockLoader.NewProject(project, "en", "en"); + GetCache(project).Should().BeSameAs(lcmCache); + + // Queues the eviction callback, as expiry does, so close races it for the key lock. + _memoryCache.Remove(FwDataFactory.CacheKey(project)); + await _factory.CloseProjectAsync(project); + + lcmCache.IsDisposed.Should().BeTrue($"iteration {i}"); + } + } + + [Fact] + public void ReloadAfterRemovalDisposesTheOldCacheBeforeLoading() + { + _gatedLoader.Release.Set(); + var project = NewProject("reload-after-removal"); + var firstCache = _mockLoader.NewProject(project, "en", "en"); + GetCache(project).Should().BeSameAs(firstCache); + var secondCache = _mockLoader.NewProject(NewProject("reload-after-removal-second"), "en", "en"); + _mockLoader.Projects[project.Name] = secondCache; + bool? firstDisposedWhenLoading = null; + _gatedLoader.OnLoadCache = () => firstDisposedWhenLoading = firstCache.IsDisposed; + + _memoryCache.Remove(FwDataFactory.CacheKey(project)); + var reloaded = GetCache(project); + + firstDisposedWhenLoading.Should().BeTrue(); + reloaded.Should().BeSameAs(secondCache); + reloaded.IsDisposed.Should().BeFalse(); + } + + // The requester may get the cache or an "already disposed" error, depending on how it races the disposal. + private static async Task IgnoreFailure(Task load) + { + try { await load.WaitAsync(Timeout); } + catch (InvalidOperationException) { } + } + + private class GatedProjectLoader(MockFwProjectLoader inner) : IProjectLoader + { + private int _loadCount; + public int LoadCount => _loadCount; + public ManualResetEventSlim Entered { get; } = new(); + public ManualResetEventSlim Release { get; } = new(); + public bool FailNext { get; set; } + public Action? OnLoadCache { get; set; } + + public LcmCache LoadCache(FwDataProject project) + { + Interlocked.Increment(ref _loadCount); + OnLoadCache?.Invoke(); + if (FailNext) + { + FailNext = false; + throw new InvalidOperationException("Simulated load failure"); + } + Entered.Set(); + Release.Wait(Timeout); + return inner.LoadCache(project); + } + + public LcmCache NewProject(FwDataProject project, string analysisWs, string vernacularWs) => + inner.NewProject(project, analysisWs, vernacularWs); + } +} diff --git a/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs b/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs index 8dff6914bc..f8e46ae090 100644 --- a/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs +++ b/backend/FwLite/FwDataMiniLcmBridge/FwDataFactory.cs @@ -1,4 +1,4 @@ -using System.Diagnostics; +using System.Collections.Concurrent; using FwDataMiniLcmBridge.Api; using FwDataMiniLcmBridge.LcmUtils; using FwDataMiniLcmBridge.Media; @@ -40,79 +40,114 @@ public FwDataFactory(ILogger fwdataLogger, }); } - private static string CacheKey(FwDataProject project) => $"{nameof(FwDataFactory)}|{project.FilePath}"; - private static string FilePathFromCacheKey(string cacheKey) => cacheKey.Split('|')[1]; + internal static string CacheKey(FwDataProject project) => $"{nameof(FwDataFactory)}|{project.FilePath}"; public FwDataMiniLcmApi GetFwDataMiniLcmApi(FwDataProject project, bool saveOnDispose) { return new FwDataMiniLcmApi(new(() => GetProjectServiceCached(project)), saveOnDispose, fwdataLogger, project, mediaAdapter, config); } - private HashSet _projectCacheKeys = []; + // One lock per project, held for the whole load, so concurrent requests share a load instead of racing + // (IMemoryCache.GetOrCreate isn't atomic) and the cache only ever holds a finished LcmCache. + // Never pruned: it grows by one small Lock per distinct project path this factory opens. + private readonly ConcurrentDictionary _keyLocks = new(); + private Lock KeyLock(string key) => _keyLocks.GetOrAdd(key, _ => new()); + + private readonly Lock _openCachesLock = new(); + // Whoever removes an instance from here disposes it, so eviction, close and shutdown dispose it exactly once. + // Keyed by instance so a stale eviction callback can't untrack a newer cache for the same project. + private Dictionary _openCaches = []; + + private static string FilePathFromCacheKey(string cacheKey) => cacheKey.Split('|')[1]; + private LcmCache GetProjectServiceCached(FwDataProject project) { var key = CacheKey(project); - var projectService = cache.GetOrCreate(key, - entry => - { - entry.SlidingExpiration = CacheSlidingExpiration; - entry.RegisterPostEvictionCallback(OnLcmProjectCacheEviction, (logger, _projectCacheKeys)); - logger.LogInformation("Loading project {ProjectFileName}", project.FileName); - var projectService = projectLoader.LoadCache(project); - logger.LogInformation("Project {ProjectFileName} loaded", project.FileName); - _projectCacheKeys.Add(key); - return projectService; - }); - if (projectService is null) + if (cache.TryGetValue(key, out LcmCache? hit) && !hit!.IsDisposed) return hit; + lock (KeyLock(key)) { - throw new InvalidOperationException("Project service is null"); + if (cache.TryGetValue(key, out hit) && !hit!.IsDisposed) return hit; + DisposeTrackedCaches(key); + logger.LogInformation("Loading project {ProjectFileName}", project.FileName); + var lcmCache = projectLoader.LoadCache(project); + logger.LogInformation("Project {ProjectFileName} loaded", project.FileName); + lock (_openCachesLock) + { + _openCaches.Add(lcmCache, key); + } + // The entry is committed to the cache when it's disposed. + using (var entry = cache.CreateEntry(key)) + { + entry.SlidingExpiration = CacheSlidingExpiration; + entry.RegisterPostEvictionCallback(OnLcmProjectCacheEviction); + entry.Value = lcmCache; + } + return lcmCache; } - if (projectService.IsDisposed) + } + + private bool Untrack(LcmCache lcmCache) + { + lock (_openCachesLock) { - throw new InvalidOperationException("Project service is disposed"); + return _openCaches.Remove(lcmCache); } + } - return projectService; + // Call with KeyLock(key) held: an expired entry's eviction callback may not have run yet. + private void DisposeTrackedCaches(string key) + { + List untracked; + lock (_openCachesLock) + { + untracked = _openCaches.Where(open => open.Value == key).Select(open => open.Key).ToList(); + foreach (var lcmCache in untracked) _openCaches.Remove(lcmCache); + } + foreach (var lcmCache in untracked) + { + if (lcmCache.IsDisposed) continue; + lcmCache.Dispose(); + logger.LogInformation("FW Data Project {ProjectFileName} disposed", FilePathFromCacheKey(key)); + } } - private static void OnLcmProjectCacheEviction(object keyObj, object? value, EvictionReason reason, object? state) + private void OnLcmProjectCacheEviction(object keyObj, object? value, EvictionReason reason, object? state) { - if (value is null) return; + if (value is not LcmCache lcmCache) return; // todo this could trigger when the service is still referenced elsewhere, for example in a long running task. // disposing of the service while it's still in use would be bad. // one way around this would be to return a lease object, only after a timeout and no more references to the lease object would the service be disposed. - var lcmCache = (LcmCache)value; - var (logger, projectCacheKeys) = ((ILogger, HashSet))state!; - if (keyObj.ToString() is not string key) - { - Debug.Fail($"Eviction callback called with null key {keyObj}"); - logger.LogError("Eviction callback called with null key {Key}", keyObj); - return; - } + var key = (string)keyObj; var filePath = FilePathFromCacheKey(key); - logger.LogInformation("Evicting project {ProjectFileName} from cache", filePath); - projectCacheKeys.Remove(key); - if (!lcmCache.IsDisposed) + logger.LogInformation("Evicting project {ProjectFileName} from cache ({EvictionReason})", filePath, reason); + lock (KeyLock(key)) { + if (!Untrack(lcmCache) || lcmCache.IsDisposed) return; lcmCache.Dispose(); - logger.LogInformation("FW Data Project {ProjectFileName} disposed", filePath); - GC.Collect(); } + logger.LogInformation("FW Data Project {ProjectFileName} disposed", filePath); + GC.Collect(); } public void Dispose() { logger.LogInformation("Closing all projects"); - //ensure a race condition doesn't cause us to dispose of a project that's already been disposed - var projectCacheKeys = Interlocked.Exchange(ref _projectCacheKeys, []); - foreach (var key in projectCacheKeys) + Dictionary openCaches; + lock (_openCachesLock) { - var lcmCache = cache.Get(key); - if (lcmCache is null || lcmCache.IsDisposed) continue; - var filePath = FilePathFromCacheKey(key); - lcmCache.Dispose(); //need to explicitly call dispose as that blocks, just removing from the cache does not block, meaning it will not finish disposing before the program exits. - logger.LogInformation("FW Data Project {ProjectFileName} disposed", filePath); - cache.Remove(key); + openCaches = _openCaches; + _openCaches = []; + } + foreach (var (lcmCache, key) in openCaches) + { + if (!lcmCache.IsDisposed) + { + //need to explicitly call dispose as that blocks, just removing from the cache does not block, meaning it will not finish disposing before the program exits. + lcmCache.Dispose(); + logger.LogInformation("FW Data Project {ProjectFileName} disposed", FilePathFromCacheKey(key)); + } + if (cache.TryGetValue(key, out LcmCache? current) && ReferenceEquals(current, lcmCache)) + cache.Remove(key); } } @@ -121,13 +156,23 @@ public async Task CloseProjectAsync(FwDataProject project) // if we are shutting down, don't do anything because we want project dispose to be called as part of the shutdown process. if (_shuttingDown) return; logger.LogInformation("Explicitly Closing project {ProjectFileName}", project.FilePath); - var cacheKey = CacheKey(project); - var lcmCache = cache.Get(cacheKey); - if (lcmCache is null) return; - // Dispose cache immediately so file locks are released before we return. + var key = CacheKey(project); + // Dispose immediately (waiting for a load still in flight) so file locks are released before we return. // The caller assumes the project is ready to be opened somewhere else (e.g. in FieldWorks) - if (!lcmCache.IsDisposed) await Task.Run(lcmCache.Dispose); - cache.Remove(cacheKey); + await Task.Run(() => + { + lock (KeyLock(key)) + { + if (cache.TryGetValue(key, out LcmCache? lcmCache) && lcmCache is not null) + { + // Untracked first, so the eviction callback that Remove triggers leaves the disposal to us. + var owned = Untrack(lcmCache); + cache.Remove(key); + if (owned && !lcmCache.IsDisposed) lcmCache.Dispose(); + } + DisposeTrackedCaches(key); + } + }); } public IAsyncDisposable DeferCloseAsync(FwDataProject project)