From a9b906870ee8d098756067f91c0ba3cb76179644 Mon Sep 17 00:00:00 2001 From: "DESKTOP-382ETUR\\Hiroshi Tsugawa" Date: Thu, 15 Oct 2026 21:46:10 +0900 Subject: [PATCH 1/2] Dispose a model's items even when its collection is cleared first With an application running, DisposableCollection.Dispose queues the disposal of its items on the dispatcher (#373), and DisposableModelBase clears the collection as soon as that call returns (#639). The queued callback then enumerated an empty collection, so a DisposableModelBase disposed nothing while MS-DIAL ran. A file model's .dcl reader, for one, stayed open after its dataset closed, until a finalizer closed it. DisposableCollection now copies the items before it queues their disposal. LcmsAnalysisModelWithoutRawTests failed in Debug because it never disposed the file model that owns the .dcl reader, so Cleanup could not delete sample.dcl. In Release the JIT let the file model die early and the test's own GC.Collect finalized the stream. The test now disposes the file model as the dataset would, waits for the dispatcher, and checks that neither the .dcl nor the .pai2 is open before any collection. A new AnalysisFileBeanModelTests case checks that ReleaseMSDecLoader and Dispose close the .dcl while an application runs. Co-Authored-By: Claude Opus 5.5 --- .../CommonMVVM/Common/DisposableCollection.cs | 6 ++- .../DataObj/AnalysisFileBeanModelTests.cs | 27 ++++++++++++- .../Lcms/LcmsAnalysisModelWithoutRawTests.cs | 40 +++++++++---------- .../Utility/TestApplication.cs | 38 ++++++++++++++++++ .../MsdialGuiAppTests/Utility/TestFiles.cs | 20 ++++++++++ 5 files changed, 107 insertions(+), 24 deletions(-) create mode 100644 tests/MSDIAL5/MsdialGuiAppTests/Utility/TestApplication.cs create mode 100644 tests/MSDIAL5/MsdialGuiAppTests/Utility/TestFiles.cs diff --git a/src/Common/CommonMVVM/Common/DisposableCollection.cs b/src/Common/CommonMVVM/Common/DisposableCollection.cs index 2ac116e62..4134621f0 100644 --- a/src/Common/CommonMVVM/Common/DisposableCollection.cs +++ b/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); diff --git a/tests/MSDIAL5/MsdialGuiAppTests/Model/DataObj/AnalysisFileBeanModelTests.cs b/tests/MSDIAL5/MsdialGuiAppTests/Model/DataObj/AnalysisFileBeanModelTests.cs index 371251dac..c1b670cb2 100644 --- a/tests/MSDIAL5/MsdialGuiAppTests/Model/DataObj/AnalysisFileBeanModelTests.cs +++ b/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 { 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; diff --git a/tests/MSDIAL5/MsdialGuiAppTests/Model/Lcms/LcmsAnalysisModelWithoutRawTests.cs b/tests/MSDIAL5/MsdialGuiAppTests/Model/Lcms/LcmsAnalysisModelWithoutRawTests.cs index d490285fd..a8b315497 100644 --- a/tests/MSDIAL5/MsdialGuiAppTests/Model/Lcms/LcmsAnalysisModelWithoutRawTests.cs +++ b/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; @@ -123,6 +111,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(); diff --git a/tests/MSDIAL5/MsdialGuiAppTests/Utility/TestApplication.cs b/tests/MSDIAL5/MsdialGuiAppTests/Utility/TestApplication.cs new file mode 100644 index 000000000..56c572c65 --- /dev/null +++ b/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; + +/// +/// 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. +/// +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(); + } + + /// + /// 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. + /// + public static void WaitForBackgroundWork() { + Application.Current?.Dispatcher.Invoke(() => { }, DispatcherPriority.ContextIdle); + } +} diff --git a/tests/MSDIAL5/MsdialGuiAppTests/Utility/TestFiles.cs b/tests/MSDIAL5/MsdialGuiAppTests/Utility/TestFiles.cs new file mode 100644 index 000000000..ee564fac2 --- /dev/null +++ b/tests/MSDIAL5/MsdialGuiAppTests/Utility/TestFiles.cs @@ -0,0 +1,20 @@ +using System.IO; + +namespace CompMs.App.Msdial.Utility.Tests; + +internal static class TestFiles +{ + /// + /// Whether any handle on the file is open, in this process or another: exclusive access fails while one is. + /// A missing file still throws. + /// + 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; + } + } +} From 0df2b426c065317fafdf8809d2ef27df6775f1e9 Mon Sep 17 00:00:00 2001 From: "DESKTOP-382ETUR\\Hiroshi Tsugawa" Date: Fri, 16 Oct 2026 08:45:43 +0900 Subject: [PATCH 2/2] Bound opening without raw data by one retry wait, not by two seconds The test guards against the old freeze, in which reading the missing raw data waited 5 s before each of its retries (BaseDataProvider). Two seconds also failed a cold CI runner that took 2.8 s to open the sample without any retry (PR #835's PR Check). Below 5 s still fails any retry. Co-Authored-By: Claude Opus 5.5 --- .../Model/Lcms/LcmsAnalysisModelWithoutRawTests.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/MSDIAL5/MsdialGuiAppTests/Model/Lcms/LcmsAnalysisModelWithoutRawTests.cs b/tests/MSDIAL5/MsdialGuiAppTests/Model/Lcms/LcmsAnalysisModelWithoutRawTests.cs index a8b315497..9a988b580 100644 --- a/tests/MSDIAL5/MsdialGuiAppTests/Model/Lcms/LcmsAnalysisModelWithoutRawTests.cs +++ b/tests/MSDIAL5/MsdialGuiAppTests/Model/Lcms/LcmsAnalysisModelWithoutRawTests.cs @@ -95,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);