From 6b729426ec3ad561a2db18105a10dabc19f415bc Mon Sep 17 00:00:00 2001 From: peter-esik <67009121+peter-esik@users.noreply.github.com> Date: Wed, 19 Jun 2024 15:48:23 +0200 Subject: [PATCH 1/3] Fix crash due to incorrect condition variable usage - There was a race condition on the condition variable, when ChangeList::Flush called notify_all. After the waiter (in this case, the main thread) is woken up, notify_all might not have returned just yet. If the main thread proceeded and called Clear on the ChangeList object in the meantime, the condition variable is deleted when notify_all is still executing, causing a crash. This occurrs frequently on my machine when running the tool in "branching mode", and lots of "uninteresting" branches (in other words: changelists with 0 files to be committed). - As part of the bugfix, synchronization in the ChangeList class is reworked: instead of multiple atomic variables, mutexes, and condition variables, a single condition variable and mutex is used, with a state machine. --- p4-fusion/commands/change_list.cc | 47 ++++++++++++++++--------------- p4-fusion/commands/change_list.h | 19 ++++++++----- 2 files changed, 37 insertions(+), 29 deletions(-) diff --git a/p4-fusion/commands/change_list.cc b/p4-fusion/commands/change_list.cc index 06646507..6fe0b7c0 100644 --- a/p4-fusion/commands/change_list.cc +++ b/p4-fusion/commands/change_list.cc @@ -20,6 +20,10 @@ ChangeList::ChangeList(const std::string& clNumber, const std::string& clDescrip , description(clDescription) , timestamp(clTimestamp) , changedFileGroups(ChangedFileGroups::Empty()) + , stateCV(new std::condition_variable ()) + , stateMutex(new std::mutex ()) + , filesDownloaded(-1) + , state(Initialized) { } @@ -48,11 +52,9 @@ void ChangeList::PrepareDownload(const BranchSet& branchSet) cl.changedFileGroups = branchSet.ParseAffectedFiles(describe.GetFileData()); } - { - std::unique_lock lock((*(cl.canDownloadMutex))); - *cl.canDownload = true; - } - cl.canDownloadCV->notify_all(); + std::unique_lock lock(*(cl.stateMutex)); + cl.state = Described; + cl.stateCV->notify_all(); }); } @@ -64,12 +66,12 @@ void ChangeList::StartDownload(const int& printBatch) { // Wait for describe to finish, if it is still running { - std::unique_lock lock(*(cl.canDownloadMutex)); - cl.canDownloadCV->wait(lock, [&cl]() - { return *(cl.canDownload) == true; }); + std::unique_lock lock(*cl.stateMutex); + cl.stateCV->wait(lock, [&cl]() + { return cl.state == Described; }); } - *cl.filesDownloaded = 0; + cl.filesDownloaded = 0; std::shared_ptr> printBatchFiles = std::make_shared>(); std::shared_ptr> printBatchFileData = std::make_shared>(); @@ -122,20 +124,23 @@ void ChangeList::Flush(std::shared_ptr> printBatchFiles { printBatchFileData->at(i)->MoveContentsOnceFrom(printData.GetPrintData().at(i).contents); } - - (*filesDownloaded) += printBatchFiles->size(); } - // Ensure the notify_all is called. - commitCV->notify_all(); + std::lock_guard lock(*stateMutex); + filesDownloaded += printBatchFiles->size(); + if (filesDownloaded == changedFileGroups->totalFileCount) + { + state = Downloaded; + stateCV->notify_all(); + } }); } void ChangeList::WaitForDownload() { - std::unique_lock lock(*commitMutex); - commitCV->wait(lock, [this]() - { return *(filesDownloaded) == (int)changedFileGroups->totalFileCount; }); + std::unique_lock lock(*stateMutex); + stateCV->wait(lock, [this]() + { return state == Downloaded; }); } void ChangeList::Clear() @@ -145,10 +150,8 @@ void ChangeList::Clear() description.clear(); changedFileGroups->Clear(); - filesDownloaded.reset(); - canDownload.reset(); - canDownloadMutex.reset(); - canDownloadCV.reset(); - commitMutex.reset(); - commitCV.reset(); + stateCV.release(); + stateMutex.release(); + filesDownloaded = -1; + state = Freed; } diff --git a/p4-fusion/commands/change_list.h b/p4-fusion/commands/change_list.h index d1bda7a7..1bc94c32 100644 --- a/p4-fusion/commands/change_list.h +++ b/p4-fusion/commands/change_list.h @@ -16,22 +16,27 @@ struct ChangeList { + enum State { + Initialized, + Described, + Downloaded, + Freed + }; + std::string number; std::string user; std::string description; int64_t timestamp = 0; std::unique_ptr changedFileGroups = ChangedFileGroups::Empty(); - std::shared_ptr> filesDownloaded = std::make_shared>(-1); - std::shared_ptr> canDownload = std::make_shared>(false); - std::shared_ptr canDownloadMutex = std::make_shared(); - std::shared_ptr canDownloadCV = std::make_shared(); - std::shared_ptr commitMutex = std::make_shared(); - std::shared_ptr commitCV = std::make_shared(); + std::unique_ptr stateCV; + std::unique_ptr stateMutex; + int filesDownloaded; + State state; ChangeList(const std::string& number, const std::string& description, const std::string& user, const int64_t& timestamp); - ChangeList(const ChangeList& other) = default; + ChangeList(const ChangeList& other) = delete; ChangeList& operator=(const ChangeList&) = delete; ChangeList(ChangeList&&) = default; ChangeList& operator=(ChangeList&&) = default; From 6a8fbe93a1f3d884e6de0487ab656ba5ed96c83f Mon Sep 17 00:00:00 2001 From: peter-esik <67009121+peter-esik@users.noreply.github.com> Date: Thu, 20 Jun 2024 09:48:00 +0200 Subject: [PATCH 2/3] Formatting fixes --- p4-fusion/commands/change_list.cc | 24 ++++++++++++------------ p4-fusion/commands/change_list.h | 3 ++- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/p4-fusion/commands/change_list.cc b/p4-fusion/commands/change_list.cc index 6fe0b7c0..86e4644f 100644 --- a/p4-fusion/commands/change_list.cc +++ b/p4-fusion/commands/change_list.cc @@ -20,8 +20,8 @@ ChangeList::ChangeList(const std::string& clNumber, const std::string& clDescrip , description(clDescription) , timestamp(clTimestamp) , changedFileGroups(ChangedFileGroups::Empty()) - , stateCV(new std::condition_variable ()) - , stateMutex(new std::mutex ()) + , stateCV(new std::condition_variable()) + , stateMutex(new std::mutex()) , filesDownloaded(-1) , state(Initialized) { @@ -52,9 +52,9 @@ void ChangeList::PrepareDownload(const BranchSet& branchSet) cl.changedFileGroups = branchSet.ParseAffectedFiles(describe.GetFileData()); } - std::unique_lock lock(*(cl.stateMutex)); - cl.state = Described; - cl.stateCV->notify_all(); + std::unique_lock lock(*cl.stateMutex); + cl.state = Described; + cl.stateCV->notify_all(); }); } @@ -126,13 +126,13 @@ void ChangeList::Flush(std::shared_ptr> printBatchFiles } } - std::lock_guard lock(*stateMutex); - filesDownloaded += printBatchFiles->size(); - if (filesDownloaded == changedFileGroups->totalFileCount) - { - state = Downloaded; - stateCV->notify_all(); - } + std::lock_guard lock(*stateMutex); + filesDownloaded += printBatchFiles->size(); + if (filesDownloaded == changedFileGroups->totalFileCount) + { + state = Downloaded; + stateCV->notify_all(); + } }); } diff --git a/p4-fusion/commands/change_list.h b/p4-fusion/commands/change_list.h index 1bc94c32..d143036c 100644 --- a/p4-fusion/commands/change_list.h +++ b/p4-fusion/commands/change_list.h @@ -16,7 +16,8 @@ struct ChangeList { - enum State { + enum State + { Initialized, Described, Downloaded, From 8817dde2f0190d674ad1383189b6b9b9f54594d1 Mon Sep 17 00:00:00 2001 From: peter-esik <67009121+peter-esik@users.noreply.github.com> Date: Thu, 20 Jun 2024 09:58:28 +0200 Subject: [PATCH 3/3] Even more formatting fixes --- p4-fusion/commands/change_list.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/p4-fusion/commands/change_list.cc b/p4-fusion/commands/change_list.cc index 86e4644f..3bdf8c16 100644 --- a/p4-fusion/commands/change_list.cc +++ b/p4-fusion/commands/change_list.cc @@ -20,10 +20,10 @@ ChangeList::ChangeList(const std::string& clNumber, const std::string& clDescrip , description(clDescription) , timestamp(clTimestamp) , changedFileGroups(ChangedFileGroups::Empty()) - , stateCV(new std::condition_variable()) - , stateMutex(new std::mutex()) - , filesDownloaded(-1) - , state(Initialized) + , stateCV(new std::condition_variable()) + , stateMutex(new std::mutex()) + , filesDownloaded(-1) + , state(Initialized) { }