From d4627f0daf3f6bf041734f258c82da236cea654a Mon Sep 17 00:00:00 2001 From: Leland Lucius Date: Tue, 28 Jul 2020 23:25:50 -0500 Subject: [PATCH] AUP3: Reduce crash exposure when compacting There's still the possibility if a crash happens at just the right time that the project will be named "_compact_back" so we should probably look for it during startup. This also changes all "Vacuum" references to "compact". --- src/DBConnection.cpp | 5 + src/ProjectFileIO.cpp | 193 ++++++++++++++----------------------- src/ProjectFileIO.h | 26 ++--- src/ProjectFileManager.cpp | 8 +- src/ProjectFileManager.h | 2 +- src/ProjectManager.cpp | 4 +- src/menus/FileMenus.cpp | 2 +- 7 files changed, 101 insertions(+), 139 deletions(-) diff --git a/src/DBConnection.cpp b/src/DBConnection.cpp index 70f253c9a..2944916ca 100644 --- a/src/DBConnection.cpp +++ b/src/DBConnection.cpp @@ -68,6 +68,11 @@ bool DBConnection::Open(const char *fileName) } // Set default mode + // + // NOTE: There is a noticable delay here when dealing with large multi-hour projects + // that were just created with "Save As". Presumably this is because of the + // journal mode switch from NONE to WAL. Should it be wrapped in a progress + // dialog? SafeMode(); // Kick off the checkpoint thread diff --git a/src/ProjectFileIO.cpp b/src/ProjectFileIO.cpp index a3df16f57..4352b965c 100644 --- a/src/ProjectFileIO.cpp +++ b/src/ProjectFileIO.cpp @@ -722,11 +722,11 @@ bool ProjectFileIO::DeleteBlocks(const BlockIDs &blockids, bool complement) return true; } -Connection ProjectFileIO::CopyTo(const FilePath &destpath, - const TranslatableString &msg, - bool isTemporary, - bool prune /* = false */, - const std::shared_ptr &tracks /* = nullptr */) +bool ProjectFileIO::CopyTo(const FilePath &destpath, + const TranslatableString &msg, + bool isTemporary, + bool prune /* = false */, + const std::shared_ptr &tracks /* = nullptr */) { // Get access to the active tracklist auto pProject = &mProject; @@ -751,7 +751,7 @@ Connection ProjectFileIO::CopyTo(const FilePath &destpath, if (!Query("SELECT blockid FROM sampleblocks;", cb)) { - return nullptr; + return false; } } @@ -793,17 +793,20 @@ Connection ProjectFileIO::CopyTo(const FilePath &destpath, SetDBError( XO("Unable to attach destination database") ); - return nullptr; + return false; } // Ensure attached DB connection gets configured + // + // NOTE: Between the above attach and setting the mode here, a normal DELETE + // mode journal will be used and will briefly appear in the filesystem. CurrConn()->FastMode("outbound"); // Install our schema into the new database if (!InstallSchema(db, "outbound")) { // Message already set - return nullptr; + return false; } // Copy over tags (not really used yet) @@ -818,7 +821,7 @@ Connection ProjectFileIO::CopyTo(const FilePath &destpath, XO("Failed to copy tags") ); - return nullptr; + return false; } { @@ -845,7 +848,7 @@ Connection ProjectFileIO::CopyTo(const FilePath &destpath, SetDBError( XO("Unable to prepare project file command:\n\n%s").Format(sql) ); - return nullptr; + return false; } /* i18n-hint: This title appears on a dialog that indicates the progress @@ -881,7 +884,7 @@ Connection ProjectFileIO::CopyTo(const FilePath &destpath, SetDBError( XO("Failed to update the project file.\nThe following command failed:\n\n%s").Format(sql) ); - return nullptr; + return false; } // Reset statement to beginning @@ -895,7 +898,7 @@ Connection ProjectFileIO::CopyTo(const FilePath &destpath, { // Note that we're not setting success, so the finally // block above will take care of cleaning up - return nullptr; + return false; } } @@ -906,7 +909,7 @@ Connection ProjectFileIO::CopyTo(const FilePath &destpath, // projects do not have a "project" doc. if (!WriteDoc(isTemporary ? "autosave" : "project", doc, "outbound")) { - return nullptr; + return false; } // See BEGIN above... @@ -921,29 +924,16 @@ Connection ProjectFileIO::CopyTo(const FilePath &destpath, XO("Destination project could not be detached") ); - return nullptr; - } - - // Open the newly created database - destConn = std::make_unique(mProject.shared_from_this()); - if (!destConn->Open(destpath)) - { - SetDBError( - XO("Failed to open copy of project file") - ); - - destConn = nullptr; - - return nullptr; + return false; } // Tell cleanup everything is good to go success = true; - return destConn; + return true; } -bool ProjectFileIO::ShouldVacuum(const std::shared_ptr &tracks) +bool ProjectFileIO::ShouldCompact(const std::shared_ptr &tracks) { SampleBlockIDSet active; unsigned long long current = 0; @@ -970,7 +960,7 @@ bool ProjectFileIO::ShouldVacuum(const std::shared_ptr &tracks) "FROM sampleblocks;", cb) || total == 0) { - // Shouldn't vacuum since we don't have the full picture + // Shouldn't compact since we don't have the full picture return false; } @@ -983,10 +973,10 @@ bool ProjectFileIO::ShouldVacuum(const std::shared_ptr &tracks) wxLogDebug(wxT("used = %lld total = %lld %lld"), current, total, total ? current / total : 0); if (!total || current / total > 80) { - wxLogDebug(wxT("not vacuuming")); + wxLogDebug(wxT("not compacting")); return false; } - wxLogDebug(wxT("vacuuming")); + wxLogDebug(wxT("compacting")); return true; } @@ -997,20 +987,20 @@ Connection &ProjectFileIO::CurrConn() return connectionPtr.mpConnection; } -void ProjectFileIO::Vacuum(const std::shared_ptr &tracks, bool force /* = false */) +void ProjectFileIO::Compact(const std::shared_ptr &tracks, bool force /* = false */) { - // Haven't vacuumed yet - mWasVacuumed = false; + // Haven't compacted yet + mWasCompacted = false; // Assume we have unused block until we found out otherwise. That way cleanup // at project close time will still occur. mHadUnused = true; - // Don't vacuum if this is a temporary project or if it's determined there are not + // Don't compact if this is a temporary project or if it's determined there are not // enough unused blocks to make it worthwhile if (!force) { - if (IsTemporary() || !ShouldVacuum(tracks)) + if (IsTemporary() || !ShouldCompact(tracks)) { // Delete the AutoSave doc it if exists if (IsModified()) @@ -1031,82 +1021,47 @@ void ProjectFileIO::Vacuum(const std::shared_ptr &tracks, bool force WriteXML(doc, false, tracks); wxString origName = mFileName; - wxString tempName = origName + "_vacuum"; - - // Must close the database to rename it - if (!CloseConnection()) - { - return; - } - - // Shouldn't need to do this, but doesn't hurt. - wxRemoveFile(tempName); - - // Rename the original to temporary - if (!wxRenameFile(origName, tempName)) - { - OpenConnection(origName); - - return; - } - - // Reopen the original database using the temporary name - Connection tempConn = - std::make_unique(mProject.shared_from_this()); - if (!tempConn->Open(tempName)) - { - SetDBError(XO("Failed to open project file")); - - wxRenameFile(tempName, origName); - - OpenConnection(origName); - - return; - } - - // Reactivate the original database using the temporary name - UseConnection(std::move(tempConn), tempName); + wxString backName = origName + "_compact_back"; + wxString tempName = origName + "_compact_temp"; // Copy the original database to a new database while pruning unused sample blocks - Connection newConn = CopyTo(origName, - XO("Compacting project"), - mTemporary, - true, - tracks); - - // Close connection referencing the original database via it's temporary name - CloseConnection(); - - // If the copy failed or we weren't able to write the project doc, backout - if (!newConn) + if (CopyTo(tempName, XO("Compacting project"), mTemporary, true, tracks)) { - // AUD3 warn user somehow - wxRemoveFile(origName); + // Must close the database to rename it + if (CloseConnection()) + { + // Rename the original to backup + if (wxRenameFile(origName, backName)) + { + // Rename the temporary to original + if (wxRenameFile(tempName, origName)) + { + // Open the newly compacted original file + OpenConnection(origName); - // AUD3 warn user somehow - wxRenameFile(tempName, origName); + // Remove the old original file + wxRemoveFile(backName); - // Reopen original file - OpenConnection(origName); + // Remember that we compacted + mWasCompacted = true; - return; + return; + } + wxRenameFile(backName, origName); + } + + OpenConnection(origName); + } + + wxRemoveFile(tempName); } - // Use the newly vacuumed file and the original name. - UseConnection(std::move(newConn), origName); - - // Remove the unvacuumed version of the original - wxRemoveFile(tempName); - - // Remember that we vacuumed - mWasVacuumed = true; - return; } -bool ProjectFileIO::WasVacuumed() +bool ProjectFileIO::WasCompacted() { - return mWasVacuumed; + return mWasCompacted; } bool ProjectFileIO::HadUnused() @@ -1921,17 +1876,29 @@ bool ProjectFileIO::SaveProject(const FilePath &fileName, const std::shared_ptr< { // Do NOT prune here since we need to retain the Undo history // after we switch to the new file. - Connection newConn = CopyTo(fileName, XO("Saving project"), false); - if (!newConn) + if (!CopyTo(fileName, XO("Saving project"), false)) { return false; } + // Open the newly created database + Connection newConn = std::make_unique(mProject.shared_from_this()); + if (!newConn->Open(fileName)) + { + SetDBError( + XO("Failed to open copy of project file") + ); + + newConn = nullptr; + + return false; + } + // Autosave no longer needed in original project file AutoSaveDelete(); - // Try to vacuum the orignal project file - Vacuum(lastSaved); + // Try to compact the orignal project file + Compact(lastSaved ? lastSaved : TrackList::Create(&mProject)); // Save to close the original project file now CloseProject(); @@ -1974,17 +1941,7 @@ bool ProjectFileIO::SaveProject(const FilePath &fileName, const std::shared_ptr< bool ProjectFileIO::SaveCopy(const FilePath& fileName) { - Connection db = CopyTo(fileName, XO("Backing up project"), false, true); - if (!db) - { - return false; - } - - // All good...close the database - db->Close(); - db = nullptr; - - return true; + return CopyTo(fileName, XO("Backing up project"), false, true); } bool ProjectFileIO::CloseProject() @@ -2104,14 +2061,14 @@ void ProjectFileIO::SetBypass() // Determine if we can bypass sample block deletes during shutdown. // // IMPORTANT: - // If the project was vacuumed, then we MUST bypass further + // If the project was compacted, then we MUST bypass further // deletions since the new file doesn't have the blocks that the // Sequences expect to be there. currConn->SetBypass( true ); // Only permanent project files need cleaning at shutdown - if (!IsTemporary() && !WasVacuumed()) + if (!IsTemporary() && !WasCompacted()) { // If we still have unused blocks, then we must not bypass deletions // during shutdown. Otherwise, we would have orphaned blocks the next time diff --git a/src/ProjectFileIO.h b/src/ProjectFileIO.h index 5699cace9..34a6298dc 100644 --- a/src/ProjectFileIO.h +++ b/src/ProjectFileIO.h @@ -106,12 +106,12 @@ public: void SetBypass(); // Remove all unused space within a project file - void Vacuum(const std::shared_ptr &tracks, bool force = false); + void Compact(const std::shared_ptr &tracks, bool force = false); - // The last vacuum check did actually vacuum the project file if true - bool WasVacuumed(); + // The last compact check did actually compact the project file if true + bool WasCompacted(); - // The last vacuum check found unused blocks in the project file + // The last compact check found unused blocks in the project file bool HadUnused(); bool TransactionStart(const wxString &name); @@ -178,16 +178,16 @@ public: private: // Return a database connection if successful, which caller must close - Connection CopyTo(const FilePath &destpath, - const TranslatableString &msg, - bool isTemporary, - bool prune = false, - const std::shared_ptr &tracks = nullptr); + bool CopyTo(const FilePath &destpath, + const TranslatableString &msg, + bool isTemporary, + bool prune = false, + const std::shared_ptr &tracks = nullptr); void SetError(const TranslatableString & msg); void SetDBError(const TranslatableString & msg); - bool ShouldVacuum(const std::shared_ptr &tracks); + bool ShouldCompact(const std::shared_ptr &tracks); private: Connection &CurrConn(); @@ -207,10 +207,10 @@ private: // Is this project still a temporary/unsaved project bool mTemporary; - // Project was vacuumed last time Vacuum() ran - bool mWasVacuumed; + // Project was compacted last time Compact() ran + bool mWasCompacted; - // Project had unused blocks during last Vacuum() + // Project had unused blocks during last Compact() bool mHadUnused; Connection mPrevConn; diff --git a/src/ProjectFileManager.cpp b/src/ProjectFileManager.cpp index a743eaf51..49d0b5040 100644 --- a/src/ProjectFileManager.cpp +++ b/src/ProjectFileManager.cpp @@ -676,7 +676,7 @@ bool ProjectFileManager::SaveFromTimerRecording(wxFileName fnFile) return success; } -void ProjectFileManager::VacuumProject() +void ProjectFileManager::CompactProject() { auto &project = mProject; auto &projectFileIO = ProjectFileIO::Get(project); @@ -691,8 +691,8 @@ void ProjectFileManager::VacuumProject() wt->CloseLock(); } - // Attempt to vacuum the project - projectFileIO.Vacuum(mLastSavedTracks); + // Attempt to compact the project + projectFileIO.Compact(mLastSavedTracks); } } @@ -711,7 +711,7 @@ void ProjectFileManager::CloseProject() projectFileIO.CloseProject(); - // Blocks were locked in VacuumProject, so DELETE the data structure so that + // Blocks were locked in CompactProject, so DELETE the data structure so that // there's no memory leak. if (mLastSavedTracks) { diff --git a/src/ProjectFileManager.h b/src/ProjectFileManager.h index e8bf9ca86..de201eab2 100644 --- a/src/ProjectFileManager.h +++ b/src/ProjectFileManager.h @@ -53,7 +53,7 @@ public: bool OpenProject(); void CloseProject(); - void VacuumProject(); + void CompactProject(); bool Save(); bool SaveAs(); diff --git a/src/ProjectManager.cpp b/src/ProjectManager.cpp index daac0d110..1d3acb597 100644 --- a/src/ProjectManager.cpp +++ b/src/ProjectManager.cpp @@ -732,8 +732,8 @@ void ProjectManager::OnCloseWindow(wxCloseEvent & event) // TODO: Is there a Mac issue here?? // SetMenuBar(NULL); - // Vacuum the project. - projectFileManager.VacuumProject(); + // Compact the project. + projectFileManager.CompactProject(); // Set (or not) the bypass flag to indicate that deletes that would happen during // the UndoManager::ClearStates() below are not necessary. diff --git a/src/menus/FileMenus.cpp b/src/menus/FileMenus.cpp index 5a1c570f5..7e44e34d7 100644 --- a/src/menus/FileMenus.cpp +++ b/src/menus/FileMenus.cpp @@ -183,7 +183,7 @@ void OnCompact(const CommandContext &context) currentTracks->Add(t->Duplicate()); } - projectFileIO.Vacuum(currentTracks, true); + projectFileIO.Compact(currentTracks, true); auto after = wxFileName(projectFileIO.GetFileName()).GetSize() + wxFileName(projectFileIO.GetFileName() + wxT("-wal")).GetSize();