From 2a805d24fe5af11da527ed68086c093e6daf008a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fran=C3=A7ois-Simon=20Fauteux-Chapleau?=
<francois-simon.fauteux-chapleau@savoirfairelinux.com>
Date: Mon, 15 Jul 2024 14:36:12 -0400
Subject: [PATCH] jamiaccount: fix deadlock
This patch moves the updateConvForContact function from JamiAccount to
ConversationModule. This fixes a deadlock that sometimes occurred when
starting Jami.
GitLab: #1033
Change-Id: I441426fa339e5cfb327566df6132bfecb206bd1e
---
src/jamidht/conversation_module.cpp | 50 +++++++++++++++------
src/jamidht/conversation_module.h | 13 +++++-
src/jamidht/jamiaccount.cpp | 31 +------------
src/jamidht/jamiaccount.h | 11 -----
test/unitTest/conversation/conversation.cpp | 2 +-
5 files changed, 50 insertions(+), 57 deletions(-)
diff --git a/src/jamidht/conversation_module.cpp b/src/jamidht/conversation_module.cpp
index 1a9076ea1c..c5c39115d3 100644
--- a/src/jamidht/conversation_module.cpp
+++ b/src/jamidht/conversation_module.cpp
@@ -128,7 +128,6 @@ public:
SengMsgCb&& sendMsgCb,
NeedSocketCb&& onNeedSocket,
NeedSocketCb&& onNeedSwarmSocket,
- UpdateConvReq&& updateConvReqCb,
OneToOneRecvCb&& oneToOneRecvCb);
template<typename S, typename T>
@@ -248,6 +247,11 @@ public:
std::string getOneToOneConversation(const std::string& uri) const noexcept;
+ bool updateConvForContact(const std::string& uri,
+ const std::string& oldConv,
+ const std::string& newConv);
+
+
std::shared_ptr<SyncedConversation> getConversation(std::string_view convId) const
{
std::lock_guard lk(conversationsMtx_);
@@ -373,7 +377,6 @@ public:
SengMsgCb sendMsgCb_;
NeedSocketCb onNeedSocket_;
NeedSocketCb onNeedSwarmSocket_;
- UpdateConvReq updateConvReqCb_;
OneToOneRecvCb oneToOneRecvCb_;
std::string accountId_ {};
@@ -457,7 +460,6 @@ ConversationModule::Impl::Impl(std::shared_ptr<JamiAccount>&& account,
SengMsgCb&& sendMsgCb,
NeedSocketCb&& onNeedSocket,
NeedSocketCb&& onNeedSwarmSocket,
- UpdateConvReq&& updateConvReqCb,
OneToOneRecvCb&& oneToOneRecvCb)
: account_(account)
, accountManager_(accountManager)
@@ -465,7 +467,6 @@ ConversationModule::Impl::Impl(std::shared_ptr<JamiAccount>&& account,
, sendMsgCb_(sendMsgCb)
, onNeedSocket_(onNeedSocket)
, onNeedSwarmSocket_(onNeedSwarmSocket)
- , updateConvReqCb_(updateConvReqCb)
, oneToOneRecvCb_(oneToOneRecvCb)
, accountId_(account->getAccountID())
{
@@ -907,6 +908,25 @@ ConversationModule::Impl::getOneToOneConversation(const std::string& uri) const
return {};
}
+bool
+ConversationModule::Impl::updateConvForContact(const std::string& uri,
+ const std::string& oldConv,
+ const std::string& newConv)
+{
+ if (newConv != oldConv) {
+ auto conversation = getOneToOneConversation(uri);
+ if (conversation != oldConv) {
+ JAMI_DEBUG("Old conversation is not found in details {} - found: {}",
+ oldConv,
+ conversation);
+ return false;
+ }
+ accountManager_->updateContactConversation(uri, newConv);
+ return true;
+ }
+ return false;
+}
+
void
ConversationModule::Impl::declineOtherConversationWith(const std::string& uri) noexcept
{
@@ -1027,8 +1047,8 @@ ConversationModule::Impl::removeConversationImpl(SyncedConversation& conv)
}
} else {
for (const auto& m : members)
- if (username_ != m.at("uri") && updateConvReqCb_)
- updateConvReqCb_(conv.info.id, m.at("uri"), false);
+ if (username_ != m.at("uri"))
+ updateConvForContact(m.at("uri"), conv.info.id, "");
}
// Else we are the last member, so we can remove
removeRepositoryImpl(conv, true);
@@ -1253,7 +1273,7 @@ ConversationModule::Impl::fixStructures(
const std::set<std::string>& toRm)
{
for (const auto& [uri, oldConv, newConv] : updateContactConv) {
- acc->updateConvForContact(uri, oldConv, newConv);
+ updateConvForContact(uri, oldConv, newConv);
}
////////////////////////////////////////////////////////////////
// Note: This is only to homogeneize trust and convRequests
@@ -1491,7 +1511,6 @@ ConversationModule::ConversationModule(std::shared_ptr<JamiAccount> account,
SengMsgCb&& sendMsgCb,
NeedSocketCb&& onNeedSocket,
NeedSocketCb&& onNeedSwarmSocket,
- UpdateConvReq&& updateConvReqCb,
OneToOneRecvCb&& oneToOneRecvCb,
bool autoLoadConversations)
: pimpl_ {std::make_unique<Impl>(std::move(account),
@@ -1500,7 +1519,6 @@ ConversationModule::ConversationModule(std::shared_ptr<JamiAccount> account,
std::move(sendMsgCb),
std::move(onNeedSocket),
std::move(onNeedSwarmSocket),
- std::move(updateConvReqCb),
std::move(oneToOneRecvCb))}
{
if (autoLoadConversations) {
@@ -1846,6 +1864,14 @@ ConversationModule::getOneToOneConversation(const std::string& uri) const noexce
return pimpl_->getOneToOneConversation(uri);
}
+bool
+ConversationModule::updateConvForContact(const std::string& uri,
+ const std::string& oldConv,
+ const std::string& newConv)
+{
+ return pimpl_->updateConvForContact(uri, oldConv, newConv);
+}
+
std::vector<std::map<std::string, std::string>>
ConversationModule::getConversationRequests() const
{
@@ -2003,8 +2029,7 @@ ConversationModule::acceptConversationRequest(const std::string& conversationId,
}
pimpl_->rmConversationRequest(conversationId);
lkCr.unlock();
- if (pimpl_->updateConvReqCb_)
- pimpl_->updateConvReqCb_(conversationId, request->from, true);
+ pimpl_->accountManager_->acceptTrustRequest(request->from, true);
cloneConversationFrom(conversationId, request->from);
}
@@ -2816,8 +2841,7 @@ ConversationModule::removeContact(const std::string& uri, bool banned)
auto isSelf = uri == pimpl_->username_;
std::vector<std::string> toRm;
auto updateClient = [&](const auto& convId) {
- if (pimpl_->updateConvReqCb_)
- pimpl_->updateConvReqCb_(convId, uri, false);
+ pimpl_->updateConvForContact(uri, convId, "");
emitSignal<libjami::ConversationSignal::ConversationRemoved>(pimpl_->accountId_, convId);
};
auto removeConvInfo = [&](const auto& conv, const auto& members) {
diff --git a/src/jamidht/conversation_module.h b/src/jamidht/conversation_module.h
index 3f89f1b30d..bfbd9462b9 100644
--- a/src/jamidht/conversation_module.h
+++ b/src/jamidht/conversation_module.h
@@ -66,7 +66,6 @@ using NeedSocketCb
using SengMsgCb = std::function<
uint64_t(const std::string&, const DeviceId&, std::map<std::string, std::string>, uint64_t)>;
using NeedsSyncingCb = std::function<void(std::shared_ptr<SyncMsg>&&)>;
-using UpdateConvReq = std::function<void(const std::string&, const std::string&, bool)>;
using OneToOneRecvCb = std::function<void(const std::string&, const std::string&)>;
class ConversationModule
@@ -78,7 +77,6 @@ public:
SengMsgCb&& sendMsgCb,
NeedSocketCb&& onNeedSocket,
NeedSocketCb&& onNeedSwarmSocket,
- UpdateConvReq&& updateConvReqCb,
OneToOneRecvCb&& oneToOneRecvCb,
bool autoLoadConversations = true);
~ConversationModule() = default;
@@ -125,6 +123,17 @@ public:
*/
std::string getOneToOneConversation(const std::string& uri) const noexcept;
+ /**
+ * Replace linked conversation in contact's details
+ * @param uri Of the contact
+ * @param oldConv Current conversation
+ * @param newConv
+ * @return if replaced
+ */
+ bool updateConvForContact(const std::string& uri,
+ const std::string& oldConv,
+ const std::string& newConv);
+
/**
* Return conversation's requests
*/
diff --git a/src/jamidht/jamiaccount.cpp b/src/jamidht/jamiaccount.cpp
index 5638828da0..0112f6738c 100644
--- a/src/jamidht/jamiaccount.cpp
+++ b/src/jamidht/jamiaccount.cpp
@@ -1208,7 +1208,7 @@ JamiAccount::loadAccount(const std::string& archive_password_scheme,
// TODO: In the future, we may want to re-commit the messages we
// may have send in the request we sent.
if (oldConv != convFromReq
- && shared->updateConvForContact(uri, oldConv, convFromReq)) {
+ && cm->updateConvForContact(uri, oldConv, convFromReq)) {
cm->initReplay(oldConv, convFromReq);
cm->cloneConversationFrom(convFromReq, uri, oldConv);
}
@@ -2268,15 +2268,6 @@ JamiAccount::convModule(bool noCreation)
}
});
},
- [this](auto&& convId, auto&& contactUri, bool accept) {
- // NOTE: do not reschedule as the conversation's requests
- // should be synched with trust requests
- if (accept) {
- accountManager_->acceptTrustRequest(contactUri, true);
- } else {
- updateConvForContact(contactUri, convId, "");
- }
- },
[this](auto&& convId, auto&& from) {
accountManager_
->findCertificate(dht::InfoHash(from),
@@ -2815,26 +2806,6 @@ JamiAccount::removeContact(const std::string& uri, bool ban)
JAMI_WARNING("[Account {}] removeContact: account not loaded", getAccountID());
}
-bool
-JamiAccount::updateConvForContact(const std::string& uri,
- const std::string& oldConv,
- const std::string& newConv)
-{
- auto cm = convModule(false);
- if (newConv != oldConv && cm) {
- auto conversation = cm->getOneToOneConversation(uri);
- if (conversation != oldConv) {
- JAMI_DEBUG("Old conversation is not found in details {} - found: {}",
- oldConv,
- conversation);
- return false;
- }
- accountManager_->updateContactConversation(uri, newConv);
- return true;
- }
- return false;
-}
-
std::map<std::string, std::string>
JamiAccount::getContactDetails(const std::string& uri) const
{
diff --git a/src/jamidht/jamiaccount.h b/src/jamidht/jamiaccount.h
index e0b533ff90..347deb409a 100644
--- a/src/jamidht/jamiaccount.h
+++ b/src/jamidht/jamiaccount.h
@@ -303,17 +303,6 @@ public:
void removeContact(const std::string& uri, bool banned = true);
std::vector<std::map<std::string, std::string>> getContacts(bool includeRemoved = false) const;
- /**
- * Replace in contact's details related conversation
- * @param uri Of the contact
- * @param oldConv Current conversation
- * @param newConv
- * @return if replaced
- */
- bool updateConvForContact(const std::string& uri,
- const std::string& oldConv,
- const std::string& newConv);
-
///
/// Obtain details about one account contact in serializable form.
///
diff --git a/test/unitTest/conversation/conversation.cpp b/test/unitTest/conversation/conversation.cpp
index 0979182f95..a9a0baef78 100644
--- a/test/unitTest/conversation/conversation.cpp
+++ b/test/unitTest/conversation/conversation.cpp
@@ -2145,7 +2145,7 @@ ConversationTest::testFixContactDetails()
auto details = aliceAccount->getContactDetails(bobUri);
CPPUNIT_ASSERT(details["conversationId"] == aliceData.conversationId);
// Erase convId from contact details, this should be fixed by next reload.
- CPPUNIT_ASSERT(aliceAccount->updateConvForContact(bobUri, aliceData.conversationId, ""));
+ CPPUNIT_ASSERT(aliceAccount->convModule()->updateConvForContact(bobUri, aliceData.conversationId, ""));
details = aliceAccount->getContactDetails(bobUri);
CPPUNIT_ASSERT(details["conversationId"].empty());
--
GitLab