From 3243334bfad42a8d3fc83d40556f39abeecad57f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Blin?=
<sebastien.blin@savoirfairelinux.com>
Date: Thu, 13 Jul 2023 11:43:25 -0400
Subject: [PATCH] conversation: use same lock for active/hosted calls
and avoid deadlock while announcing ended calls because convModule()
is already locked
Change-Id: I3430afc6aa8cafba151a52313c3904cdfd9013fe
---
src/jamidht/conversation.cpp | 52 +++++++++++++----------------
src/jamidht/conversation.h | 2 +-
src/jamidht/conversation_module.cpp | 2 +-
3 files changed, 25 insertions(+), 31 deletions(-)
diff --git a/src/jamidht/conversation.cpp b/src/jamidht/conversation.cpp
index 5f5a9b0c16..f8ce212dc7 100644
--- a/src/jamidht/conversation.cpp
+++ b/src/jamidht/conversation.cpp
@@ -216,13 +216,6 @@ public:
}
mutable std::string fmtStr_;
- /**
- * If, for whatever reason, the daemon is stopped while hosting a conference,
- * we need to announce the end of this call when restarting.
- * To avoid to keep active calls forever.
- */
- std::vector<std::string> announceEndedCalls();
-
~Impl()
{
try {
@@ -233,7 +226,12 @@ public:
}
}
- std::vector<std::string> refreshActiveCalls();
+ /**
+ * If, for whatever reason, the daemon is stopped while hosting a conference,
+ * we need to announce the end of this call when restarting.
+ * To avoid to keep active calls forever.
+ */
+ std::vector<std::string> commitsEndedCalls();
bool isAdmin() const;
std::string repoPath() const;
@@ -536,7 +534,7 @@ public:
auto file = fileutils::loadFile(hostedCallsPath_);
// load values
msgpack::object_handle oh = msgpack::unpack((const char*) file.data(), file.size());
- std::lock_guard<std::mutex> lk {hostedCallsMtx_};
+ std::lock_guard<std::mutex> lk {activeCallsMtx_};
oh.get().convert(hostedCalls_);
} catch (const std::exception& e) {
return;
@@ -629,7 +627,6 @@ public:
// Manage hosted calls on this device
std::string hostedCallsPath_ {};
- mutable std::mutex hostedCallsMtx_ {};
mutable std::map<std::string, uint64_t /* start time */> hostedCalls_ {};
// Manage active calls for this conversation (can be hosted by other devices)
std::string activeCallsPath_ {};
@@ -687,15 +684,7 @@ Conversation::Impl::getMembers(bool includeInvited, bool includeLeft, bool inclu
}
std::vector<std::string>
-Conversation::Impl::refreshActiveCalls()
-{
- loadActiveCalls();
- loadHostedCalls();
- return announceEndedCalls();
-}
-
-std::vector<std::string>
-Conversation::Impl::announceEndedCalls()
+Conversation::Impl::commitsEndedCalls()
{
auto shared = account_.lock();
// Handle current calls
@@ -733,11 +722,6 @@ Conversation::Impl::announceEndedCalls()
hostedCalls_.clear();
saveActiveCalls();
saveHostedCalls();
- lkA.unlock();
- lk.unlock();
- if (!commits.empty()) {
- announce(commits);
- }
return commits;
}
@@ -1915,9 +1899,19 @@ Conversation::bootstrap(std::function<void()> onBootstraped, const std::vector<D
}
std::vector<std::string>
-Conversation::refreshActiveCalls()
+Conversation::commitsEndedCalls()
{
- return pimpl_->refreshActiveCalls();
+ pimpl_->loadActiveCalls();
+ pimpl_->loadHostedCalls();
+ auto commits = pimpl_->commitsEndedCalls();
+ if (!commits.empty()) {
+ // Announce to client
+ dht::ThreadPool::io().run([w = weak(), commits]{
+ if (auto sthis = w.lock())
+ sthis->pimpl_->announce(commits);
+ });
+ }
+ return commits;
}
void
@@ -1997,7 +1991,7 @@ Conversation::hostConference(Json::Value&& message, OnDoneCb&& cb)
auto now = std::chrono::system_clock::now();
auto nowSecs = std::chrono::duration_cast<std::chrono::seconds>(now.time_since_epoch()).count();
{
- std::lock_guard<std::mutex> lk(pimpl_->hostedCallsMtx_);
+ std::lock_guard<std::mutex> lk(pimpl_->activeCallsMtx_);
pimpl_->hostedCalls_[message["confId"].asString()] = nowSecs;
pimpl_->saveHostedCalls();
}
@@ -2014,7 +2008,7 @@ Conversation::isHosting(const std::string& confId) const
auto info = infos();
if (info["rdvDevice"] == shared->currentDeviceId() && info["rdvHost"] == shared->getUsername())
return true; // We are the current device Host
- std::lock_guard<std::mutex> lk(pimpl_->hostedCallsMtx_);
+ std::lock_guard<std::mutex> lk(pimpl_->activeCallsMtx_);
return pimpl_->hostedCalls_.find(confId) != pimpl_->hostedCalls_.end();
}
@@ -2028,7 +2022,7 @@ Conversation::removeActiveConference(Json::Value&& message, OnDoneCb&& cb)
auto erased = false;
{
- std::lock_guard<std::mutex> lk(pimpl_->hostedCallsMtx_);
+ std::lock_guard<std::mutex> lk(pimpl_->activeCallsMtx_);
erased = pimpl_->hostedCalls_.erase(message["confId"].asString());
}
if (erased) {
diff --git a/src/jamidht/conversation.h b/src/jamidht/conversation.h
index 96d778ea88..387d18b1ad 100644
--- a/src/jamidht/conversation.h
+++ b/src/jamidht/conversation.h
@@ -171,7 +171,7 @@ public:
* and commit all the crashed calls
* @return Commits added
*/
- std::vector<std::string> refreshActiveCalls();
+ std::vector<std::string> commitsEndedCalls();
/**
* Add a callback to update upper layers
diff --git a/src/jamidht/conversation_module.cpp b/src/jamidht/conversation_module.cpp
index 8919f1831d..73efdc0fb1 100644
--- a/src/jamidht/conversation_module.cpp
+++ b/src/jamidht/conversation_module.cpp
@@ -1184,7 +1184,7 @@ ConversationModule::loadConversations()
conv->setRemovingFlag();
toRm.insert(repository);
}
- auto commits = conv->refreshActiveCalls();
+ auto commits = conv->commitsEndedCalls();
if (!commits.empty()) {
// Note: here, this means that some calls were actives while the
// daemon finished (can be a crash).
--
GitLab