From c308dc2cf9673cbcacdc741241576c6d491d29a5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fran=C3=A7ois-Simon=20Fauteux-Chapleau?=
<francois-simon.fauteux-chapleau@savoirfairelinux.com>
Date: Thu, 23 Jan 2025 13:46:21 -0500
Subject: [PATCH] account: refactor unregistering logic
This patch removes the callback argument to the doUnregister function,
which wasn't really needed since it was always called right before
returning from the function.
The patch also removes all calls to the shutdownConnections function
outside of doUnregister, and instead adds a boolean argument to
doUnregister to allow the caller to specify if they want the connections
to be shut down. This is a preparatory step for fixing the bug described
in the GitLab issue linked below.
GitLab: #1095
Change-Id: Ic78659f2e00684fd1db91c0ee3cf894a53d558ce
---
src/account.h | 2 +-
src/client/configurationmanager.cpp | 13 +++++-----
src/jamidht/jamiaccount.cpp | 13 ++++------
src/jamidht/jamiaccount.h | 2 +-
src/manager.cpp | 37 ++++++++++-------------------
src/sip/sipaccount.cpp | 31 ++++++++++++------------
src/sip/sipaccount.h | 2 +-
7 files changed, 41 insertions(+), 59 deletions(-)
diff --git a/src/account.h b/src/account.h
index 5baf2bb48..3200da86d 100644
--- a/src/account.h
+++ b/src/account.h
@@ -166,7 +166,7 @@ public:
* Unregister the account.
* This should update the getRegistrationState() return value.
*/
- virtual void doUnregister(std::function<void(bool)> cb = std::function<void(bool)>()) = 0;
+ virtual void doUnregister(bool forceShutdownConnections = false) = 0;
RegistrationState getRegistrationState() const { return registrationState_; }
diff --git a/src/client/configurationmanager.cpp b/src/client/configurationmanager.cpp
index 245faab6f..7110b07af 100644
--- a/src/client/configurationmanager.cpp
+++ b/src/client/configurationmanager.cpp
@@ -968,13 +968,12 @@ setCredentials(const std::string& accountId,
const std::vector<std::map<std::string, std::string>>& details)
{
if (auto sipaccount = jami::Manager::instance().getAccount<SIPAccount>(accountId)) {
- sipaccount->doUnregister([&](bool /* transport_free */) {
- sipaccount->editConfig(
- [&](jami::SipAccountConfig& config) { config.setCredentials(details); });
- sipaccount->loadConfig();
- if (sipaccount->isEnabled())
- sipaccount->doRegister();
- });
+ sipaccount->doUnregister();
+ sipaccount->editConfig(
+ [&](jami::SipAccountConfig& config) { config.setCredentials(details); });
+ sipaccount->loadConfig();
+ if (sipaccount->isEnabled())
+ sipaccount->doRegister();
jami::Manager::instance().saveConfig(sipaccount);
}
}
diff --git a/src/jamidht/jamiaccount.cpp b/src/jamidht/jamiaccount.cpp
index a36ad6494..3c30fa561 100644
--- a/src/jamidht/jamiaccount.cpp
+++ b/src/jamidht/jamiaccount.cpp
@@ -2346,13 +2346,10 @@ JamiAccount::loadConversation(const std::string& convId)
}
void
-JamiAccount::doUnregister(std::function<void(bool)> released_cb)
+JamiAccount::doUnregister(bool forceShutdownConnections)
{
std::unique_lock<std::recursive_mutex> lock(configurationMutex_);
if (registrationState_ >= RegistrationState::ERROR_GENERIC) {
- lock.unlock();
- if (released_cb)
- released_cb(false);
return;
}
@@ -2381,9 +2378,9 @@ JamiAccount::doUnregister(std::function<void(bool)> released_cb)
}
// Stop all current P2P connections if account is disabled
- // Else, we let the system managing if the co is down or not
- // NOTE: this is used for changing account's config.
- if (not isEnabled())
+ // or if explicitly requested by the caller.
+ // NOTE: Leaving the connections open is useful when changing an account's config.
+ if (not isEnabled() || forceShutdownConnections)
shutdownConnections();
// Release current UPnP mapping if any.
@@ -2400,8 +2397,6 @@ JamiAccount::doUnregister(std::function<void(bool)> released_cb)
lock.unlock();
- if (released_cb)
- released_cb(false);
#ifdef ENABLE_PLUGIN
jami::Manager::instance().getJamiPluginManager().getChatServicesManager().cleanChatSubjects(
getAccountID());
diff --git a/src/jamidht/jamiaccount.h b/src/jamidht/jamiaccount.h
index ceda7125c..c6c00ef61 100644
--- a/src/jamidht/jamiaccount.h
+++ b/src/jamidht/jamiaccount.h
@@ -177,7 +177,7 @@ public:
/**
* Disconnect from the DHT.
*/
- void doUnregister(std::function<void(bool)> cb = {}) override;
+ void doUnregister(bool forceShutdownConnections = false) override;
/**
* Set the registration state of the specified link
diff --git a/src/manager.cpp b/src/manager.cpp
index 48b3ed9ea..2f2e4bdc7 100644
--- a/src/manager.cpp
+++ b/src/manager.cpp
@@ -1070,12 +1070,7 @@ Manager::unregisterAccounts()
{
for (const auto& account : getAllAccounts()) {
if (account->isEnabled()) {
- if (auto acc = std::dynamic_pointer_cast<JamiAccount>(account)) {
- // Note: shutdown the connections as doUnregister will not do it (because the
- // account is enabled)
- acc->shutdownConnections();
- }
- account->doUnregister();
+ account->doUnregister(true);
}
}
}
@@ -2751,17 +2746,17 @@ Manager::setAccountDetails(const std::string& accountID,
return;
// Unregister before modifying any account information
- account->doUnregister([&](bool /* transport_free */) {
- account->setAccountDetails(details);
+ account->doUnregister();
- if (account->isUsable())
- account->doRegister();
- else
- account->doUnregister();
+ account->setAccountDetails(details);
- // Update account details to the client side
- emitSignal<libjami::ConfigurationSignal::AccountDetailsChanged>(accountID, details);
- });
+ if (account->isUsable())
+ account->doRegister();
+ else
+ account->doUnregister();
+
+ // Update account details to the client side
+ emitSignal<libjami::ConfigurationSignal::AccountDetailsChanged>(accountID, details);
}
std::mt19937_64
@@ -2821,13 +2816,10 @@ Manager::removeAccount(const std::string& accountID, bool flush)
{
// Get it down and dying
if (const auto& remAccount = getAccount(accountID)) {
- // Force stopping connection before doUnregister as it will
- // wait for dht threads to finish
if (auto acc = std::dynamic_pointer_cast<JamiAccount>(remAccount)) {
acc->hangupCalls();
- acc->shutdownConnections();
}
- remAccount->doUnregister();
+ remAccount->doUnregister(true);
if (flush)
remAccount->flush();
accountFactory.removeAccount(*remAccount);
@@ -3025,12 +3017,7 @@ Manager::setAccountActive(const std::string& accountID, bool active, bool shutdo
if (active) {
acc->doRegister();
} else {
- acc->doUnregister();
- if (shutdownConnections) {
- if (auto jamiAcc = std::dynamic_pointer_cast<JamiAccount>(acc)) {
- jamiAcc->shutdownConnections();
- }
- }
+ acc->doUnregister(shutdownConnections);
}
}
emitSignal<libjami::ConfigurationSignal::VolatileDetailsChanged>(
diff --git a/src/sip/sipaccount.cpp b/src/sip/sipaccount.cpp
index e0a8b0a57..13cf63204 100644
--- a/src/sip/sipaccount.cpp
+++ b/src/sip/sipaccount.cpp
@@ -583,8 +583,10 @@ SIPAccount::setPushNotificationToken(const std::string& pushDeviceToken)
{
JAMI_WARNING("[SIP Account {}] setPushNotificationToken: {}", getAccountID(), pushDeviceToken);
if (SIPAccountBase::setPushNotificationToken(pushDeviceToken)) {
- if (config().enabled)
- doUnregister([&](bool /* transport_free */) { doRegister(); });
+ if (config().enabled) {
+ doUnregister();
+ doRegister();
+ }
return true;
}
return false;
@@ -594,8 +596,10 @@ bool
SIPAccount::setPushNotificationConfig(const std::map<std::string, std::string>& data)
{
if (SIPAccountBase::setPushNotificationConfig(data)) {
- if (config().enabled)
- doUnregister([&](bool /* transport_free */) { doRegister(); });
+ if (config().enabled) {
+ doUnregister();
+ doRegister();
+ }
return true;
}
return false;
@@ -607,8 +611,10 @@ SIPAccount::pushNotificationReceived(const std::string& from,
{
JAMI_WARNING("[SIP Account {:s}] pushNotificationReceived: {:s}", getAccountID(), from);
- if (config().enabled)
- doUnregister([&](bool /* transport_free */) { doRegister(); });
+ if (config().enabled) {
+ doUnregister();
+ doRegister();
+ }
}
void
@@ -744,7 +750,7 @@ SIPAccount::doRegister2_()
}
void
-SIPAccount::doUnregister(std::function<void(bool)> released_cb)
+SIPAccount::doUnregister(bool /* forceShutdownConnections */)
{
std::unique_lock<std::recursive_mutex> lock(configurationMutex_);
@@ -761,10 +767,6 @@ SIPAccount::doUnregister(std::function<void(bool)> released_cb)
if (transport_)
setTransport();
resetAutoRegistration();
-
- lock.unlock();
- if (released_cb)
- released_cb(not isIP2IP());
}
void
@@ -775,10 +777,9 @@ SIPAccount::connectivityChanged()
return;
}
- doUnregister([acc = shared()](bool /* transport_free */) {
- if (acc->isUsable())
- acc->doRegister();
- });
+ doUnregister();
+ if (isUsable())
+ doRegister();
}
void
diff --git a/src/sip/sipaccount.h b/src/sip/sipaccount.h
index 237e65c19..5caabcc81 100644
--- a/src/sip/sipaccount.h
+++ b/src/sip/sipaccount.h
@@ -140,7 +140,7 @@ public:
/**
* Send unregistration.
*/
- void doUnregister(std::function<void(bool)> cb = std::function<void(bool)>()) override;
+ void doUnregister(bool forceShutdownConnections = false) override;
/**
* Build and send SIP registration request
--
GitLab