From 95dff9bc16234ecde02a9e5b0ec8e8223675bda2 Mon Sep 17 00:00:00 2001 From: Ger Hobbelt Date: Fri, 1 Nov 2019 04:05:11 +0100 Subject: [PATCH] fix b0rk introduced by commit SHA-1: bcd73cd877b72cd2b9aba9183172dd6c46590880 :: we don't do a *revert* action per se, but rather improve upon the patch we picked up there from the experimental branch: as it turns out, the patch caused a lot of trouble which has been resolved to allow the running background task(s) access to a reduced clone of the WebLibraryDetails, which does not exhibit the cyclic dependency that the original WebLibraryDetails instance incorporated, thus breaking the cyclic reference and allowing the .NET GC to do its job on the Library instance(s) ASAP. As this problem was discovered while doing work on commit SHA-1: ed2cb589a2e3562102163c4b3129310c4850e33a, these files also include the remainder of the work done for that commit, as it was important to separate out the patches which fixed the cyclic memory reference. --- Qiqqa/DocumentLibrary/Library.cs | 223 ++++++++---------- .../WebLibraryStuff/WebLibraryDetail.cs | 52 ++-- 2 files changed, 125 insertions(+), 150 deletions(-) diff --git a/Qiqqa/DocumentLibrary/Library.cs b/Qiqqa/DocumentLibrary/Library.cs index 74e3498d0..a5332afa2 100644 --- a/Qiqqa/DocumentLibrary/Library.cs +++ b/Qiqqa/DocumentLibrary/Library.cs @@ -1,8 +1,6 @@ using System; using System.Collections.Generic; using System.Diagnostics; -using System.IO; -using System.Linq; using Qiqqa.Common.Configuration; using Qiqqa.Common.TagManagement; using Qiqqa.DocumentConversionStuff; @@ -18,113 +16,53 @@ using Utilities.Files; using Utilities.GUI; using Utilities.Misc; -using Utilities.Strings; -using File = Alphaleonis.Win32.Filesystem.File; using Directory = Alphaleonis.Win32.Filesystem.Directory; +using File = Alphaleonis.Win32.Filesystem.File; using Path = Alphaleonis.Win32.Filesystem.Path; + namespace Qiqqa.DocumentLibrary { public class Library : IDisposable { public override string ToString() { - return String.Format("Library: {0}", this.web_library_detail.Title); + return String.Format("Library: {0}", web_library_detail.Title); } /// /// Use this singleton instance ONLY for testing purposes! /// - public static Library GuestInstance - { - get - { - return WebLibraryManager.Instance.Library_Guest; - } - } + public static Library GuestInstance => WebLibraryManager.Instance.Library_Guest; private WebLibraryDetail web_library_detail; - public WebLibraryDetail WebLibraryDetail - { - get - { - return web_library_detail; - } - } + public WebLibraryDetail WebLibraryDetail => web_library_detail; private LibraryDB library_db; - public LibraryDB LibraryDB - { - get - { - return library_db; - } - } + public LibraryDB LibraryDB => library_db; private FolderWatcherManager folder_watcher_manager; - public FolderWatcherManager FolderWatcherManager - { - get - { - return folder_watcher_manager; - } - } + public FolderWatcherManager FolderWatcherManager => folder_watcher_manager; private LibraryIndex library_index; - public LibraryIndex LibraryIndex - { - get - { - return library_index; - } - } + public LibraryIndex LibraryIndex => library_index; private AITagManager ai_tag_manager; - public AITagManager AITagManager - { - get - { - return ai_tag_manager; - } - } + public AITagManager AITagManager => ai_tag_manager; private RecentlyReadManager recently_read_manager; - public RecentlyReadManager RecentlyReadManager - { - get - { - return recently_read_manager; - } - } + public RecentlyReadManager RecentlyReadManager => recently_read_manager; private BlackWhiteListManager blackwhite_list_manager; - public BlackWhiteListManager BlackWhiteListManager - { - get - { - return blackwhite_list_manager; - } - } + public BlackWhiteListManager BlackWhiteListManager => blackwhite_list_manager; - PasswordManager password_manager; - public PasswordManager PasswordManager - { - get - { - return password_manager; - } - } + private PasswordManager password_manager; + public PasswordManager PasswordManager => password_manager; - ExpeditionManager expedition_manager; - public ExpeditionManager ExpeditionManager - { - get - { - return expedition_manager; - } - } + private ExpeditionManager expedition_manager; + public ExpeditionManager ExpeditionManager => expedition_manager; - Dictionary pdf_documents = new Dictionary(); + private Dictionary pdf_documents = new Dictionary(); private object pdf_documents_lock = new object(); public delegate void OnLibraryLoadedHandler(Library library); @@ -214,14 +152,14 @@ public Library(WebLibraryDetail web_library_detail) Directory.CreateDirectory(LIBRARY_BASE_PATH); Directory.CreateDirectory(LIBRARY_DOCUMENTS_BASE_PATH); - this.library_db = new LibraryDB(this.LIBRARY_BASE_PATH); - this.folder_watcher_manager = new FolderWatcherManager(this); - this.library_index = new LibraryIndex(this); - this.ai_tag_manager = new AITagManager(this); - this.recently_read_manager = new RecentlyReadManager(this); - this.blackwhite_list_manager = new BlackWhiteListManager(this); - this.password_manager = new PasswordManager(this); - this.expedition_manager = new ExpeditionManager(this); + library_db = new LibraryDB(LIBRARY_BASE_PATH); + folder_watcher_manager = new FolderWatcherManager(this); + library_index = new LibraryIndex(this); + ai_tag_manager = new AITagManager(this); + recently_read_manager = new RecentlyReadManager(this); + blackwhite_list_manager = new BlackWhiteListManager(this); + password_manager = new PasswordManager(this); + expedition_manager = new ExpeditionManager(this); // Start loading the documents in the background... SafeThreadPool.QueueUserWorkItem(o => BuildFromDocumentRepository()); @@ -231,30 +169,51 @@ public Library(WebLibraryDetail web_library_detail) // // Once completed, an event will be fired to // help the main application update any relevant views. - void BuildFromDocumentRepository() + private void BuildFromDocumentRepository() { try { LibraryIsLoaded = false; + // abort work when this library instance has already been Dispose()d in the main UI thread: + if (LibraryIsKilled) + { + Logging.Info("Building the library has been SKIPPED/ABORTED as the library {0} has already been killed.", WebLibraryDetail.Id); + return; + } + Stopwatch clk = Stopwatch.StartNew(); long prev_clk = 0; long elapsed = 0; Logging.Debug特("+Build library from repository"); - List library_items = this.library_db.GetLibraryItems(null, PDFDocumentFileLocations.METADATA); + List library_items = library_db.GetLibraryItems(null, PDFDocumentFileLocations.METADATA); + + // abort work when this library instance has already been Dispose()d in the main UI thread: + if (LibraryIsKilled) + { + Logging.Info("Building the library has been SKIPPED/ABORTED as the library {0} has already been killed.", WebLibraryDetail.Id); + return; + } elapsed = clk.ElapsedMilliseconds; - Logging.Debug特(":Build library '{2}' from repository -- time spent: {0} ms on fetching {1} records from SQLite DB.", elapsed, library_items.Count, this.WebLibraryDetail.DescriptiveTitle); + Logging.Debug特(":Build library '{2}' from repository -- time spent: {0} ms on fetching {1} records from SQLite DB.", elapsed, library_items.Count, WebLibraryDetail.DescriptiveTitle); prev_clk = elapsed; // Get the annotations cache - Dictionary library_items_annotations_cache = this.library_db.GetLibraryItemsAsCache(PDFDocumentFileLocations.ANNOTATIONS); + Dictionary library_items_annotations_cache = library_db.GetLibraryItemsAsCache(PDFDocumentFileLocations.ANNOTATIONS); + + // abort work when this library instance has already been Dispose()d in the main UI thread: + if (LibraryIsKilled) + { + Logging.Info("Building the library has been SKIPPED/ABORTED as the library {0} has already been killed.", WebLibraryDetail.Id); + return; + } elapsed = clk.ElapsedMilliseconds; - Logging.Debug特(":Build library '{2}' from repository -- time spent: {0} ms on fetching annotation cache for {1} records.", elapsed - prev_clk, library_items.Count, this.WebLibraryDetail.DescriptiveTitle); + Logging.Debug特(":Build library '{2}' from repository -- time spent: {0} ms on fetching annotation cache for {1} records.", elapsed - prev_clk, library_items.Count, WebLibraryDetail.DescriptiveTitle); prev_clk = elapsed; - Logging.Info("Library '{2}': Loading {0} files from repository at {1}", library_items.Count, LIBRARY_DOCUMENTS_BASE_PATH, this.WebLibraryDetail.DescriptiveTitle); + Logging.Info("Library '{2}': Loading {0} files from repository at {1}", library_items.Count, LIBRARY_DOCUMENTS_BASE_PATH, WebLibraryDetail.DescriptiveTitle); for (int i = 0; i < library_items.Count; ++i) { @@ -264,8 +223,8 @@ void BuildFromDocumentRepository() elapsed = clk.ElapsedMilliseconds; if (prev_clk + 1000 <= elapsed) { - StatusManager.Instance.UpdateStatus("LibraryInitialLoad", String.Format("Loading your library '{0}'", this.WebLibraryDetail.DescriptiveTitle), i, library_items.Count); - Logging.Info("Library '{2}': Loaded {0}/{1} documents", i, library_items.Count, this.WebLibraryDetail.DescriptiveTitle); + StatusManager.Instance.UpdateStatus("LibraryInitialLoad", String.Format("Loading your library '{0}'", WebLibraryDetail.DescriptiveTitle), i, library_items.Count); + Logging.Info("Library '{2}': Loaded {0}/{1} documents", i, library_items.Count, WebLibraryDetail.DescriptiveTitle); prev_clk = elapsed; System.Threading.Thread.Yield(); @@ -273,6 +232,8 @@ void BuildFromDocumentRepository() if (LibraryIsKilled) { + // abort work when this library instance has already been Dispose()d in the main UI thread: + Logging.Info("Building the library has been SKIPPED/ABORTED as the library {0} has already been killed.", WebLibraryDetail.Id); break; } @@ -282,23 +243,23 @@ void BuildFromDocumentRepository() } catch (Exception ex) { - Logging.Error(ex, "Library '{1}': There was a problem loading document {0}", library_item, this.WebLibraryDetail.DescriptiveTitle); + Logging.Error(ex, "Library '{1}': There was a problem loading document {0}", library_item, WebLibraryDetail.DescriptiveTitle); } } StatusManager.Instance.ClearStatus("LibraryInitialLoad"); - Logging.Debug特("-Build library '{2}' from repository -- time spent: {0} ms on {1} library records.", clk.ElapsedMilliseconds, library_items.Count, this.WebLibraryDetail.DescriptiveTitle); + Logging.Debug特("-Build library '{2}' from repository -- time spent: {0} ms on {1} library records.", clk.ElapsedMilliseconds, library_items.Count, WebLibraryDetail.DescriptiveTitle); } catch (Exception ex) { if (LibraryIsKilled) { - Logging.Warn(ex, "There was a failure while building the *KILLED* document library instance for library {0} ({1})", this.WebLibraryDetail.DescriptiveTitle, this.WebLibraryDetail.Id); + Logging.Warn(ex, "There was a failure while building the *KILLED* document library instance for library {0} ({1})", WebLibraryDetail.DescriptiveTitle, WebLibraryDetail.Id); } else { - Logging.Error(ex, "There was a problem while building the document library {0} ({1})", this.WebLibraryDetail.DescriptiveTitle, this.WebLibraryDetail.Id); + Logging.Error(ex, "There was a problem while building the document library {0} ({1})", WebLibraryDetail.DescriptiveTitle, WebLibraryDetail.Id); } } finally @@ -932,8 +893,8 @@ internal void NotifyLibraryThatDocumentHasChangedExternally(string fingerprint) Logging.Info("Library has been notified that {0} has changed", fingerprint); try { - LibraryDB.LibraryItem library_item = this.library_db.GetLibraryItem(fingerprint, PDFDocumentFileLocations.METADATA); - this.LoadDocumentFromMetadata(library_item, null, true); + LibraryDB.LibraryItem library_item = library_db.GetLibraryItem(fingerprint, PDFDocumentFileLocations.METADATA); + LoadDocumentFromMetadata(library_item, null, true); } catch (Exception ex) { @@ -954,7 +915,7 @@ public class PDFDocumentEventArgs : EventArgs { public PDFDocumentEventArgs(PDFDocument pdf_document) { - this.PDFDocument = pdf_document; + PDFDocument = pdf_document; } public PDFDocument PDFDocument { get; } @@ -1043,13 +1004,7 @@ public static string GetLibraryBasePathForId(string id) return Path.GetFullPath(Path.Combine(ConfigurationManager.Instance.BaseDirectoryForQiqqa, id)); } - public string LIBRARY_BASE_PATH - { - get - { - return GetLibraryBasePathForId(this.web_library_detail.Id); - } - } + public string LIBRARY_BASE_PATH => GetLibraryBasePathForId(web_library_detail.Id); public string LIBRARY_DOCUMENTS_BASE_PATH { @@ -1067,13 +1022,7 @@ public string LIBRARY_DOCUMENTS_BASE_PATH } } - public string LIBRARY_INDEX_BASE_PATH - { - get - { - return Path.GetFullPath(Path.Combine(LIBRARY_BASE_PATH, @"index")); - } - } + public string LIBRARY_INDEX_BASE_PATH => Path.GetFullPath(Path.Combine(LIBRARY_BASE_PATH, @"index")); #endregion @@ -1106,8 +1055,8 @@ protected virtual void Dispose(bool disposing) // Do we need to check that the library has finished being loaded? // Switch off the living things - this.library_index?.Dispose(); - this.folder_watcher_manager?.Dispose(); + library_index?.Dispose(); + folder_watcher_manager?.Dispose(); // NULL the memory database Utilities.LockPerfTimer l2_clk = Utilities.LockPerfChecker.Start(); @@ -1120,21 +1069,41 @@ protected virtual void Dispose(bool disposing) } // Clear the references for sanity's sake - this.expedition_manager = null; - this.password_manager = null; - this.blackwhite_list_manager = null; - this.recently_read_manager = null; - this.ai_tag_manager = null; - this.library_index = null; - this.folder_watcher_manager = null; - this.library_db = null; + expedition_manager = null; + password_manager = null; + blackwhite_list_manager = null; + recently_read_manager = null; + ai_tag_manager = null; + library_index = null; + folder_watcher_manager = null; + library_db = null; - this.web_library_detail = null; // cyclic reference as WebLibraryDetail instance reference us, so we MUST nil this one to break the cycle for the GC to work well. +#if false + web_library_detail = null; // cyclic reference as WebLibraryDetail instance reference us, so we MUST nil this one to break the cycle for the GC to work well. +#else + // cyclic reference as WebLibraryDetail instance reference us, so we MUST nil this one to break the cycle for the GC to work well. + // + // WARNING: + // The most obvious way (see above in `#if false` branch) would be to NULL the weblibdetail reference, but this will cause all sorts of extremely + // nasty crashes, including memory corruption, as this reference is accessed in Library background task(s) which might discover that the + // library at hand has been killed rather late. + // + // When those code chunks, e.g. *anything* inside `BuildFromDocumentRepository()`, crash on a NULL dereference of any of the other NULLed + // library members, the error resolution code highly depends on a *still working* web_library_detail reference/instance. + // To resolve the cyclic reference in there (as the web_lib_detail has a `Library` reference), we hack this by creating a *temporary* + // intermediate web_library_detail instance, which is a copy of the original *sans Library reference*. + // We DO NOT nuke the Library member in the original web_library_detail as that would cause all sorts of other harm since there's other + // code which depends on a certain valid lifetime of that instance and that code should dispose of the record once it is done using it... + // + // Cloning... + web_library_detail = web_library_detail.CloneSansLibraryReference(); + +#endif ++dispose_count; } - #endregion +#endregion } } diff --git a/Qiqqa/DocumentLibrary/WebLibraryStuff/WebLibraryDetail.cs b/Qiqqa/DocumentLibrary/WebLibraryStuff/WebLibraryDetail.cs index 7b096468b..7c07dcb90 100644 --- a/Qiqqa/DocumentLibrary/WebLibraryStuff/WebLibraryDetail.cs +++ b/Qiqqa/DocumentLibrary/WebLibraryStuff/WebLibraryDetail.cs @@ -1,5 +1,4 @@ using System; -using System.Reflection; using ProtoBuf; using Utilities.Strings; @@ -34,35 +33,17 @@ public class WebLibraryDetail [ProtoMember(14)] public bool IsReadOnly { get; set; } - public bool IsWebLibrary - { - get - { - return !String.IsNullOrEmpty(ShortWebId); - } - } + public bool IsWebLibrary => !String.IsNullOrEmpty(ShortWebId); /* Only valid for intranet libraries */ [ProtoMember(13)] public string IntranetPath { get; set; } - public bool IsIntranetLibrary - { - get - { - return !String.IsNullOrEmpty(IntranetPath); - } - } + public bool IsIntranetLibrary => !String.IsNullOrEmpty(IntranetPath); /* Only valid for Bundle Libraries */ [ProtoMember(15)] public string BundleManifestJSON { get; set; } - public bool IsBundleLibrary - { - get - { - return !String.IsNullOrEmpty(BundleManifestJSON); - } - } + public bool IsBundleLibrary => !String.IsNullOrEmpty(BundleManifestJSON); [ProtoMember(16)] public DateTime? LastBundleManifestDownloadTimestampUTC { get; set; } @@ -78,7 +59,7 @@ public bool IsBundleLibrary public bool AutoSync { get; set; } [NonSerialized] - public Library library; + public Library library; public override string ToString() { @@ -105,5 +86,30 @@ public string LibraryType() if (IsWebLibrary) return "Web"; return "UNKNOWN"; } + + public WebLibraryDetail CloneSansLibraryReference() + { + // only clone the important fields: + WebLibraryDetail rv = new WebLibraryDetail(); + rv.Id = this.Id; + rv.Title = this.Title; + rv.Description = this.Description; + rv.Deleted = this.Deleted; + //LastSynced + //FolderToWatch + rv.IsLocalGuestLibrary = this.IsLocalGuestLibrary; + //ShortWebId + //IsAdministrator + //IsReadOnly + //IntranetPath + //BundleManifestJSON + //LastBundleManifestDownloadTimestampUTC + //LastBundleManifestIgnoreVersion + rv.IsPurged = this.IsPurged; + //LastServerSyncNotificationDate + rv.AutoSync = false; + + return rv; + } } }