upnp: change UPnPContext::hasValidIGD() to be optionally blocking

* Add timeout parameter to hasValidIGD to let caller decide
  if he want to block or not and how long.
* Change callers to support this timout.
* replace extra search mutex/cv by exiting validIGD mutex and add
a validIGD cv.

Refs #67319

Change-Id: If743aa6df9aa21c0a542d489615c9b032e2b5fa7

daemon/src/account.cpp

@@ -502,11 +502,11 @@ Account::getUPnPIpAddress() const
  * ie: if it is able to make port mappings
  */
 bool
-Account::getUPnPActive() const
+Account::getUPnPActive(std::chrono::seconds timeout) const
 {
     std::lock_guard<std::mutex> lk(upnp_mtx);
     if (upnpEnabled_)
-        return upnp_->hasValidIGD();
+        return upnp_->hasValidIGD(timeout);
     return false;
 }
 

daemon/src/account.h

@@ -53,6 +53,7 @@
 #include <stdexcept>
 #include <atomic>
 #include <mutex>
+#include <chrono>
 
 namespace ring { namespace upnp {
 class Controller;
@@ -255,7 +256,7 @@ class Account : public Serializable, public std::enable_shared_from_this<Account
          * returns whether or not UPnP is enabled and active
          * ie: if it is able to make port mappings
          */
-        bool getUPnPActive() const;
+        bool getUPnPActive(std::chrono::seconds timeout = {}) const;
 
         /**
          * Get the UPnP IP (external router) address.

daemon/src/upnp/upnp_context.cpp

@@ -40,6 +40,7 @@
 #include <memory>
 #include <condition_variable>
 #include <random>
+#include <chrono>
 #include <cstdlib> // for std::free
 
 #if HAVE_LIBUPNP
@@ -178,39 +179,33 @@ UPnPContext::~UPnPContext()
 void
 UPnPContext::searchForIGD()
 {
-    if (clientRegistered_) {
-        std::lock_guard<std::mutex> lock(igdSearchMutex_);
-        /* send out search for both types, as some routers may possibly only reply to one */
-        UpnpSearchAsync(ctrlptHandle_, SEARCH_TIMEOUT_SEC, UPNP_ROOT_DEVICE, nullptr);
-        UpnpSearchAsync(ctrlptHandle_, SEARCH_TIMEOUT_SEC, UPNP_IGD_DEVICE, nullptr);
-        UpnpSearchAsync(ctrlptHandle_, SEARCH_TIMEOUT_SEC, UPNP_WANIP_SERVICE, nullptr);
-        UpnpSearchAsync(ctrlptHandle_, SEARCH_TIMEOUT_SEC, UPNP_WANPPP_SERVICE, nullptr);
-
-        pendingIGDSearchRequests_ = 4;
-    } else
-        RING_WARN("UPnP: Control Point not registered.");
+    if (not clientRegistered_) {
+        RING_WARN("UPnP: Control Point not registered");
+        return;
+    }
+
+    /* send out search for both types, as some routers may possibly only reply to one */
+    UpnpSearchAsync(ctrlptHandle_, SEARCH_TIMEOUT, UPNP_ROOT_DEVICE, nullptr);
+    UpnpSearchAsync(ctrlptHandle_, SEARCH_TIMEOUT, UPNP_IGD_DEVICE, nullptr);
+    UpnpSearchAsync(ctrlptHandle_, SEARCH_TIMEOUT, UPNP_WANIP_SERVICE, nullptr);
+    UpnpSearchAsync(ctrlptHandle_, SEARCH_TIMEOUT, UPNP_WANPPP_SERVICE, nullptr);
 }
 
-/**
- * returns 'true' if there is at least one valid (connected) IGD
- * note: this function will until an IGD has been found or SEARCH_TIMEOUT_SEC
- *       have expired; the timeout starts when the context is created as we start
- *       searchign for IGDs immediately
- */
 bool
