diff --git a/daemon/src/account.h b/daemon/src/account.h index 0aabc35fb739378c39db94a3f2c2026b4eca2ee9..ce9e70a6245e7750b276f62f46cef0d7b287d0b8 100644 --- a/daemon/src/account.h +++ b/daemon/src/account.h @@ -32,14 +32,15 @@ #ifndef ACCOUNT_H #define ACCOUNT_H -#include <string> -#include <vector> - #include "noncopyable.h" #include "config/sfl_config.h" #include "config/serializable.h" #include "registration_states.h" +#include <functional> +#include <string> +#include <vector> + class Account; class VoIPLink; @@ -97,7 +98,7 @@ class Account : public Serializable { * Unregister the underlying VoIPLink. Stop the event listener. * This should update the getRegistrationState() return value. */ - virtual void unregisterVoIPLink() = 0; + virtual void unregisterVoIPLink(std::function<void(bool)> cb = std::function<void(bool)>()) = 0; /** * Tell if the account is enable or not. diff --git a/daemon/src/iax/iaxaccount.cpp b/daemon/src/iax/iaxaccount.cpp index f88c889f7d553adff84d4cb77d95de058eed64ad..d1a7cb52d0b320da57e5a14b8a4203484cdf3256 100644 --- a/daemon/src/iax/iaxaccount.cpp +++ b/daemon/src/iax/iaxaccount.cpp @@ -147,7 +147,7 @@ void IAXAccount::registerVoIPLink() } void -IAXAccount::unregisterVoIPLink() +IAXAccount::unregisterVoIPLink(std::function<void(bool)> cb) { try { link_.sendUnregister(*this); @@ -155,6 +155,8 @@ IAXAccount::unregisterVoIPLink() } catch (const VoipLinkException &e) { ERROR("IAXAccount: %s", e.what()); } + if (cb) + cb(true); } void diff --git a/daemon/src/iax/iaxaccount.h b/daemon/src/iax/iaxaccount.h index dc9bfd23f2e4ba7ba335c5a16b287d71f8ce6c9b..3947709aada841d5141bdb71b020fb2f5e10c910 100644 --- a/daemon/src/iax/iaxaccount.h +++ b/daemon/src/iax/iaxaccount.h @@ -54,7 +54,7 @@ class IAXAccount : public Account { void registerVoIPLink(); // Unregister an account - void unregisterVoIPLink(); + void unregisterVoIPLink(std::function<void(bool)> cb = std::function<void(bool)>()); std::string getPassword() const { return password_; diff --git a/daemon/src/iax/iaxvoiplink.cpp b/daemon/src/iax/iaxvoiplink.cpp index 0b3dad5d11b263225841ba64a082455731decf41..899898e68884a524ff8c76c74ad14ff3c200b61d 100644 --- a/daemon/src/iax/iaxvoiplink.cpp +++ b/daemon/src/iax/iaxvoiplink.cpp @@ -282,7 +282,7 @@ IAXVoIPLink::sendRegister(Account& a) } void -IAXVoIPLink::sendUnregister(Account& a) +IAXVoIPLink::sendUnregister(Account& a, std::function<void(bool)> cb) { if (regSession_) { std::lock_guard<std::mutex> lock(mutexIAX_); @@ -293,6 +293,9 @@ IAXVoIPLink::sendUnregister(Account& a) nextRefreshStamp_ = 0; static_cast<IAXAccount&>(a).setRegistrationState(RegistrationState::UNREGISTERED); + + if (cb) + cb(true); } Call* diff --git a/daemon/src/iax/iaxvoiplink.h b/daemon/src/iax/iaxvoiplink.h index 1d8d7e3fbb8d548d1d307263213899fcbc5e0cca..f4135f0e23f29847cd8f343dc8cd506abd24b2ff 100644 --- a/daemon/src/iax/iaxvoiplink.h +++ b/daemon/src/iax/iaxvoiplink.h @@ -109,7 +109,7 @@ class IAXVoIPLink : public VoIPLink { * @todo Send an IAX_COMMAND_REGREL to force unregistration upstream. * Urgency: low */ - virtual void sendUnregister(Account& a); + virtual void sendUnregister(Account& a, std::function<void(bool)> cb = std::function<void(bool)>()); /** * Create a new outgoing call diff --git a/daemon/src/managerimpl.cpp b/daemon/src/managerimpl.cpp index d054c52483d0c98db8b9c894ce0dbbdbca79a26a..e9b388e9be42a034f6d067a93cb5ea5e0af83d82 100644 --- a/daemon/src/managerimpl.cpp +++ b/daemon/src/managerimpl.cpp @@ -2377,19 +2377,19 @@ void ManagerImpl::setAccountDetails(const std::string& accountID, return; // Unregister before modifying any account information - account->unregisterVoIPLink(); - account->setAccountDetails(details); - - // Serialize configuration to disk once it is done - saveConfig(); + account->unregisterVoIPLink([&](bool /* transport_free */) { + account->setAccountDetails(details); + // Serialize configuration to disk once it is done + saveConfig(); - if (account->isEnabled()) - account->registerVoIPLink(); - else - account->unregisterVoIPLink(); + if (account->isEnabled()) + account->registerVoIPLink(); + else + account->unregisterVoIPLink(); - // Update account details to the client side - client_.getConfigurationManager()->accountsChanged(); + // Update account details to the client side + client_.getConfigurationManager()->accountsChanged(); + }); } std::string diff --git a/daemon/src/sip/sipaccount.cpp b/daemon/src/sip/sipaccount.cpp index bc9292c4e08560db92f49344625dc5b087d6a51e..a059b847093172e8e2c692b39c94a1ec884511c1 100644 --- a/daemon/src/sip/sipaccount.cpp +++ b/daemon/src/sip/sipaccount.cpp @@ -156,10 +156,7 @@ SIPAccount::SIPAccount(const std::string& accountID, bool presenceEnabled) SIPAccount::~SIPAccount() { - if (transport_) { - pjsip_transport_dec_ref(transport_); - transport_ = nullptr; - } + setTransport(); #ifdef SFL_PRESENCE delete presence_; @@ -651,6 +648,8 @@ void parseInt(const std::map<std::string, std::string> &details, const char *key void SIPAccount::setAccountDetails(const std::map<std::string, std::string> &details) { + ERROR("SIPAccount::setAccountDetails"); + // Account setting common to SIP and IAX parseString(details, CONFIG_ACCOUNT_ALIAS, alias_); parseString(details, CONFIG_ACCOUNT_USERNAME, username_); @@ -950,15 +949,21 @@ void SIPAccount::registerVoIPLink() #endif } -void SIPAccount::unregisterVoIPLink() +void SIPAccount::unregisterVoIPLink(std::function<void(bool)> released_cb) { - if (isIP2IP()) + if (isIP2IP()) { + if (released_cb) + released_cb(false); return; + } try { - link_.sendUnregister(*this); + link_.sendUnregister(*this, released_cb); } catch (const VoipLinkException &e) { - ERROR("%s", e.what()); + ERROR("SIPAccount::unregisterVoIPLink %s", e.what()); + setTransport(); + if (released_cb) + released_cb(false); } } @@ -1377,7 +1382,6 @@ SIPAccount::setTransport(pjsip_transport* transport, pjsip_tpfactory* lis) if (regc_) pjsip_regc_release_transport(regc_); pjsip_transport_dec_ref(transport_); - DEBUG("Transport %s has count %d", transport_->info, pj_atomic_get(transport_->ref_cnt)); } if (tlsListener_ && tlsListener_ != lis) tlsListener_->destroy(tlsListener_); @@ -1672,6 +1676,7 @@ SIPAccount::generateVideoPort() const void SIPAccount::destroyRegistrationInfo() { + if (!regc_) return; pjsip_regc_destroy(regc_); regc_ = nullptr; } diff --git a/daemon/src/sip/sipaccount.h b/daemon/src/sip/sipaccount.h index 12b6ad4de32caa5ba7c152a9b008403772bbcd2c..9ef6d04cab741d0c172ace15cf3e709413258ad6 100644 --- a/daemon/src/sip/sipaccount.h +++ b/daemon/src/sip/sipaccount.h @@ -207,9 +207,9 @@ class SIPAccount : public Account { void registerVoIPLink(); /** - * Send unregistration and clean all related stuff ( calls , thread ) + * Send unregistration. */ - void unregisterVoIPLink(); + void unregisterVoIPLink(std::function<void(bool)> cb = std::function<void(bool)>()); /** * Start the keep alive function, once started, the account will be registered periodically @@ -367,7 +367,7 @@ class SIPAccount : public Account { return stunEnabled_; } - /* + /** * @return pj_str_t "From" uri based on account information. * From RFC3261: "The To header field first and foremost specifies the desired * logical" recipient of the request, or the address-of-record of the @@ -378,7 +378,7 @@ class SIPAccount : public Account { */ std::string getFromUri() const; - /* + /** * This method adds the correct scheme, hostname and append * the ;transport= parameter at the end of the uri, in accordance with RFC3261. * It is expected that "port" is present in the internal hostname_. @@ -388,7 +388,7 @@ class SIPAccount : public Account { */ std::string getToUri(const std::string& username) const; - /* + /** * In the current version of SFLPhone, "srv" uri is obtained in the preformated * way: hostname:port. This method adds the correct scheme and append * the ;transport= parameter at the end of the uri, in accordance with RFC3261. diff --git a/daemon/src/sip/siptransport.cpp b/daemon/src/sip/siptransport.cpp index 93e6df66ae22c6b9441cb26688e98d58e516efe1..bc87f8051fbc8dab78b7c7180c1b6b5a3937398a 100644 --- a/daemon/src/sip/siptransport.cpp +++ b/daemon/src/sip/siptransport.cpp @@ -37,6 +37,7 @@ #include "manager.h" #include "client/configurationmanager.h" +#include "map_utils.h" #include "ip_utils.h" #include <pjsip.h> @@ -55,8 +56,8 @@ #define RETURN_IF_FAIL(A, VAL, M, ...) if (!(A)) { ERROR(M, ##__VA_ARGS__); return (VAL); } -SipTransport::SipTransport(pjsip_endpoint *endpt, pj_caching_pool& cp, pj_pool_t& pool, std::function<void(pjsip_transport*)> transportDestroyed) : -transportMap_(), transportDestroyedCb_(transportDestroyed), cp_(cp), pool_(pool), endpt_(endpt) +SipTransport::SipTransport(pjsip_endpoint *endpt, pj_caching_pool& cp, pj_pool_t& pool) : +transportMap_(), transportMapMutex_(), transportDestroyedCv_(), cp_(cp), pool_(pool), endpt_(endpt) { auto status = pjsip_tpmgr_set_state_cb(pjsip_endpt_get_tpmgr(endpt_), SipTransport::tp_state_callback); if (status != PJ_SUCCESS) { @@ -91,68 +92,97 @@ SipTransport::tp_state_callback(pjsip_transport *tp, pjsip_transport_state state void SipTransport::transportStateChanged(pjsip_transport* tp, pjsip_transport_state state) { - WARN("SipTransport::transportStateChanged: {%s} is now in state %d", tp->info, state); - std::map<std::string, pjsip_transport*>::const_iterator transport_key = transportMap_.cend(); - for (auto i = transportMap_.cbegin(); i != transportMap_.cend(); ++i) { - if (i->second == tp) { - transport_key = i; - break; - } - } - if (transport_key == transportMap_.cend()) { - ERROR("Transport not found."); + std::lock_guard<std::mutex> lock(transportMapMutex_); + auto transport_key = map_utils::findByValue(transportMap_, tp); + if (transport_key == transportMap_.cend()) return; - } - if (state == PJSIP_TP_STATE_SHUTDOWN || state == PJSIP_TP_STATE_DESTROY) { - if (transportDestroyedCb_) - transportDestroyedCb_(transport_key->second); + WARN("Transport was destroyed: {%s}", tp->info); transportMap_.erase(transport_key++); + transportDestroyedCv_.notify_all(); + } +} + +void +SipTransport::waitForReleased(pjsip_transport* tp, std::function<void(bool)> released_cb) +{ + if (!released_cb) + return; + if (!tp) { + released_cb(true); + return; + } + std::vector<pjsip_transport*> to_destroy_all; + bool destroyed = false; + { + std::unique_lock<std::mutex> lock(transportMapMutex_); + auto check_destroyed = [&](){ + std::vector<pjsip_transport*> to_destroy = _cleanupTransports(); + bool _destr = false; + for (auto t : to_destroy) { + if (t == tp) { + _destr = true; + break; + } + } + to_destroy_all.insert(to_destroy_all.end(), to_destroy.begin(), to_destroy.end()); + return _destr; + }; + destroyed = transportDestroyedCv_.wait_for(lock, std::chrono::milliseconds(50), check_destroyed); + if (!destroyed) + destroyed = check_destroyed(); + } + for (auto t : to_destroy_all) { + pj_lock_release(t->lock); + pjsip_transport_destroy(t); } + released_cb(destroyed); } void SipTransport::createSipTransport(SIPAccount &account) { - WARN("SipTransport::createSipTransport %s", account.getAccountID().c_str()); // Remove any existing transport from the account account.setTransport(); + cleanupTransports(); auto type = account.getTransportType(); auto family = pjsip_transport_type_get_af(type); auto interface = account.getLocalInterface(); - std::string key; + pjsip_transport* new_transport = nullptr; + { + std::lock_guard<std::mutex> lock(transportMapMutex_); + std::string key; #if HAVE_TLS - if (account.isTlsEnabled()) { - cleanupTransports(); - key = transportMapKey(interface, account.getTlsListenerPort(), type); - if (transportMap_.find(key) != transportMap_.end()) { - throw std::runtime_error("TLS transport already exists"); - } - createTlsTransport(account); - } else { + if (account.isTlsEnabled()) { + key = transportMapKey(interface, account.getTlsListenerPort(), type); + if (transportMap_.find(key) != transportMap_.end()) { + throw std::runtime_error("TLS transport already exists"); + } + createTlsTransport(account); + } else { #else - { + { #endif - auto port = account.getLocalPort(); - key = transportMapKey(interface, port, type); - // if this transport already exists, reuse it - auto iter = transportMap_.find(key); - if (iter != transportMap_.end()) { - auto status = pjsip_transport_add_ref(iter->second); - if (status == PJ_SUCCESS) - account.setTransport(iter->second); - } - if (!account.getTransport()) { - account.setTransport(createUdpTransport(interface, port, family)); + auto port = account.getLocalPort(); + key = transportMapKey(interface, port, type); + // if this transport already exists, reuse it + auto iter = transportMap_.find(key); + if (iter != transportMap_.end()) { + auto status = pjsip_transport_add_ref(iter->second); + if (status == PJ_SUCCESS) + account.setTransport(iter->second); + } + if (!account.getTransport()) { + account.setTransport(createUdpTransport(interface, port, family)); + } } - } - - auto new_transport = account.getTransport(); - if (new_transport) - transportMap_[key] = new_transport; + new_transport = account.getTransport(); + if (new_transport) + transportMap_[key] = new_transport; + } cleanupTransports(); if (!new_transport) { @@ -163,8 +193,6 @@ SipTransport::createSipTransport(SIPAccount &account) #endif throw std::runtime_error("Could not create new UDP transport"); } - - ERROR("Transport %s has count %d", account.transport_->info, pj_atomic_get(account.transport_->ref_cnt)); } pjsip_transport * @@ -201,7 +229,7 @@ SipTransport::createUdpTransport(const std::string &interface, pj_uint16_t port, DEBUG("Created UDP transport on %s : %s", interface.c_str(), ip_utils::addrToStr(listeningAddress, true, true).c_str()); // dump debug information to stdout - pjsip_tpmgr_dump_transports(pjsip_endpt_get_tpmgr(endpt_)); + //pjsip_tpmgr_dump_transports(pjsip_endpt_get_tpmgr(endpt_)); return transport; } @@ -283,11 +311,24 @@ SipTransport::createTlsTransport(SIPAccount &account) void SipTransport::cleanupTransports() { - ERROR("SipTransport::cleanupTransports"); + std::vector<pjsip_transport*> to_destroy; + { + std::lock_guard<std::mutex> lock(transportMapMutex_); + to_destroy = _cleanupTransports(); + } + for (auto t : to_destroy) { + pj_lock_release(t->lock); + pjsip_transport_destroy(t); + } +} + +std::vector<pjsip_transport*> +SipTransport::_cleanupTransports() +{ + std::vector<pjsip_transport*> to_destroy; for (auto it = transportMap_.cbegin(); it != transportMap_.cend();) { pjsip_transport* t = (*it).second; if (!t) { - ERROR("Null pointer found in transportMap_ for key %s", (*it).first.c_str()); transportMap_.erase(it++); continue; } @@ -297,15 +338,16 @@ SipTransport::cleanupTransports() DEBUG("Removing transport for %s", t->info ); bool is_shutdown = t->is_shutdown || t->is_destroying; transportMap_.erase(it++); - pj_lock_release(t->lock); if (!is_shutdown) - pjsip_transport_destroy(t); + to_destroy.push_back(t); + else + pj_lock_release(t->lock); } else { - DEBUG("Transport {%s} has refcount %d", t->info, ref_cnt); ++it; pj_lock_release(t->lock); } } + return to_destroy; } std::vector<pj_sockaddr> diff --git a/daemon/src/sip/siptransport.h b/daemon/src/sip/siptransport.h index 74cd7e8598778652a75a0292e3fddd652a972d7a..0d899ca6ad323958d036001ffa5f03dcf1ab60fb 100644 --- a/daemon/src/sip/siptransport.h +++ b/daemon/src/sip/siptransport.h @@ -46,19 +46,20 @@ #include <pjnath.h> #include <pjnath/stun_config.h> +#include <functional> +#include <mutex> +#include <condition_variable> #include <map> #include <string> #include <vector> #include <memory> -#include <functional> - class SIPAccount; class SipTransport { public: - SipTransport(pjsip_endpoint *endpt, pj_caching_pool& cp, pj_pool_t& pool, std::function<void(pjsip_transport*)> transportDestroyed = std::function<void(pjsip_transport*)>()); + SipTransport(pjsip_endpoint *endpt, pj_caching_pool& cp, pj_pool_t& pool); ~SipTransport(); /** @@ -101,7 +102,13 @@ class SipTransport { */ void cleanupTransports(); - void transportStateChanged(pjsip_transport* tp, pjsip_transport_state state); + /** + * Call released_cb(success) when transport tp is destroyed, making the + * socket available for a new similar transport. + * success is true if the transport is actually released. + * TODO: make this call non-blocking. + */ + void waitForReleased(pjsip_transport* tp, std::function<void(bool)> released_cb); private: NON_COPYABLE(SipTransport); @@ -137,15 +144,23 @@ class SipTransport { pjsip_transport *createUdpTransport(const std::string &interface, pj_uint16_t port, pj_uint16_t family = pj_AF_UNSPEC()); + /** + * Go through the transport list and remove unused ones. + * Returns a list of LOCKED transports that have to be processed and unlocked. + */ + std::vector<pjsip_transport*> _cleanupTransports(); + static void tp_state_callback(pjsip_transport *, pjsip_transport_state, const pjsip_transport_state_info *); + void transportStateChanged(pjsip_transport* tp, pjsip_transport_state state); + /** * UDP Transports are stored in this map in order to retreive them in case * several accounts would share the same port number. */ std::map<std::string, pjsip_transport*> transportMap_; - - std::function<void(pjsip_transport*)> transportDestroyedCb_; + std::mutex transportMapMutex_; + std::condition_variable transportDestroyedCv_; pj_caching_pool& cp_; pj_pool_t& pool_; diff --git a/daemon/src/sip/sipvoiplink.cpp b/daemon/src/sip/sipvoiplink.cpp index 8a10edeb852d1beb2321dd98eef8f3bb5d549708..dd69d36b9f8673842333eb9e477d978984dda9c3 100644 --- a/daemon/src/sip/sipvoiplink.cpp +++ b/daemon/src/sip/sipvoiplink.cpp @@ -550,14 +550,7 @@ SIPVoIPLink::SIPVoIPLink() : sipTransport(), sipAccountMap_(), TRY(pjsip_endpt_create(&cp_->factory, pj_gethostname()->ptr, &endpt_)); - sipTransport.reset(new SipTransport(endpt_, *cp_, *pool_, [&](pjsip_transport* transport) { - for (auto& a : sipAccountMap_ ) { - SIPAccount* account = static_cast<SIPAccount*>(a.second); - if ( account->getTransport() == transport ) { - account->setTransport(); - } - } - })); + sipTransport.reset(new SipTransport(endpt_, *cp_, *pool_)); if (ip_utils::getLocalAddr().addr.sa_family == pj_AF_UNSPEC()) throw VoipLinkException("UserAgent: Unable to determine network capabilities"); @@ -828,13 +821,15 @@ SIPVoIPLink::sendRegister(Account& a) sipTransport->cleanupTransports(); } -void SIPVoIPLink::sendUnregister(Account& a) +void SIPVoIPLink::sendUnregister(Account& a, std::function<void(bool)> released_cb) { SIPAccount& account = static_cast<SIPAccount&>(a); // This may occurs if account failed to register and is in state INVALID if (!account.isRegistered()) { account.setRegistrationState(RegistrationState::UNREGISTERED); + if (released_cb) + released_cb(true); return; } @@ -856,9 +851,14 @@ void SIPVoIPLink::sendUnregister(Account& a) } account.setRegister(false); - account.setTransport(); // remove the transport from the account + // remove the transport from the account + pjsip_transport* transport_ = account.getTransport(); + account.setTransport(); sipTransport->cleanupTransports(); + if (released_cb) { + sipTransport->waitForReleased(transport_, released_cb); + } } void SIPVoIPLink::registerKeepAliveTimer(pj_timer_entry &timer, pj_time_val &delay) @@ -2474,6 +2474,8 @@ void setCallMediaLocal(SIPCall* call, const pj_sockaddr& localIP) if (!account) return; + ERROR("setCallMediaLocal %s", ip_utils::addrToStr(localIP).c_str()); + // Reference: http://www.cs.columbia.edu/~hgs/rtp/faq.html#ports // We only want to set ports to new values if they haven't been set if (call->getLocalAudioPort() == 0) { diff --git a/daemon/src/sip/sipvoiplink.h b/daemon/src/sip/sipvoiplink.h index fc0858f5999fca9d84296783610669d54c512779..0b6a3f5c2f94be528e8fd1d70c4a0ac0a7daf75a 100644 --- a/daemon/src/sip/sipvoiplink.h +++ b/daemon/src/sip/sipvoiplink.h @@ -122,7 +122,7 @@ class SIPVoIPLink : public VoIPLink { * Build and send SIP unregistration request * @param destroy_transport If true, attempt to destroy the transport. */ - virtual void sendUnregister(Account& a); + virtual void sendUnregister(Account& a, std::function<void(bool)> cb = std::function<void(bool)>()); /** * Register a new keepalive registration timer to this endpoint diff --git a/daemon/src/voiplink.h b/daemon/src/voiplink.h index cf689c7e181246e79a1f6caf89b7fcf136421deb..752cfc8cdb0362c263ae4998fb86a82c46ed1a8d 100644 --- a/daemon/src/voiplink.h +++ b/daemon/src/voiplink.h @@ -35,6 +35,7 @@ #define __VOIP_LINK_H__ #include <stdexcept> +#include <functional> #include <string> #include <vector> @@ -73,7 +74,7 @@ class VoIPLink { * Build and send account unregistration request * Underlying ressources are released after unregistration */ - virtual void sendUnregister(Account& a) = 0; + virtual void sendUnregister(Account& a, std::function<void(bool)> cb = std::function<void(bool)>()) = 0; /** * Place a new call