ice: synchronize the upnp mappings with reflexive candidates
wait for the upnp to become active, meaning it is able to make port mappings, with a timeout UPNP_ACTIVE_TIMEOUT.
GitLab: #34
Change-Id: Ia4cd76e581c12f7c6b454f2557d4f20874371d53
diff --git a/include/connectionmanager.h b/include/connectionmanager.h
index 086981f..6cd3b56 100644
--- a/include/connectionmanager.h
+++ b/include/connectionmanager.h
@@ -315,12 +315,6 @@
std::shared_ptr<dhtnet::upnp::Controller> upnpCtrl;
std::shared_ptr<dht::log::Logger> logger;
- /**
- * returns whether or not UPnP is enabled and active
- * ie: if it is able to make port mappings
- */
- bool getUPnPActive() const;
-
/** Optional pseudo random generator to be used, allowing to control the seed. */
std::unique_ptr<std::mt19937_64> rng;
};
diff --git a/src/connectionmanager.cpp b/src/connectionmanager.cpp
index c0bde96..da5897e 100644
--- a/src/connectionmanager.cpp
+++ b/src/connectionmanager.cpp
@@ -363,19 +363,6 @@
std::map<DeviceId, std::shared_ptr<DeviceInfo>> infos_ {};
};
-
-/**
- * returns whether or not UPnP is enabled and active_
- * ie: if it is able to make port mappings
- */
-bool
-ConnectionManager::Config::getUPnPActive() const
-{
- if (upnpCtrl)
- return upnpCtrl->isReady();
- return false;
-}
-
class ConnectionManager::Impl : public std::enable_shared_from_this<ConnectionManager::Impl>
{
public:
@@ -569,12 +556,6 @@
std::function<void(const std::shared_ptr<dht::crypto::Certificate>&)>&& cb);
bool findCertificate(const dht::InfoHash& h, std::function<void(const std::shared_ptr<dht::crypto::Certificate>&)>&& cb);
- /**
- * returns whether or not UPnP is enabled and active
- * ie: if it is able to make port mappings
- */
- bool getUPnPActive() const;
-
std::shared_ptr<ConnectionManager::Config> config_;
std::unique_ptr<std::thread> ioContextRunner_;
@@ -1543,15 +1524,6 @@
return !treatedMessages_.add(id);
}
-/**
- * returns whether or not UPnP is enabled and active_
- * ie: if it is able to make port mappings
- */
-bool
-ConnectionManager::Impl::getUPnPActive() const
-{
- return config_->getUPnPActive();
-}
IpAddr
ConnectionManager::Impl::getPublishedIpAddress(uint16_t family) const
@@ -1644,7 +1616,7 @@
{
IceTransportOptions opts;
opts.factory = config_->factory;
- opts.upnpEnable = getUPnPActive();
+ opts.upnpEnable = config_->upnpEnabled;
opts.upnpContext = config_->upnpCtrl ? config_->upnpCtrl->upnpContext() : nullptr;
if (config_->stunEnabled)
diff --git a/src/ice_transport.cpp b/src/ice_transport.cpp
index 1edf00c..a860ec3 100644
--- a/src/ice_transport.cpp
+++ b/src/ice_transport.cpp
@@ -67,7 +67,7 @@
static constexpr int MAX_CANDIDATES {32};
static constexpr int MAX_DESTRUCTION_TIMEOUT {3000};
static constexpr int HANDLE_EVENT_DURATION {500};
-
+static constexpr std::chrono::seconds PORT_MAPPING_TIMEOUT {4};
//==============================================================================
using namespace upnp;
@@ -210,10 +210,12 @@
};
std::shared_ptr<upnp::Controller> upnp_ {};
- std::mutex upnpMutex_ {};
std::map<Mapping::key_t, Mapping> upnpMappings_;
std::mutex upnpMappingsMutex_ {};
+ std::mutex upnpMutex_ {};
+ std::condition_variable upnpCv_;
+
bool onlyIPv4Private_ {true};
// IO/Timer events are handled by following thread
@@ -922,44 +924,74 @@
void
IceTransport::Impl::requestUpnpMappings()
{
- // Must be called once !
-
- std::lock_guard lock(upnpMutex_);
-
if (not upnp_)
return;
-
auto transport = isTcpEnabled() ? PJ_CAND_TCP_PASSIVE : PJ_CAND_UDP;
auto portType = transport == PJ_CAND_UDP ? PortType::UDP : PortType::TCP;
+ // Use a different map instead of upnpMappings_ to store pointers to the mappings
+ auto upnpMappings = std::make_shared<std::map<Mapping::key_t, Mapping::sharedPtr_t>>();
+ auto isFailed = std::make_shared<bool>(false);
+
+ std::unique_lock lock(upnpMutex_);
+
// Request upnp mapping for each component.
for (unsigned id = 1; id <= compCount_; id++) {
// Set port number to 0 to get any available port.
Mapping requestedMap(portType);
- // Request the mapping
- Mapping::sharedPtr_t mapPtr = upnp_->reserveMapping(requestedMap);
+ requestedMap.setNotifyCallback([upnpMappings, isFailed, this](Mapping::sharedPtr_t mapPtr) {
+ // Ignore intermidiate states : PENDING, IN_PROGRESS
+ // only OPEN and FAILED are considered
- // To use a mapping, it must be valid, open and has valid host address.
- if (mapPtr and mapPtr->getMapKey() and (mapPtr->getState() == MappingState::OPEN)
- and mapPtr->hasValidHostAddress()) {
- std::lock_guard lock(upnpMappingsMutex_);
- auto ret = upnpMappings_.emplace(mapPtr->getMapKey(), *mapPtr);
- if (ret.second) {
+ // if the mapping is open check the validity
+ if ((mapPtr->getState() == MappingState::OPEN)) {
+ if (mapPtr->getMapKey() and mapPtr->hasValidHostAddress()){
+ std::lock_guard lockMapping(upnpMappingsMutex_);
+ upnpMappings->emplace(mapPtr->getMapKey(), mapPtr);
+ } else {
+ *isFailed = true;
+ }
+ } else if (mapPtr->getState() == MappingState::FAILED) {
+ *isFailed = true;
if (logger_)
- logger_->debug("[ice:{}] UPNP mapping {:s} successfully allocated",
- fmt::ptr(this),
- mapPtr->toString(true));
- } else {
- if (logger_)
- logger_->warn("[ice:{}] UPNP mapping {:s} already in the list!",
- fmt::ptr(this),
- mapPtr->toString());
+ logger_->error("[ice:{}] UPNP mapping failed: {:s}",
+ fmt::ptr(this),
+ mapPtr->toString(true));
}
- } else {
- if (logger_)
- logger_->warn("[ice:{}] UPNP mapping request failed!", fmt::ptr(this));
- upnp_->releaseMapping(requestedMap);
+ upnpCv_.notify_all();
+ });
+ // Request the mapping
+ upnp_->reserveMapping(requestedMap);
+ }
+ upnpCv_.wait_for(lock, PORT_MAPPING_TIMEOUT, [&] {
+ return upnpMappings->size() == compCount_ or *isFailed;
+ });
+
+ std::lock_guard lockMapping(upnpMappingsMutex_);
+
+ // remove the notify callback
+ for (auto& map : *upnpMappings) {
+ map.second->setNotifyCallback(nullptr);
+ }
+ // Check the number of mappings
+ if (upnpMappings->size() != compCount_) {
+ if (logger_)
+ logger_->error("[ice:{}] UPNP mapping failed: expected {:d} mappings, got {:d}",
+ fmt::ptr(this),
+ compCount_,
+ upnpMappings->size());
+ // release all mappings
+ for (auto& map : *upnpMappings) {
+ upnp_->releaseMapping(*map.second);
+ }
+ } else {
+ for (auto& map : *upnpMappings) {
+ upnpMappings_.emplace(map.first, *map.second);
+ if(logger_)
+ logger_->debug("[ice:{}] UPNP mapping {:s} successfully allocated\n",
+ fmt::ptr(this),
+ map.second->toString(true));
}
}
}