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

Author: AmnaSnene

include/connectionmanager.h

@@ -315,12 +315,6 @@ struct ConnectionManager::Config
     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;
 };

src/connectionmanager.cpp

@@ -363,19 +363,6 @@ class DeviceInfoSet {
     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 @@ class ConnectionManager::Impl : public std::enable_shared_from_this<ConnectionMa
                          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 @@ ConnectionManager::Impl::isMessageTreated(dht::Value::Id id)
     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 @@ ConnectionManager::Impl::getIceOptions() const noexcept
 {
     IceTransportOptions opts;
     opts.factory = config_->factory;
-    opts.upnpEnable = getUPnPActive();
+    opts.upnpEnable = config_->upnpEnabled;
     opts.upnpContext = config_->upnpCtrl ? config_->upnpCtrl->upnpContext() : nullptr;
 
     if (config_->stunEnabled)

src/ice_transport.cpp

@@ -67,7 +67,7 @@ static constexpr uint16_t IPV4_HEADER_SIZE = 20; ///< Size in bytes of IPV4 pack
 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 @@ class IceTransport::Impl
     };
 
     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 @@ IceTransport::Impl::addStunConfig(int af)
 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);
-
-        // 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) {
+        requestedMap.setNotifyCallback([upnpMappings, isFailed, this](Mapping::sharedPtr_t mapPtr) {
+            // Ignore intermidiate states : PENDING, IN_PROGRESS
+            // only OPEN and FAILED are considered
+
+            // 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));
         }
     }
 }