Improve recovery lifetime model

NAPS2.Lib.Common/StaticConfiguration.cs

@@ -35,10 +35,7 @@ namespace NAPS2
 
             var imageContext = kernel.Get<ImageContext>();
             var recoveryFolderPath = Path.Combine(Paths.Recovery, Path.GetRandomFileName());
-            var rsm = new RecoveryStorageManager(recoveryFolderPath);
-            imageContext.FileStorageManager = rsm;
-            imageContext.ConfigureBackingStorage<FileStorage>();
-            imageContext.ImageMetadataFactory = rsm;
+            imageContext.UseRecovery(recoveryFolderPath);
         }
     }
 }

NAPS2.Sdk.Tests/ContextualTexts.cs

@@ -10,8 +10,6 @@ namespace NAPS2.Sdk.Tests
 {
     public class ContextualTexts : IDisposable
     {
-        private RecoveryStorageManager rsm;
-
         public ContextualTexts()
         {
             FolderPath = $"naps2_test_temp/{Path.GetRandomFileName()}";
@@ -42,9 +40,6 @@ namespace NAPS2.Sdk.Tests
         public void UseRecovery()
         {
             var recoveryFolderPath = Path.Combine(FolderPath, "recovery", Path.GetRandomFileName());
-            // TODO: This is broken because it doesn't set the local rsm variable.
-            // TODO: Really just need to figure out my Dispose model, ideally this would dispose imagecontext which would dispose FSM.
-            // TODO: But there are a lot of consistency issues to think through.
             ImageContext.UseRecovery(recoveryFolderPath);
         }
 
@@ -55,7 +50,7 @@ namespace NAPS2.Sdk.Tests
 
         public virtual void Dispose()
         {
-            rsm?.ForceReleaseLock();
+            ImageContext.Dispose();
             try
             {
                 Directory.Delete(FolderPath, true);

NAPS2.Sdk.Tests/Images/ImageSerializationTests.cs

@@ -53,8 +53,8 @@ namespace NAPS2.Sdk.Tests.Images
         [Fact]
         public void SerializeImage()
         {
-            var sourceContext = new GdiImageContext().UseRecovery(Path.Combine(FolderPath, "source"));
-            var destContext = new GdiImageContext().UseRecovery(Path.Combine(FolderPath, "dest"));
+            using var sourceContext = new GdiImageContext().UseRecovery(Path.Combine(FolderPath, "source"));
+            using var destContext = new GdiImageContext().UseRecovery(Path.Combine(FolderPath, "dest"));
 
             using var _ = sourceContext.CreateScannedImage(new GdiImage(new Bitmap(100, 100))); // So sourceImage is at Index = 1
             using var sourceImage = sourceContext.CreateScannedImage(new GdiImage(new Bitmap(100, 100)));

NAPS2.Sdk/Images/ScannedImage.cs

@@ -35,6 +35,8 @@ namespace NAPS2.Images
 
                 // Delete the image data on disk
                 BackingStorage?.Dispose();
+                // Delete the recovery entry (if recovery is being used)
+                Metadata?.Dispose();
                 if (thumbnail != null)
                 {
                     thumbnail.Dispose();

NAPS2.Sdk/Images/Storage/FileStorageManager.cs

@@ -1,8 +1,9 @@
-using System.IO;
+using System;
+using System.IO;
 
 namespace NAPS2.Images.Storage
 {
-    public class FileStorageManager
+    public class FileStorageManager : IDisposable
     {
         public FileStorageManager() : this(Paths.Temp)
         {
@@ -16,5 +17,15 @@ namespace NAPS2.Images.Storage
         protected string FolderPath { get; }
 
         public virtual string NextFilePath() => Path.Combine(FolderPath, Path.GetRandomFileName());
+
+        protected virtual void Dispose(bool disposing)
+        {
+        }
+
+        public void Dispose()
+        {
+            Dispose(true);
+            GC.SuppressFinalize(this);
+        }
     }
 }

NAPS2.Sdk/Images/Storage/ImageContext.cs

@@ -8,7 +8,7 @@ using NAPS2.Util;
 
 namespace NAPS2.Images.Storage
 {
-    public abstract class ImageContext
+    public abstract class ImageContext : IDisposable
     {
         private static ImageContext _default = new GdiImageContext();
 
@@ -203,6 +203,20 @@ namespace NAPS2.Images.Storage
             ConfigureBackingStorage<FileStorage>();
             return this;
         }
+
+        protected virtual void Dispose(bool disposing)
+        {
+            if (disposing)
+            {
+                fileStorageManager.Dispose();
+            }
+        }
+
+        public void Dispose()
+        {
+            Dispose(true);
+            GC.SuppressFinalize(this);
+        }
     }
 
     public class GdiImageContext : ImageContext

NAPS2.Sdk/Images/Storage/RecoverableImageMetadata.cs

@@ -17,7 +17,7 @@ namespace NAPS2.Images.Storage
             this.rsm = rsm;
             // TODO: Maybe not a constructor param?
             this.indexImage = indexImage;
-            rsm.Index.Images.Add(indexImage);
+            rsm.RecoveryIndex.Images.Add(indexImage);
         }
 
         public List<Transform> TransformList
@@ -30,12 +30,12 @@ namespace NAPS2.Images.Storage
 
         public int Index
         {
-            get => rsm.Index.Images.IndexOf(indexImage);
+            get => rsm.RecoveryIndex.Images.IndexOf(indexImage);
             set
             {
                 // TODO: Locking
-                rsm.Index.Images.Remove(indexImage);
-                rsm.Index.Images.Insert(value, indexImage);
+                rsm.RecoveryIndex.Images.Remove(indexImage);
+                rsm.RecoveryIndex.Images.Insert(value, indexImage);
             }
         }
 
@@ -68,7 +68,7 @@ namespace NAPS2.Images.Storage
 
         public void Dispose()
         {
-            rsm.Index.Images.Remove(indexImage);
+            rsm.RecoveryIndex.Images.Remove(indexImage);
             // TODO: Commit?
         }
     }

NAPS2.Sdk/Images/Storage/RecoveryStorageManager.cs

@@ -9,80 +9,74 @@ using NAPS2.Serialization;
 
 namespace NAPS2.Images.Storage
 {
-    // TODO: Locking needs a lot of work.
+    /// <summary>
+    /// Manages the lifetime of a recovery folder.
+    ///
+    /// "Recovery" means that in the case of a crash (of the application or machine), there is enough information on disk to restore any ScannedImage objects
+    /// that weren't previously disposed.
+    ///
+    /// From a design perspective, there are several elements to recovery:
+    /// - The recovery folder. Created in the RecoveryStorageManager constructor and deleted by ImageContext.Dispose.
+    /// - The image files. Created when constructing the IFileStorage for ScannedImage and deleted by ScannedImage.Dispose.
+    /// - The image metadata entry. Created when constructing RecoverableImageMetadata and deleted by ScannedImage.Dispose.
+    /// - The recovery index. This is file named index.xml which stores the serialized metadata entries.
+    /// - The recovery lock. This is a file named .lock that is locked until RecoveryStorageManager is disposed. This is used to determine if the application is running or not (in which case you can prompt to recover from disk).
+    ///
+    /// To use recovery, follow these steps:
+    /// 1. Call ImageContext.UseRecovery with the path to the folder you want to store the recovery-related files.
+    /// 2. Scan as usual, creating ScannedImage objects.
+    /// 3. When you no longer need each ScannedImage, call its Dispose method.
+    /// 4. When you've disposed all ScannedImages and are done with scanning, call ImageContext.Dispose.
+    /// 5. If you want to deliberately keep image data to be recovered, don't call Dispose on ScannedImage or ImageContext.
+    /// 6. See the RecoveryManager doc for how to actually recover ScannedImage data from disk. 
+    /// </summary>
     public class RecoveryStorageManager : FileStorageManager, IImageMetadataFactory
     {
         public const string LOCK_FILE_NAME = ".lock";
 
         private readonly ISerializer<RecoveryIndex> serializer = new XmlSerializer<RecoveryIndex>();
+        private readonly bool shared;
+        private readonly DirectoryInfo folder;
+        private readonly FileInfo folderLockFile;
+        private readonly Stream folderLock;
 
         private int fileNumber;
-        private bool folderCreated;
-        private FileInfo folderLockFile;
-        private Stream folderLock;
-        private RecoveryIndex recoveryIndex;
+        private bool disposed;
 
-        public RecoveryStorageManager(string recoveryFolderPath, bool skipCreate = false) : base(recoveryFolderPath)
+        public RecoveryStorageManager(string recoveryFolderPath, bool shared = false) : base(recoveryFolderPath)
         {
-            folderCreated = skipCreate;
+            this.shared = shared;
+            if (!shared)
+            {
+                folder = new DirectoryInfo(RecoveryFolderPath);
+                folder.Create();
+                folderLockFile = new FileInfo(Path.Combine(RecoveryFolderPath, LOCK_FILE_NAME));
+                folderLock = folderLockFile.Open(FileMode.CreateNew, FileAccess.Write, FileShare.None);
+            }
         }
 
         public string RecoveryFolderPath => FolderPath;
 
-        public bool DisableRecoveryCleanup { get; set; }
-
-        public RecoveryIndex Index
-        {
-            get
-            {
-                EnsureFolderCreated();
-                return recoveryIndex;
-            }
-        }
+        public RecoveryIndex RecoveryIndex { get; } = new RecoveryIndex();
 
         public override string NextFilePath()
         {
             lock (this)
             {
-                EnsureFolderCreated();
+                if (disposed) throw new ObjectDisposedException(nameof(RecoveryStorageManager));
                 string fileName = $"{Process.GetCurrentProcess().Id}_{(++fileNumber).ToString("D5", CultureInfo.InvariantCulture)}";
                 return Path.Combine(RecoveryFolderPath, fileName);
             }
         }
 
-        public void EnsureFolderCreated()
-        {
-            lock (this)
-            {
-                if (!folderCreated)
-                {
-                    var folder = new DirectoryInfo(RecoveryFolderPath);
-                    folder.Create();
-                    folderLockFile = new FileInfo(Path.Combine(RecoveryFolderPath, LOCK_FILE_NAME));
-                    folderLock = folderLockFile.Open(FileMode.CreateNew, FileAccess.Write, FileShare.None);
-                    recoveryIndex = new RecoveryIndex();
-                    folderCreated = true;
-                }
-            }
-        }
-
         public void Commit()
         {
+            // TODO: Can maybe just lock on one of these, everywhere
             lock (this)
+            lock (RecoveryIndex)
             {
-                EnsureFolderCreated();
-                if (recoveryIndex.Images.Count == 0)
-                {
-                    // Clean up
-                    ForceReleaseLock();
-                    Directory.Delete(RecoveryFolderPath, true);
-                    recoveryIndex = null;
-                    folderCreated = false;
-                }
-                else
-                {
-                    serializer.SerializeToFile(Path.Combine(RecoveryFolderPath, "index.xml"), recoveryIndex);
-                }
+                if (disposed) throw new ObjectDisposedException(nameof(RecoveryStorageManager));
+                serializer.SerializeToFile(Path.Combine(RecoveryFolderPath, "index.xml"), RecoveryIndex);
             }
         }
 
@@ -92,21 +86,31 @@ namespace NAPS2.Images.Storage
             {
                 throw new ArgumentException("RecoveryStorageManager can only used with IFileStorage.");
             }
-            return new RecoverableImageMetadata(this, new RecoveryIndexImage
+            lock (this)
+            lock (RecoveryIndex)
             {
-                FileName = Path.GetFileName(fileStorage.FullPath),
-                TransformList = new List<Transform>()
-            });
+                if (disposed) throw new ObjectDisposedException(nameof(RecoveryStorageManager));
+                return new RecoverableImageMetadata(this, new RecoveryIndexImage
+                {
+                    FileName = Path.GetFileName(fileStorage.FullPath),
+                    TransformList = new List<Transform>()
+                });
+            }
         }
 
-        public void ForceReleaseLock()
+        protected override void Dispose(bool disposing)
         {
+            if (!disposing) return;
             lock (this)
+            lock (RecoveryIndex)
             {
-                folderLock?.Close();
-                folderLockFile?.Delete();
-                folderLock = null;
-                folderLockFile = null;
+                if (!shared)
+                {
+                    folderLock.Close();
+                    folderLockFile.Delete();
+                    folder.Delete(true);
+                }
+                disposed = true;
             }
         }
     }

NAPS2.Sdk/Remoting/Worker/WorkerFactory.cs

@@ -140,7 +140,6 @@ namespace NAPS2.Remoting.Worker
         public WorkerContext Create()
         {
             var rsm = imageContext.FileStorageManager as RecoveryStorageManager;
-            rsm?.EnsureFolderCreated();
             var worker = NextWorker();
             worker.Service.Init(rsm?.RecoveryFolderPath);
             return worker;

NAPS2.Sdk/Update/UpdateOperation.cs

@@ -15,6 +15,7 @@ using NAPS2.Logging;
 using NAPS2.Operation;
 using NAPS2.Images.Storage;
 using NAPS2.Util;
+using NAPS2.WinForms;
 
 namespace NAPS2.Update
 {
@@ -125,8 +126,12 @@ namespace NAPS2.Update
                 waitHandle.Set();
             }
             // TODO: Simplify
-            ((RecoveryStorageManager)imageContext.FileStorageManager).DisableRecoveryCleanup = true;
-            Application.OpenForms.OfType<Form>().FirstOrDefault()?.Close();
+            var desktop = Application.OpenForms.OfType<FDesktop>().FirstOrDefault();
+            if (desktop != null)
+            {
+                desktop.SkipRecoveryCleanup = true;
+                desktop.Close();
+            }
         }
 
         private void InstallExe()

NAPS2.Sdk/WinForms/FDesktop.cs

@@ -69,6 +69,8 @@ namespace NAPS2.WinForms
         private bool closed = false;
         private LayoutManager layoutManager;
         private bool disableSelectedIndexChangedEvent;
+        
+        public bool SkipRecoveryCleanup { get; set; }
 
         #endregion
 
@@ -404,13 +406,12 @@ namespace NAPS2.WinForms
                 }
                 else
                 {
-                    // TODO: Make nicer.
-                    ((RecoveryStorageManager)imageContext.FileStorageManager).DisableRecoveryCleanup = true;
+                    SkipRecoveryCleanup = true;
                 }
             }
             else if (changeTracker.HasUnsavedChanges)
             {
-                if (e.CloseReason == CloseReason.UserClosing && !((RecoveryStorageManager)imageContext.FileStorageManager).DisableRecoveryCleanup)
+                if (e.CloseReason == CloseReason.UserClosing && !SkipRecoveryCleanup)
                 {
                     var result = MessageBox.Show(MiscResources.ExitWithUnsavedChanges, MiscResources.UnsavedChanges,
                         MessageBoxButtons.YesNo, MessageBoxIcon.Warning, MessageBoxDefaultButton.Button2);
@@ -425,7 +426,7 @@ namespace NAPS2.WinForms
                 }
                 else
                 {
-                    ((RecoveryStorageManager)imageContext.FileStorageManager).DisableRecoveryCleanup = true;
+                    SkipRecoveryCleanup = true;
                 }
             }
 
@@ -456,7 +457,11 @@ namespace NAPS2.WinForms
         {
             SaveToolStripLocation();
             Pipes.KillServer();
-            imageList.Delete(Enumerable.Range(0, imageList.Images.Count));
+            if (!SkipRecoveryCleanup)
+            {
+                imageList.Delete(Enumerable.Range(0, imageList.Images.Count));
+                imageContext.Dispose();
+            }
             closed = true;
             renderThumbnailsWaitHandle.Set();
         }