From f8981248dc9306cc773143e922795c98b0d2c2a2 Mon Sep 17 00:00:00 2001 From: Bruno Herbelin Date: Tue, 8 Oct 2024 19:05:52 +0200 Subject: [PATCH] BugFix Safe access to xmldoc in snapshots and undo history Added mutex to Action manager for access to undo history and added mutex to session's snapshots. Lock and unlock those mutex for all write access and long read access. --- src/ActionManager.cpp | 104 ++++++++++++++++++++++++----------------- src/ActionManager.h | 6 +-- src/Session.cpp | 2 + src/Session.h | 2 +- src/SessionCreator.cpp | 2 + src/SessionVisitor.cpp | 7 +++ 6 files changed, 76 insertions(+), 47 deletions(-) diff --git a/src/ActionManager.cpp b/src/ActionManager.cpp index 796ff89..1dda8c4 100644 --- a/src/ActionManager.cpp +++ b/src/ActionManager.cpp @@ -42,7 +42,7 @@ using namespace tinyxml2; -Action::Action(): history_step_(0), history_max_step_(0), locked_(false), +Action::Action(): history_step_(0), history_max_step_(0), snapshot_id_(0), snapshot_node_(nullptr), interpolator_(nullptr), interpolator_node_(nullptr) { @@ -64,13 +64,19 @@ void Action::init() // must be called in a thread running in parrallel of the rendering // (needs opengl update to get thumbnail) -void captureMixerSession(Session *se, tinyxml2::XMLDocument *doc, std::string node, std::string label) +void captureMixerSession(Session *se, std::string node, std::string label, tinyxml2::XMLDocument *doc = nullptr) { if (se != nullptr) { + tinyxml2::XMLDocument *_doc = doc; + if (doc == nullptr) { + _doc = se->snapshots()->xmlDoc_; + se->snapshots()->access_.lock(); + } + // create node - XMLElement *sessionNode = doc->NewElement( node.c_str() ); - doc->InsertEndChild(sessionNode); + XMLElement *sessionNode = _doc->NewElement( node.c_str() ); + _doc->InsertEndChild(sessionNode); // label describes the action sessionNode->SetAttribute("label", label.c_str() ); // label describes the action @@ -81,7 +87,7 @@ void captureMixerSession(Session *se, tinyxml2::XMLDocument *doc, std::string no // get the thumbnail (requires one opengl update to render) FrameBufferImage *thumbnail = se->renderThumbnail(); if (thumbnail) { - XMLElement *imageelement = SessionVisitor::ImageToXML(thumbnail, doc); + XMLElement *imageelement = SessionVisitor::ImageToXML(thumbnail, _doc); if (imageelement) sessionNode->InsertEndChild(imageelement); delete thumbnail; @@ -91,34 +97,52 @@ void captureMixerSession(Session *se, tinyxml2::XMLDocument *doc, std::string no sessionNode->SetAttribute("activationThreshold", se->activationThreshold()); // save all sources using source visitor - SessionVisitor sv(doc, sessionNode); + SessionVisitor sv(_doc, sessionNode); for (auto iter = se->begin(); iter != se->end(); ++iter, sv.setRoot(sessionNode) ) (*iter)->accept(sv); + if (doc == nullptr) + se->snapshots()->access_.unlock(); } } +void Action::storeSession(Session *se, std::string label) +{ + // + if (!Action::manager().history_access_.try_lock()) + return; + + // incremental naming of history nodes + Action::manager().history_step_++; + + // erase future + for (uint e = Action::manager().history_step_; e <= Action::manager().history_max_step_; e++) { + XMLElement *node = Action::manager().history_doc_.FirstChildElement( HISTORY_NODE(e).c_str() ); + if ( node ) + Action::manager().history_doc_.DeleteChild(node); + } + Action::manager().history_max_step_ = Action::manager().history_step_; + + // capture current session + captureMixerSession(se, + HISTORY_NODE(Action::manager().history_step_), + label, + &Action::manager().history_doc_); + + Action::manager().history_access_.unlock(); +} + + void Action::store(const std::string &label) { // TODO: set a maximum amount of stored steps? (even very big, just to ensure memory limit) // ignore if locked or if no label is given - if (locked_ || label.empty()) + if (label.empty()) return; - // incremental naming of history nodes - history_step_++; - - // erase future - for (uint e = history_step_; e <= history_max_step_; e++) { - XMLElement *node = history_doc_.FirstChildElement( HISTORY_NODE(e).c_str() ); - if ( node ) - history_doc_.DeleteChild(node); - } - history_max_step_ = history_step_; - // threaded capturing state of current session - std::thread(captureMixerSession, Mixer::manager().session(), &history_doc_, HISTORY_NODE(history_step_), label).detach(); + std::thread(Action::storeSession, Mixer::manager().session(), label).detach(); #ifdef ACTION_DEBUG Log::Info("Action stored %d '%s'", history_step_, label.c_str()); @@ -184,8 +208,7 @@ FrameBufferImage *Action::thumbnail(uint s) const void Action::restore(uint target) { - // lock - locked_ = true; + history_access_.lock(); // get history node of target step history_step_ = CLAMP(target, 1, history_max_step_); @@ -203,8 +226,7 @@ void Action::restore(uint target) Mixer::manager().restore(sessionNode); } - // free - locked_ = false; + history_access_.unlock(); } @@ -220,9 +242,9 @@ void Action::takeSnapshot(Session *se, const std::string &label, bool create_thr if (create_thread) // threaded capture state of current session - std::thread(captureMixerSession, se, se->snapshots()->xmlDoc_, SNAPSHOT_NODE(id), label).detach(); + std::thread(captureMixerSession, se, SNAPSHOT_NODE(id), label, nullptr).detach(); else - captureMixerSession(se, se->snapshots()->xmlDoc_, SNAPSHOT_NODE(id), label); + captureMixerSession(se, SNAPSHOT_NODE(id), label); #ifdef ACTION_DEBUG Log::Info("Snapshot stored %d '%s'", id, label.c_str()); @@ -232,10 +254,6 @@ void Action::takeSnapshot(Session *se, const std::string &label, bool create_thr void Action::snapshot(const std::string &label) { - // ignore if locked - if (locked_) - return; - // ensure label is unique std::string snap_label = BaseToolkit::uniqueName(label, labels()); @@ -250,7 +268,12 @@ void Action::open(uint64_t snapshotid) { // get snapshot node of target in current session Session *se = Mixer::manager().session(); - snapshot_node_ = se->snapshots()->xmlDoc_->FirstChildElement( SNAPSHOT_NODE(snapshotid).c_str() ); + if (se) { + se->snapshots()->access_.lock(); + snapshot_node_ = se->snapshots()->xmlDoc_->FirstChildElement( + SNAPSHOT_NODE(snapshotid).c_str()); + se->snapshots()->access_.unlock(); + } if (snapshot_node_) snapshot_id_ = snapshotid; @@ -263,10 +286,6 @@ void Action::open(uint64_t snapshotid) void Action::replace(uint64_t snapshotid) { - // ignore if locked or if no label is given - if (locked_) - return; - if (snapshotid > 0) open(snapshotid); @@ -277,14 +296,18 @@ void Action::replace(uint64_t snapshotid) // remove previous node Session *se = Mixer::manager().session(); if (se) { + + se->snapshots()->access_.lock(); se->snapshots()->xmlDoc_->DeleteChild( snapshot_node_ ); + se->snapshots()->access_.unlock(); // threaded capture state of current session - std::thread(captureMixerSession, se, se->snapshots()->xmlDoc_, SNAPSHOT_NODE(snapshot_id_), label).detach(); + std::thread(captureMixerSession, se, SNAPSHOT_NODE(snapshot_id_), label, nullptr).detach(); #ifdef ACTION_DEBUG Log::Info("Snapshot replaced %d '%s'", snapshot_id_, label.c_str()); #endif + } } } @@ -378,8 +401,10 @@ void Action::remove(uint64_t snapshotid) if (snapshot_node_) { // remove Session *se = Mixer::manager().session(); + se->snapshots()->access_.lock(); se->snapshots()->xmlDoc_->DeleteChild( snapshot_node_ ); se->snapshots()->keys_.remove( snapshot_id_ ); + se->snapshots()->access_.unlock(); } snapshot_node_ = nullptr; @@ -388,9 +413,6 @@ void Action::remove(uint64_t snapshotid) void Action::restore(uint64_t snapshotid) { - // lock - locked_ = true; - if (snapshotid > 0) open(snapshotid); @@ -398,9 +420,6 @@ void Action::restore(uint64_t snapshotid) // actually restore Mixer::manager().restore(snapshot_node_); - // free - locked_ = false; - store("Snapshot " + label(snapshot_id_)); } @@ -516,8 +535,7 @@ static void saveSnapshot(const std::string& filename, tinyxml2::XMLElement *snap void Action::saveas(const std::string& filename, uint64_t snapshotid) { - // ignore if locked or if no label is given - if (locked_) + if (filename.empty()) return; if (snapshotid > 0) diff --git a/src/ActionManager.h b/src/ActionManager.h index f52f685..2997293 100644 --- a/src/ActionManager.h +++ b/src/ActionManager.h @@ -3,7 +3,7 @@ #include #include -#include +#include #include @@ -62,11 +62,11 @@ public: void interpolate (float val, uint64_t snapshotid = 0); private: - tinyxml2::XMLDocument history_doc_; uint history_step_; uint history_max_step_; - std::atomic locked_; + std::mutex history_access_; + static void storeSession(Session *se, std::string label); void restore(uint target); uint64_t snapshot_id_; diff --git a/src/Session.cpp b/src/Session.cpp index c792e13..d2f4daf 100644 --- a/src/Session.cpp +++ b/src/Session.cpp @@ -970,6 +970,7 @@ Metronome::Synchronicity Session::inputSynchrony(uint input) void Session::applySnapshot(uint64_t key) { + snapshots_.access_.lock(); tinyxml2::XMLElement *snapshot_node_ = snapshots_.xmlDoc_->FirstChildElement( SNAPSHOT_NODE(key).c_str() );; if (snapshot_node_) { @@ -1008,6 +1009,7 @@ void Session::applySnapshot(uint64_t key) ++View::need_deep_update_; } + snapshots_.access_.unlock(); } diff --git a/src/Session.h b/src/Session.h index 6b9a69b..db65276 100644 --- a/src/Session.h +++ b/src/Session.h @@ -2,7 +2,6 @@ #define SESSION_H #include -#include #include "SourceList.h" #include "RenderView.h" @@ -34,6 +33,7 @@ struct SessionSnapshots { tinyxml2::XMLDocument *xmlDoc_; std::list keys_; + std::mutex access_; }; class Session diff --git a/src/SessionCreator.cpp b/src/SessionCreator.cpp index 2991461..b76b0d7 100644 --- a/src/SessionCreator.cpp +++ b/src/SessionCreator.cpp @@ -216,8 +216,10 @@ void SessionCreator::loadSnapshots(XMLElement *snapshotsNode) std::istringstream nodename( N->Name() ); nodename >> c >> id; + session_->snapshots()->access_.lock(); session_->snapshots()->keys_.push_back(id); session_->snapshots()->xmlDoc_->InsertEndChild( N->DeepClone(session_->snapshots()->xmlDoc_) ); + session_->snapshots()->access_.unlock(); } } } diff --git a/src/SessionVisitor.cpp b/src/SessionVisitor.cpp index 26629ba..f76ab1c 100644 --- a/src/SessionVisitor.cpp +++ b/src/SessionVisitor.cpp @@ -157,10 +157,17 @@ void SessionVisitor::saveSnapshots(tinyxml2::XMLDocument *doc, Session *session) { if (doc != nullptr && session != nullptr) { + // create element XMLElement *snapshots = doc->NewElement("Snapshots"); + + // access to session snapshot + session->snapshots()->access_.lock(); const XMLElement* N = session->snapshots()->xmlDoc_->FirstChildElement(); for( ; N ; N=N->NextSiblingElement()) snapshots->InsertEndChild( N->DeepClone( doc )); + session->snapshots()->access_.unlock(); + + // insert element doc->InsertEndChild(snapshots); } }