diff --git a/src/ProjectFileIO.cpp b/src/ProjectFileIO.cpp index 7c10c6082..277e0ad0f 100644 --- a/src/ProjectFileIO.cpp +++ b/src/ProjectFileIO.cpp @@ -14,6 +14,7 @@ Paul Licameli split from AudacityProject.cpp #include #include #include +#include #include #include @@ -134,16 +135,10 @@ static const char *SafeConfig = // Configuration to provide "Fast" connections static const char *FastConfig = - "PRAGMA .locking_mode = EXCLUSIVE;" + "PRAGMA .locking_mode = SHARED;" "PRAGMA .synchronous = OFF;" "PRAGMA .journal_mode = OFF;"; -// Configuration to provide "Fast" connections -static const char *FastWalConfig = - "PRAGMA .locking_mode = EXCLUSIVE;" - "PRAGMA .synchronous = OFF;" - "PRAGMA .journal_mode = WAL;"; - // This singleton handles initialization/shutdown of the SQLite library. // It is needed because our local SQLite is built with SQLITE_OMIT_AUTOINIT // defined. @@ -273,34 +268,12 @@ void ProjectFileIO::Init( AudacityProject &project ) mpProject = project.shared_from_this(); // Kick off the checkpoint thread - mCheckpointStop = false; mCheckpointThread = std::thread([this]{ CheckpointThread(); }); } ProjectFileIO::~ProjectFileIO() { - if (mDB) - { - // Save the filename since CloseDB() will clear it - wxString filename = mFileName; - - // Not much we can do if this fails. The user will simply get - // the recovery dialog upon next restart. - if (CloseDB()) - { - // At this point, we are shutting down cleanly and if the project file is - // still in the temp directory it means that the user has chosen not to - // save it. So, delete it. - if (mTemporary) - { - wxFileName temp(FileNames::TempDir()); - if (temp == wxPathOnly(filename)) - { - wxRemoveFile(filename); - } - } - } - } + wxASSERT_MSG(mDB == nullptr, wxT("Project file was not closed at shutdown")); // Tell the checkpoint thread to shutdown { @@ -313,20 +286,19 @@ ProjectFileIO::~ProjectFileIO() mCheckpointThread.join(); } -#define USE_DIFFERENT_DB_HANDLE 1 - void ProjectFileIO::CheckpointThread() { + mCheckpointStop = false; + while (true) { - sqlite3 *db; { - // Wait for something to show up in the queue or for the stop signal + // Wait for work or the stop signal std::unique_lock lock(mCheckpointMutex); mCheckpointCondition.wait(lock, [&] { - return mCheckpointStop || !mCheckpointWork.empty(); + return mCheckpointWaitingPages || mCheckpointStop; }); // Requested to stop, so bail @@ -335,45 +307,33 @@ void ProjectFileIO::CheckpointThread() break; } - // Pop the head of the queue - db = mCheckpointWork.front(); - mCheckpointWork.pop_front(); + // Capture the number of pages that need checkpointing and reset + mCheckpointCurrentPages = mCheckpointWaitingPages; + mCheckpointWaitingPages = 0; -#if defined(USE_DIFFERENT_DB_HANDLE) - // Lock out other while the checkpoint is running + // Lock out others while the checkpoint is running mCheckpointActive.lock(); -#endif } + // Open another connection to the DB to prevent blocking the main thread. + sqlite3 *db = nullptr; + if (sqlite3_open(mFileName, &db) == SQLITE_OK) { - // Get it's filename - const char *filename = sqlite3_db_filename(db, nullptr); + // Configure it to be safe + Config(db, SafeConfig); + // And kick off the checkpoint. This may not checkpoint ALL frames + // in the WAL. They'll be gotten the next time around. + sqlite3_wal_checkpoint_v2(db, nullptr, SQLITE_CHECKPOINT_PASSIVE, nullptr, nullptr); -#if defined(USE_DIFFERENT_DB_HANDLE) - // Open it. We don't use the queued database pointer because that - // would block the main thread if it tried to use it at the same time. - db = nullptr; - if (sqlite3_open(filename, &db) == SQLITE_OK) -#endif - { -#if defined(USE_DIFFERENT_DB_HANDLE) - // Configure it to be safe - Config(db, SafeConfig); -#endif - - // And do the actual checkpoint. This may not checkpoint ALL frames - // in the WAL. They'll be gotten the next time round. - sqlite3_wal_checkpoint_v2(db, nullptr, SQLITE_CHECKPOINT_PASSIVE, nullptr, nullptr); - } - -#if defined(USE_DIFFERENT_DB_HANDLE) - // All done...okay to call with null pointer if open failed. + // All done. sqlite3_close(db); + // Reset + mCheckpointCurrentPages = 0; + // Checkpoint is complete mCheckpointActive.unlock(); -#endif } } @@ -387,7 +347,7 @@ int ProjectFileIO::CheckpointHook(void *data, sqlite3 *db, const char *schema, i // Qeuue the database pointer for our checkpoint thread to process std::lock_guard guard(that->mCheckpointMutex); - that->mCheckpointWork.push_back(db); + that->mCheckpointWaitingPages = pages; that->mCheckpointCondition.notify_one(); return SQLITE_OK; @@ -548,26 +508,35 @@ bool ProjectFileIO::CloseDB() if (mDB) { - // Uninstall our checkpoint hook + // Uninstall our checkpoint hook so that no additional checkpoints + // are sent our way. (Though this shouldn't really happen.) sqlite3_wal_hook(mDB, nullptr, nullptr); - // Remove this connection from the checkpoint work queue, if any + // Display a progress dialog if there's active or pending checkpoints + if (mCheckpointWaitingPages || mCheckpointCurrentPages) { - std::lock_guard guard(mCheckpointMutex); + TranslatableString title = XO("Checkpointing project"); - mCheckpointWork.erase(std::remove(mCheckpointWork.begin(), - mCheckpointWork.end(), - mDB), - mCheckpointWork.end()); + // Get access to the active tracklist + auto pProject = mpProject.lock(); + if (pProject) + { + title = XO("Checkpointing %s").Format(pProject->GetProjectName()); + } + + wxGenericProgressDialog pd(title.Translation(), + XO("This may take several seconds").Translation(), + 300000, // range + nullptr, // parent + wxPD_APP_MODAL | wxPD_ELAPSED_TIME | wxPD_SMOOTH); + + while (mCheckpointWaitingPages || mCheckpointCurrentPages) + { + wxMilliSleep(50); + pd.Pulse(); + } } -#if defined(USE_DIFFERENT_DB_HANDLE) - // Wait for ANY active checkpoint to complete as it may be for - // this database - mCheckpointActive.lock(); - mCheckpointActive.unlock(); -#endif - // Close the DB rc = sqlite3_close(mDB); if (rc != SQLITE_OK) @@ -856,7 +825,7 @@ bool ProjectFileIO::InstallSchema(sqlite3 *db, const char *schema /* = "main" */ sql.Printf(ProjectFileSchema, ProjectFileID, ProjectFileVersion); sql.Replace("", schema); - rc = sqlite3_exec(db, sql.mb_str().data(), nullptr, nullptr, nullptr); + rc = sqlite3_exec(db, sql, nullptr, nullptr, nullptr); if (rc != SQLITE_OK) { SetDBError( @@ -998,7 +967,7 @@ sqlite3 *ProjectFileIO::CopyTo(const FilePath &destpath, wxString sql; sql.Printf("ATTACH DATABASE '%s' AS outbound;", destpath); - rc = sqlite3_exec(db, sql.mb_str().data(), nullptr, nullptr, nullptr); + rc = sqlite3_exec(db, sql, nullptr, nullptr, nullptr); if (rc != SQLITE_OK) { SetDBError( @@ -1133,17 +1102,11 @@ sqlite3 *ProjectFileIO::CopyTo(const FilePath &destpath, } // Ensure attached DB connection gets configured - Config(destdb, FastWalConfig); + Config(destdb, SafeConfig); // Tell cleanup everything is good to go success = true; - // IMPORTANT: At this point, the DB handle is setup with WAL journaling, - // but synchronous is OFF. Only use this connection if you can tolerate - // a crash/power failure. Otherwise, reconfigure it using SafeConfig. - // - // It's left configured this way so that the caller can do additional - // work using a faster, but VOLATILE connection. return destdb; } @@ -1225,9 +1188,19 @@ void ProjectFileIO::Vacuum(const std::shared_ptr &tracks) // close time will still occur mHadUnused = true; - // Don't vacuum if this is a temporary project or if Go figure out if we should at least try + // Don't vacuum if this is a temporary project or if it's deteremined there not + // enough unused blocks to make it worthwhile if (IsTemporary() || !ShouldVacuum(tracks)) { + // Delete the AutoSave doc it if exists + if (IsModified()) + { + // PRL: not clear what to do if the following fails, but the worst should + // be, the project may reopen in its present state as a recovery file, not + // at the last saved state. + (void) AutoSaveDelete(); + } + return; } @@ -1746,7 +1719,7 @@ bool ProjectFileIO::ImportProject(const FilePath &fileName) wxString sql; sql.Printf("ATTACH DATABASE 'file:%s?immutable=1&mode=ro' AS inbound;", fileName); - rc = sqlite3_exec(db, sql.mb_str().data(), nullptr, nullptr, nullptr); + rc = sqlite3_exec(db, sql, nullptr, nullptr, nullptr); if (rc != SQLITE_OK) { SetDBError( @@ -1933,7 +1906,7 @@ bool ProjectFileIO::ImportProject(const FilePath &fileName) wxString sql; sql.Printf("DELETE FROM main.sampleblocks WHERE blockid = %lld", blockid); - rc = sqlite3_exec(db, sql.mb_str().data(), nullptr, nullptr, nullptr); + rc = sqlite3_exec(db, sql, nullptr, nullptr, nullptr); if (rc != SQLITE_OK) { // This is non-fatal...it'll just get cleaned up the next @@ -2167,7 +2140,6 @@ bool ProjectFileIO::SaveProject(const FilePath &fileName) { wxRemoveFile(origName); } - } else { @@ -2209,6 +2181,9 @@ bool ProjectFileIO::SaveProject(const FilePath &fileName) // And make it the active project file UseConnection(newDB, fileName); + + // Install our checkpoint hook + sqlite3_wal_hook(mDB, CheckpointHook, this); } ProjectSerializer doc; @@ -2252,21 +2227,6 @@ bool ProjectFileIO::SaveCopy(const FilePath& fileName) return false; } - bool success = false; - - auto cleanup = finally([&] - { - if (db) - { - (void) sqlite3_close(db); - } - - if (!success) - { - wxRemoveFile(fileName); - } - }); - ProjectSerializer doc; WriteXMLHeader(doc); WriteXML(doc); @@ -2274,11 +2234,40 @@ bool ProjectFileIO::SaveCopy(const FilePath& fileName) // Write the project doc to the new DB if (!WriteDoc("project", doc, db)) { + wxRemoveFile(fileName); return false; } - // Tell the finally block to behave - success = true; + // All good...close the database + (void) sqlite3_close(db); + + return true; +} + +bool ProjectFileIO::CloseProject() +{ + if (mDB) + { + // Save the filename since CloseDB() will clear it + wxString filename = mFileName; + + // Not much we can do if this fails. The user will simply get + // the recovery dialog upon next restart. + if (CloseDB()) + { + // If this is a temporary project, we no longer want to keep the + // project file. + if (mTemporary) + { + // This is just a safety check. + wxFileName temp(FileNames::TempDir()); + if (temp == wxPathOnly(filename)) + { + wxRemoveFile(filename); + } + } + } + } return true; } @@ -2350,12 +2339,37 @@ void ProjectFileIO::SetDBError(const TranslatableString &msg) wxLogDebug(wxT(" Lib error: %s"), mLibraryError.Debug()); printf(" Lib error: %s", mLibraryError.Debug().mb_str().data()); } + abort(); wxASSERT(false); } -void ProjectFileIO::Bypass(bool bypass) +void ProjectFileIO::SetBypass() { - mBypass = bypass; + // Determine if we can bypass sample block deletes during shutdown. + // + // IMPORTANT: + // If the project was vacuumed, then we MUST bypass further + // deletions since the new file doesn't have the blocks that the + // Sequences expect to be there. + mBypass = true; + + // Only permanent project files need cleaning at shutdown + if (!IsTemporary() && !WasVacuumed()) + { + // If we still have unused blocks, then we must not bypass deletions + // during shutdown. Otherwise, we would have orphaned blocks the next time + // the project is opened. + // + // An example of when dead blocks will exist is when a user opens a permanent + // project, adds a track (with samples) to it, and chooses not to save the + // changes. + if (HadUnused()) + { + mBypass = false; + } + } + + return; } bool ProjectFileIO::ShouldBypass() diff --git a/src/ProjectFileIO.h b/src/ProjectFileIO.h index d29cb2f20..b839e0d2e 100644 --- a/src/ProjectFileIO.h +++ b/src/ProjectFileIO.h @@ -13,7 +13,6 @@ Paul Licameli split from AudacityProject.h #include #include -#include #include #include #include @@ -86,6 +85,7 @@ public: bool LoadProject(const FilePath &fileName); bool SaveProject(const FilePath &fileName); bool SaveCopy(const FilePath& fileName); + bool CloseProject(); wxLongLong GetFreeDiskSpace(); @@ -104,7 +104,7 @@ public: // For it's usage, see: // SqliteSampleBlock::~SqliteSampleBlock() // ProjectManager::OnCloseWindow() - void Bypass(bool bypass); + void SetBypass(); bool ShouldBypass(); // Remove all unused space within a project file @@ -230,12 +230,14 @@ private: TranslatableString mLastError; TranslatableString mLibraryError; - std::deque mCheckpointWork; + std::thread mCheckpointThread; std::condition_variable mCheckpointCondition; std::mutex mCheckpointMutex; - std::thread mCheckpointThread; - std::atomic_bool mCheckpointStop; std::mutex mCheckpointActive; + std::mutex mCheckpointClose; + std::atomic_bool mCheckpointStop; + uint64_t mCheckpointWaitingPages; + uint64_t mCheckpointCurrentPages; friend SqliteSampleBlock; friend AutoCommitTransaction; diff --git a/src/ProjectFileManager.cpp b/src/ProjectFileManager.cpp index 058badb23..e570c9412 100644 --- a/src/ProjectFileManager.cpp +++ b/src/ProjectFileManager.cpp @@ -618,12 +618,23 @@ bool ProjectFileManager::SaveCopy(const FilePath &fileName /* = wxT("") */) void ProjectFileManager::Reset() { - // mLastSavedTrack code copied from OnCloseWindow. // Lock all blocks in all tracks of the last saved version, so that // the blockfiles aren't deleted on disk when we DELETE the blockfiles // in memory. After it's locked, DELETE the data structure so that // there's no memory leak. - CloseLock(); + if (mLastSavedTracks) + { + auto &project = mProject; + auto &projectFileIO = ProjectFileIO::Get(project); + + for (auto wt : mLastSavedTracks->Any()) + { + wt->CloseLock(); + } + + mLastSavedTracks->Clear(); + mLastSavedTracks.reset(); + } ProjectFileIO::Get( mProject ).Reset(); } @@ -661,17 +672,17 @@ bool ProjectFileManager::SaveFromTimerRecording(wxFileName fnFile) return success; } -void ProjectFileManager::CloseLock() +void ProjectFileManager::VacuumProject() { + auto &project = mProject; + auto &projectFileIO = ProjectFileIO::Get(project); + // Lock all blocks in all tracks of the last saved version, so that // the blockfiles aren't deleted on disk when we DELETE the blockfiles // in memory. After it's locked, DELETE the data structure so that // there's no memory leak. if (mLastSavedTracks) { - auto &project = mProject; - auto &projectFileIO = ProjectFileIO::Get(project); - for (auto wt : mLastSavedTracks->Any()) { wt->CloseLock(); @@ -679,7 +690,20 @@ void ProjectFileManager::CloseLock() // Attempt to vacuum the project projectFileIO.Vacuum(mLastSavedTracks); + } +} +void ProjectFileManager::CloseProject() +{ + auto &project = mProject; + auto &projectFileIO = ProjectFileIO::Get(project); + + projectFileIO.CloseProject(); + + // Blocks were locked in VacuumProject, so DELETE the data structure so that + // there's no memory leak. + if (mLastSavedTracks) + { mLastSavedTracks->Clear(); mLastSavedTracks.reset(); } @@ -954,79 +978,12 @@ void ProjectFileManager::OpenFile(const FilePath &fileNameArg, bool addtohistory FileHistory::Global().Append(fileName); } - // Use a finally block here, because there are calls to Save() below which - // might throw. - bool closed = false; - auto cleanup = finally( [&] { - if (! closed ) { - // For an unknown reason, OSX requires that the project window be - // raised if a recovery took place. - window.CallAfter( [&] { window.Raise(); } ); - } - } ); - if (bParseSuccess) { - bool saved = false; - if (projectFileIO.IsRecovered()) { -#if 0 // AUD3 TODO NEED TO WRITE A PROJECT FILE CONSISTENCY CHECKER - // This project has been recovered, so write a NEW auto-save file - // now and then DELETE the old one in the auto-save folder. Note that - // at this point mFileName != fileName, because when opening a - // recovered file mFileName is faked to point to the original file - // which has been recovered, not the one in the auto-save folder. - ::ProjectFSCK(mProject, err, true); // Correct problems in auto-recover mode. -#endif - // PushState calls AutoSave(), so no longer need to do so here. history.PushState(XO("Project was recovered"), XO("Recover")); } - else - { -#if 0 // AUD3 TODO NEED TO WRITE A PROJECT FILE CONSISTENCY CHECKER - // This is a regular project, check it and ask user - int status = ::ProjectFSCK(dirManager, err, false); - if (status & FSCKstatus_CLOSE_REQ) - { - // Vaughan, 2010-08-23: Note this did not do a real close. - // It could cause problems if you get this, say on missing alias files, - // then try to open a project with, e.g., missing blockfiles. - // It then failed in SetProject, saying it cannot find the files, - // then never go through ProjectFSCK to give more info. - // Going through OnClose() may be overkill, but it's safe. - /* - // There was an error in the load/check and the user - // explicitly opted to close the project. - mTracks->Clear(true); - mFileName = wxT(""); - SetProjectTitle(); - mTrackPanel->Refresh(true); - */ - closed = true; - SetMenuClose(true); - window.Close(); - return; - } - else if (status & FSCKstatus_CHANGED) - { - // Mark the wave tracks as changed and redraw. - for ( auto wt : tracks.Any() ) - // Only wave tracks have a notion of "changed". - for (const auto &clip: wt->GetClips()) - clip->MarkChanged(); - - trackPanel.Refresh(true); - - // Vaughan, 2010-08-20: This was bogus, as all the actions in ProjectFSCK - // that return FSCKstatus_CHANGED cannot be undone. - // this->PushState(XO("Project checker repaired file"), XO("Project Repair")); - - if (status & FSCKstatus_SAVE_AUP) - Save(), saved = true; - } -#endif - } } else { // Vaughan, 2011-10-30: diff --git a/src/ProjectFileManager.h b/src/ProjectFileManager.h index bbfc13f4f..bfad5129b 100644 --- a/src/ProjectFileManager.h +++ b/src/ProjectFileManager.h @@ -50,9 +50,8 @@ public: }; ReadProjectResults ReadProjectFile( const FilePath &fileName ); - // To be called when closing a project that has been saved, so that - // block files are not erased - void CloseLock(); + void VacuumProject(); + void CloseProject(); bool Save(); bool SaveAs(); diff --git a/src/ProjectManager.cpp b/src/ProjectManager.cpp index 67d2e6f36..ca2f5526b 100644 --- a/src/ProjectManager.cpp +++ b/src/ProjectManager.cpp @@ -734,44 +734,12 @@ void ProjectManager::OnCloseWindow(wxCloseEvent & event) // TODO: Is there a Mac issue here?? // SetMenuBar(NULL); - // Lock active sample blocks so they don't get deleted below. This also performs - // a vacuum if necessary. - projectFileManager.CloseLock(); + // Vacuum the project. + projectFileManager.VacuumProject(); - // Determine if we can bypass sample block deletes during shutdown. - // - // IMPORTANT: - // If the project was vacuumed, then we MUST bypass further - // deletions since the new file doesn't have the blocks that the - // Sequences expect to be there. - bool bypass = true; - - // Only permanent project files need cleaning at shutdown - if (!projectFileIO.IsTemporary() && !projectFileIO.WasVacuumed()) - { - // Delete the AutoSave doc it if still exists - if (projectFileIO.IsModified()) - { - // The user doesn't want to save the project, so delete the AutoSave doc - // PRL: not clear what to do if the following fails, but the worst should - // be, the project may reopen in its present state as a recovery file, not - // at the last saved state. - (void) projectFileIO.AutoSaveDelete(); - } - - // If we still have unused blocks, then we must not bypass deletions - // during shutdown. Otherwise, we would have orphaned blocks the next time - // the project is opened. - // - // An example of when dead blocks will exist is when a user opens a permanent - // project, adds a track (with samples) to it, and chooses not to save the - // changes. - if (projectFileIO.HadUnused()) - { - bypass = false; - } - } - projectFileIO.Bypass(bypass); + // Set (or not) the bypass flag to indicate that deletes that would happen during + // the UndoManager::ClearStates() below are not necessary. + projectFileIO.SetBypass(); { AutoCommitTransaction trans(projectFileIO, "Shutdown"); @@ -784,6 +752,9 @@ void ProjectManager::OnCloseWindow(wxCloseEvent & event) tracks.Clear(); } + // We're all done with the project file, so close it now + projectFileManager.CloseProject(); + // Some of the AdornedRulerPanel functions refer to the TrackPanel, so destroy this // before the TrackPanel is destroyed. This change was needed to stop Audacity // crashing when running with Jaws on Windows 10 1703. diff --git a/src/effects/Effect.cpp b/src/effects/Effect.cpp index d5f30b356..a1535a4c5 100644 --- a/src/effects/Effect.cpp +++ b/src/effects/Effect.cpp @@ -1318,11 +1318,6 @@ bool Effect::DoEffect(double projectRate, auto vr = valueRestorer( mProgress, &progress ); { - // This is for performance purposes only, no additional recovery implied - auto &pProject = *const_cast(FindProject()); // how to remove this const_cast? - auto &pIO = ProjectFileIO::Get(pProject); - AutoCommitTransaction trans(pIO, "Effect"); - returnVal = Process(); } }