Dispose a model's items even when its collection is cleared first

src/Common/CommonMVVM/Common/DisposableCollection.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.ObjectModel;
+using System.Linq;
 using System.Windows;
 
 namespace CompMs.CommonMVVM.Common
@@ -33,9 +34,12 @@ protected virtual void Dispose(bool disposing) {
                         }
                     }
                     else {
+                        // The items are taken now: DisposableModelBase clears the collection as soon as this returns,
+                        // before the dispatcher runs the disposal, and a clear while it runs would break the enumeration.
+                        var disposables = this.ToArray();
                         Application.Current.Dispatcher.InvokeAsync(() =>
                         {
-                            foreach (var disposable in this) {
+                            foreach (var disposable in disposables) {
                                 disposable?.Dispose();
                             }
                         }, System.Windows.Threading.DispatcherPriority.Background);

tests/MSDIAL5/MsdialGuiAppTests/Model/DataObj/AnalysisFileBeanModelTests.cs

@@ -1,4 +1,7 @@
-using Microsoft.VisualStudio.TestTools.UnitTesting;
+using CompMs.App.Msdial.Utility.Tests;
+using CompMs.MsdialCore.MSDec;
+using CompMs.MsdialCore.Parser;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
 using System;
 using System.Collections.Generic;
 using System.IO;
@@ -93,6 +96,28 @@ public void PeakListFileExistsFollowsTheFile() {
             Assert.IsFalse(model.PeakListFileExists());
         }
 
+        [TestMethod()]
+        public void ReleasingTheLoaderOrDisposingTheModelClosesTheDeconvolutionFile() {
+            // With an application running, as in MS-DIAL, the model's disposables are disposed on its dispatcher.
+            TestApplication.Ensure();
+            var dcl = Path.Combine(_folder, "sample.dcl");
+            MsdecResultsWriter.Write(dcl, new List<MSDecResult> { new MSDecResult { ScanID = 0, PrecursorMz = 300.1, }, });
+            var model = AnalysisFileBeanModelTestHelper.Create();
+            model.File.DeconvolutionFilePath = dcl;
+
+            // Reprocessing rewrites the .dcl after DatasetModel releases the loaders.
+            Assert.IsNotNull(model.MSDecLoader.LoadMSDecResult(0));
+            Assert.IsTrue(TestFiles.IsOpen(dcl));
+            model.ReleaseMSDecLoader();
+            Assert.IsFalse(TestFiles.IsOpen(dcl));
+
+            // Closing the dataset disposes its file models, and the loader opened again goes with them.
+            Assert.IsNotNull(model.MSDecLoader.LoadMSDecResult(0));
+            model.Dispose();
+            TestApplication.WaitForBackgroundWork();
+            Assert.IsFalse(TestFiles.IsOpen(dcl));
+        }
+
         private static AnalysisFileBeanModel Create(string analysisFilePath) {
             var model = AnalysisFileBeanModelTestHelper.Create();
             model.AnalysisFilePath = analysisFilePath;

tests/MSDIAL5/MsdialGuiAppTests/Model/Lcms/LcmsAnalysisModelWithoutRawTests.cs

@@ -2,6 +2,7 @@
 using CompMs.App.Msdial.Model.DataObj.Tests;
 using CompMs.App.Msdial.Model.Search;
 using CompMs.App.Msdial.Model.Service;
+using CompMs.App.Msdial.Utility.Tests;
 using CompMs.Common.Components;
 using CompMs.Common.DataObj.Result;
 using CompMs.Common.Enum;
@@ -25,42 +26,29 @@ namespace CompMs.App.Msdial.Model.Lcms.Tests
     public class LcmsAnalysisModelWithoutRawTests
     {
         private string _folder = string.Empty;
+        private AnalysisFileBeanModel? _file;
 
         [TestInitialize()]
         public void Setup() {
             _folder = Path.Combine(Path.GetTempPath(), $"msdial-lcms-without-raw-{Guid.NewGuid():N}");
             Directory.CreateDirectory(_folder);
-            EnsureApplication();
-        }
-
-        // The peak spot filters are attached through Application.Current.Dispatcher, so the model needs an application
-        // whose dispatcher runs; it gets one on its own STA thread, as the application's UI thread would be.
-        private static void EnsureApplication() {
-            if (System.Windows.Application.Current is not null) {
-                return;
-            }
-            using var ready = new System.Threading.ManualResetEventSlim();
-            var thread = new System.Threading.Thread(() => {
-                _ = new System.Windows.Application { ShutdownMode = System.Windows.ShutdownMode.OnExplicitShutdown, };
-                ready.Set();
-                System.Windows.Threading.Dispatcher.Run();
-            })
-            {
-                IsBackground = true,
-            };
-            thread.SetApartmentState(System.Threading.ApartmentState.STA);
-            thread.Start();
-            ready.Wait();
+            // The peak spot filters are attached through Application.Current.Dispatcher, so the model needs an application
+            // whose dispatcher runs.
+            TestApplication.Ensure();
         }
 
         [TestCleanup()]
         public void Cleanup() {
+            // The file model owns the .dcl reader that opening the sample creates, as the dataset's file collection
+            // does in the application, so the folder can be deleted only after it is disposed.
+            _file?.Dispose();
+            TestApplication.WaitForBackgroundWork();
             Directory.Delete(_folder, recursive: true);
         }
 
         [TestMethod()]
         public void ASampleWithoutRawDataOpensFromWhatTheProjectStores() {
-            var file = AnalysisFileBeanModelTestHelper.Create();
+            var file = _file = AnalysisFileBeanModelTestHelper.Create();
             file.AnalysisFileName = "sample";
             file.AnalysisFilePath = Path.Combine(_folder, "sample.mzML"); // not written: the raw data are missing
             file.AcquisitionType = AcquisitionType.DDA;
@@ -107,7 +95,9 @@ public void ASampleWithoutRawDataOpensFromWhatTheProjectStores() {
                     new MsfinderSearcherFactory(dataBases, mapper, parameter, "MS-FINDER"),
                     new MessageBroker())) {
                     stopwatch.Stop();
-                    Assert.IsTrue(stopwatch.Elapsed < TimeSpan.FromSeconds(2), $"Opening took {stopwatch.Elapsed}.");
+                    // Reading the missing raw data would wait 5 s before its first retry (BaseDataProvider), so any retry
+                    // fails this; a cold CI runner has taken 2.8 s to open the sample without one.
+                    Assert.IsTrue(stopwatch.Elapsed < TimeSpan.FromSeconds(5), $"Opening took {stopwatch.Elapsed}.");
 
                     Assert.IsFalse(model.IsRawDataAvailable);
                     Assert.IsNull(model.EicLoader);
@@ -123,6 +113,14 @@ public void ASampleWithoutRawDataOpensFromWhatTheProjectStores() {
                     Assert.AreEqual(RawDataGuard.EicTitle, model.EicModel.GraphTitle);
                     Assert.AreEqual(RawDataGuard.Ms1SpectrumTitle, model.SurveyScanModel.Elements.GraphTitle);
                 }
+                // The model borrows the .dcl reader from the file model, which keeps it for the next time the sample is
+                // opened. Once the file model is disposed too, as closing the dataset does, nothing the project stores
+                // may stay open. This is checked before any collection, so no finalizer closes a leaked stream first.
+                file.Dispose();
+                TestApplication.WaitForBackgroundWork();
+                Assert.IsFalse(TestFiles.IsOpen(file.File.DeconvolutionFilePath), "The .dcl is still open.");
+                Assert.IsFalse(TestFiles.IsOpen(file.PeakListFilePath), "The .pai2 is still open.");
+
                 GC.Collect();
                 GC.WaitForPendingFinalizers();
                 GC.Collect();

tests/MSDIAL5/MsdialGuiAppTests/Utility/TestApplication.cs

@@ -0,0 +1,38 @@
+using System.Threading;
+using System.Windows;
+using System.Windows.Threading;
+
+namespace CompMs.App.Msdial.Utility.Tests;
+
+/// <summary>
+/// A WPF application whose dispatcher runs on its own STA thread, as the application's UI thread would be.
+/// An AppDomain holds one application at most, so once a test makes it, every later test in the run has it too.
+/// </summary>
+internal static class TestApplication
+{
+    public static void Ensure() {
+        if (Application.Current is not null) {
+            return;
+        }
+        using var ready = new ManualResetEventSlim();
+        var thread = new Thread(() => {
+            _ = new Application { ShutdownMode = ShutdownMode.OnExplicitShutdown, };
+            ready.Set();
+            Dispatcher.Run();
+        })
+        {
+            IsBackground = true,
+        };
+        thread.SetApartmentState(ApartmentState.STA);
+        thread.Start();
+        ready.Wait();
+    }
+
+    /// <summary>
+    /// Returns once the dispatcher has run everything queued at Background priority or above, which is where
+    /// DisposableCollection disposes its items while an application runs. Call it from a thread other than the dispatcher's.
+    /// </summary>
+    public static void WaitForBackgroundWork() {
+        Application.Current?.Dispatcher.Invoke(() => { }, DispatcherPriority.ContextIdle);
+    }
+}

tests/MSDIAL5/MsdialGuiAppTests/Utility/TestFiles.cs

@@ -0,0 +1,20 @@
+using System.IO;
+
+namespace CompMs.App.Msdial.Utility.Tests;
+
+internal static class TestFiles
+{
+    /// <summary>
+    /// Whether any handle on the file is open, in this process or another: exclusive access fails while one is.
+    /// A missing file still throws.
+    /// </summary>
+    public static bool IsOpen(string path) {
+        try {
+            using (File.Open(path, FileMode.Open, FileAccess.ReadWrite, FileShare.None)) { }
+            return false;
+        }
+        catch (IOException) when (File.Exists(path)) {
+            return true;
+        }
+    }
+}