-UPnPContext::hasValidIGD()
+UPnPContext::hasValidIGD(std::chrono::seconds timeout)
 {
     if (not clientRegistered_) {
-        RING_WARN("UPnP: Control Point not registered.");
+        RING_WARN("UPnP: Control Point not registered");
         return false;
     }
 
-    {
-        std::unique_lock<std::mutex> lock(igdSearchMutex_);
-        igdSearchCondition_.wait(lock, [this] { return pendingIGDSearchRequests_ <= 0;});
+    std::unique_lock<std::mutex> lock(validIGDMutex_);
+    if (!validIGDCondVar_.wait_for(lock, timeout,
+                                   [this]{return not validIGDs_.empty();})) {
+        RING_WARN("UPnP: Valid IGD search timeout");
+        return false;
     }
 
-    std::lock_guard<std::mutex> lock(validIGDMutex_);
     return not validIGDs_.empty();
 }
 
@@ -256,7 +251,7 @@ UPnPContext::addMapping(IGD* igd,
     auto iter = globalMappings->find(port_external);
     if (iter != globalMappings->end()) {
         /* mapping exists with same external port */
-        GlobalMapping *mapping_ptr = &iter->second;
+        GlobalMapping* mapping_ptr = &iter->second;
         if (*mapping_ptr == mapping) {
             /* the same mapping, so nothing needs to be done */
             *upnp_error = UPNP_E_SUCCESS;
@@ -337,17 +332,12 @@ UPnPContext::addAnyMapping(uint16_t port_desired,
                            bool use_same_port,
                            bool unique)
 {
-    if (not hasValidIGD()) {
-        RING_WARN("UPnP: no valid IGD availabe");
-        return {};
-    }
-
     /* get a lock on the igd list because we don't want the igd to be modified
      * or removed from the list while using it */
     std::lock_guard<std::mutex> lock(validIGDMutex_);
     IGD* igd = chooseIGD_unlocked();
     if (not igd) {
-        RING_WARN("UPnP: no valid IGD availabe");
+        RING_WARN("UPnP: no valid IGD available");
         return {};
     }
 
@@ -402,18 +392,13 @@ UPnPContext::addAnyMapping(uint16_t port_desired,
 void
 UPnPContext::removeMapping(const Mapping& mapping)
 {
-    if (not hasValidIGD()) {
-        RING_WARN("UPnP: no valid IGD availabe");
-        return;
-    }
-
     /* get a lock on the igd list because we don't want the igd to be modified
      * or removed from the list while using it */
     std::lock_guard<std::mutex> lock(validIGDMutex_);
     IGD* igd = chooseIGD_unlocked();
     if (not igd) {
-        RING_WARN("UPnP: no valid IGD availabe");
-        return;;
+        RING_WARN("UPnP: no valid IGD available");
+        return;
     }
 
     /* first make sure the mapping exists in the global list of the igd */
@@ -451,17 +436,12 @@ UPnPContext::removeMapping(const Mapping& mapping)
 IpAddr
 UPnPContext::getExternalIP()
 {
-    if (not hasValidIGD()) {
-        RING_WARN("UPnP: no valid IGD availabe");
-        return {};
-    }
-
     /* get a lock on the igd list because we don't want the igd to be modified
      * or removed from the list while using it */
     std::lock_guard<std::mutex> lock(validIGDMutex_);
     IGD* igd = chooseIGD_unlocked();
     if (not igd) {
-        RING_WARN("UPnP: no valid IGD availabe");
+        RING_WARN("UPnP: no valid IGD available");
         return {};
     }
 
@@ -637,15 +617,6 @@ UPnPContext::parseIGD(IXML_Document* doc, const Upnp_Discovery* d_event)
             removeMappingsByLocalIPAndDescription(new_igd.get(), Mapping::UPNP_DEFAULT_MAPPING_DESCRIPTION);
             validIGDs_.emplace(UDN, std::move(new_igd));
         }
-
-        /* check if we have pending search requests and notify anyone waiting */
-        {
-            std::lock_guard<std::mutex> lock(igdSearchMutex_);
-            if (pendingIGDSearchRequests_ > 0) {
-                pendingIGDSearchRequests_ = 0;
-            }
-        }
-        igdSearchCondition_.notify_all();
     }
 }
 
@@ -692,7 +663,6 @@ get_first_element_item(IXML_Element* element, const char* item)
     return ret;
 }
 
-
 int
 cp_callback(Upnp_EventType event_type, void* event, void* /*user_data*/)
 {
@@ -801,16 +771,8 @@ cp_callback(Upnp_EventType event_type, void* event, void* /*user_data*/)
         break;
 
     case UPNP_DISCOVERY_SEARCH_TIMEOUT:
-    {
         RING_DBG("UPnP: Control Point search timeout");
-        {
-            std::lock_guard<std::mutex> lock(upnpContext->igdSearchMutex_);
-            if (upnpContext->pendingIGDSearchRequests_ > 0)
-                --(upnpContext->pendingIGDSearchRequests_);
-        }
-        upnpContext->igdSearchCondition_.notify_all();
-    }
-    break;
+        break;
 
     case UPNP_CONTROL_ACTION_COMPLETE:
     {

daemon/src/upnp/upnp_context.h

@@ -40,6 +40,7 @@
 #include <mutex>
 #include <memory>
 #include <condition_variable>
+#include <chrono>
 
 #if HAVE_LIBUPNP
 #include <upnp/upnp.h>
@@ -58,22 +59,19 @@ namespace ring { namespace upnp {
 class UPnPContext {
 
 public:
+    constexpr static unsigned SEARCH_TIMEOUT {30};
 
-#if HAVE_LIBUPNP
-
-    /* search timeout in seconds */
-    constexpr static int SEARCH_TIMEOUT_SEC = 30;
 
+#if HAVE_LIBUPNP
     UPnPContext();
     ~UPnPContext();
 
     /**
-     * returns 'true' if there is at least one valid (connected) IGD
-     * note: this function will wait until an IGD has been found or SEARCH_TIMEOUT_SEC
-     *       have expired; the timeout starts when the context is created as we start
-     *       searching for IGDs immediately
+     * Returns 'true' if there is at least one valid (connected) IGD.
+     * @param timeout Time to wait until a valid IGD is found.
+     * If timeout is not given or 0, the function pool (non-blocking).
      */
-    bool hasValidIGD();
+    bool hasValidIGD(std::chrono::seconds timeout = {});
 
     /**
      * tries to add mapping from and to the port_desired
@@ -86,8 +84,6 @@ public:
      * that the external and internal ports are the same
      *
      * returns a valid mapping on success and an invalid mapping on failure
-     *
-     * note: this function will call hasValidIGD and wait upto SEARCH_TIMEOUT_SEC
      */
     Mapping addAnyMapping(uint16_t port_desired,
                           uint16_t port_local,
@@ -97,11 +93,12 @@ public:
 
     /**
      * tries to remove the given mapping
-     *
-     * note: this function will call hasValidIGD and wait upto SEARCH_TIMEOUT_SEC
      */
     void removeMapping(const Mapping& mapping);
 
+    /**
+     * tries to get the external ip of the router
+     */
     IpAddr getExternalIP();
 
 #else
@@ -162,7 +159,8 @@ private:
      * the mutex is used to access these lists and IGDs in a thread-safe manner
      */
     std::map<std::string, std::unique_ptr<IGD>> validIGDs_;
-    std::mutex validIGDMutex_;
+    mutable std::mutex validIGDMutex_;
+    std::condition_variable validIGDCondVar_;
 
     /**
      * chooses the IGD to use (currently selects the first one in the map)
@@ -173,11 +171,6 @@ private:
     /* sends out async search for IGD */
     void searchForIGD();
 
-    /* vars to sync search timeout */
-    unsigned pendingIGDSearchRequests_ {0};
-    std::mutex igdSearchMutex_;
-    std::condition_variable igdSearchCondition_;
-
     /**
      * callback function for the UPnP client (control point)
      * all UPnP events received by the client are processed here
@@ -233,4 +226,4 @@ std::shared_ptr<UPnPContext> getUPnPContext();
 
 }} // namespace ring::upnp
 
-#endif /* UPNP_CONTEXT_H_ */
\ No newline at end of file
+#endif /* UPNP_CONTEXT_H_ */

daemon/src/upnp/upnp_control.cpp

@@ -56,29 +56,15 @@ Controller::~Controller()
     removeMappings();
 }
 
-/**
- * Return whether or not this controller has a valid IGD,
- * if 'flase' then all requests will fail
- */
 bool
-Controller::hasValidIGD()
+Controller::hasValidIGD(std::chrono::seconds timeout)
 {
 #if HAVE_LIBUPNP
-    return upnpContext_->hasValidIGD();
+    return upnpContext_->hasValidIGD(timeout);
 #endif
     return false;
 }
 
-/**
- * tries to add mapping from and to the port_desired
- * if unique == true, makes sure the client is not using this port already
- * if the mapping fails, tries other available ports until success
- *
- * tries to use a random port between 1024 < > 65535 if desired port fails
- *
- * maps port_desired to port_local; if use_same_port == true, makes sure that
- * that the extranl and internal ports are the same
- */
 bool
 Controller::addAnyMapping(uint16_t port_desired,
                           uint16_t port_local,
@@ -104,9 +90,6 @@ Controller::addAnyMapping(uint16_t port_desired,
     return false;
 }
 
-/**
- * addAnyMapping with the local port being the same as the external port
- */
 bool
 Controller::addAnyMapping(uint16_t port_desired,
                           PortType type,
@@ -116,11 +99,6 @@ Controller::addAnyMapping(uint16_t port_desired,
     addAnyMapping(port_desired, port_desired, type, true, unique, port_used);
 }
 
-/**
- * removes mappings added by this instance of the specified port type
- * if an mapping has more than one user in the global list, it is not deleted
- * from the router, but the number of users is decremented
- */
 void
 Controller::removeMappings(PortType type) {
 #if HAVE_LIBUPNP
@@ -132,10 +110,6 @@ Controller::removeMappings(PortType type) {
     }
 #endif
 }
-
-/**
- * removes all mappings added by this instance
- */
 void
 Controller::removeMappings()
 {
@@ -145,9 +119,6 @@ Controller::removeMappings()
 #endif
 }
 
-/**
- * tries to get the external ip of the router
- */
 IpAddr
 Controller::getExternalIP()
 {

daemon/src/upnp/upnp_control.h

@@ -36,6 +36,7 @@
 #endif
 
 #include <memory>
+#include <chrono>
 
 #include "noncopyable.h"
 #include "upnp_igd.h"
@@ -58,16 +59,11 @@ public:
     ~Controller();
 
     /**
-     * Return whether or not this controller has a valid IGD,
-     * if 'flase' then all requests will fail
+     * Return whether or not this controller has a valid IGD.
+     * @param timeout Time to wait until a valid IGD is found.
+     * If timeout is not given or 0, the function pool (non-blocking).
      */
-    bool hasValidIGD();
-
-    /**
-     * like hasValidIGD, but calls the given callback when the IGD is found
-     * or when the search times out without finding one
-     */
-    // void waitForValildIGD(IGDFoundCallback cb);
+    bool hasValidIGD(std::chrono::seconds timeout = {});
 
     /**
      * tries to add mapping from and to the port_desired