diff --git a/include/upnp/upnp_context.h b/include/upnp/upnp_context.h
index fca38a530b7df0fc1795aade85db5cb6469186be..d16105372438f9873846dcbdb830b154548f7f98 100644
--- a/include/upnp/upnp_context.h
+++ b/include/upnp/upnp_context.h
@@ -75,6 +75,7 @@ public:
virtual void onMappingRequestFailed(const Mapping& map) = 0;
virtual void onMappingRenewed(const std::shared_ptr<IGD>& igd, const Mapping& map) = 0;
virtual void onMappingRemoved(const std::shared_ptr<IGD>& igd, const Mapping& map) = 0;
+ virtual void onIgdDiscoveryStarted() = 0;
};
class UPnPContext : public UpnpMappingObserver
@@ -133,6 +134,11 @@ public:
});
}
+ // Set the timeout for the IGD discovery process.
+ // If the timeout expires and no valid IGD has been discovered,
+ // then the state of all pending mappings is set to FAILED.
+ void setIgdDiscoveryTimeout(std::chrono::milliseconds timeout);
+
private:
// Initialization
void init();
@@ -242,6 +248,9 @@ private:
// Callback used to report remove request status.
void onMappingRemoved(const std::shared_ptr<IGD>& igd, const Mapping& map) override;
+ // Callback used to report the start of the discovery process: search for IGDs.
+ void onIgdDiscoveryStarted() override;
+
private:
UPnPContext(const UPnPContext&) = delete;
UPnPContext(UPnPContext&&) = delete;
@@ -304,6 +313,16 @@ private:
// Shutdown synchronization
bool shutdownComplete_ {false};
bool shutdownTimedOut_ {false};
+
+ // IGD Discovery synchronization. This boolean indicates if the IGD discovery is in progress.
+ bool igdDiscoveryInProgress_ {true};
+ std::mutex igdDiscoveryMutex_;
+ std::chrono::milliseconds igdDiscoveryTimeout_ {std::chrono::milliseconds(500)};
+
+ // End of the discovery process.
+ void _endIgdDiscovery();
+
+ asio::steady_timer igdDiscoveryTimer_;
};
} // namespace upnp
diff --git a/src/upnp/protocol/natpmp/nat_pmp.cpp b/src/upnp/protocol/natpmp/nat_pmp.cpp
index 3c17e09e07a22f0c45cd8b536a88c13a9fa92de0..9005ee4a4e8801ff0335040ff0c19537b2f33355 100644
--- a/src/upnp/protocol/natpmp/nat_pmp.cpp
+++ b/src/upnp/protocol/natpmp/nat_pmp.cpp
@@ -189,6 +189,7 @@ void
NatPmp::searchForIgd()
{
if (not initialized_) {
+ observer_->onIgdDiscoveryStarted();
initNatPmp();
}
@@ -196,7 +197,6 @@ NatPmp::searchForIgd()
if (not initialized_) {
if (igdSearchCounter_++ < MAX_RESTART_SEARCH_RETRIES) {
if (logger_) logger_->debug("NAT-PMP: Start search for IGDs. Attempt {}", igdSearchCounter_);
-
// Cancel the current timer (if any) and re-schedule.
searchForIgdTimer_.expires_after(NATPMP_SEARCH_RETRY_UNIT * igdSearchCounter_);
searchForIgdTimer_.async_wait([w=weak()](const asio::error_code& ec) {
diff --git a/src/upnp/protocol/pupnp/pupnp.cpp b/src/upnp/protocol/pupnp/pupnp.cpp
index c5a54e6990fd8b3cf0c31a7da3f28d169bf8ff98..afb110a8778731d9bea7803cb76ed16d485143e1 100644
--- a/src/upnp/protocol/pupnp/pupnp.cpp
+++ b/src/upnp/protocol/pupnp/pupnp.cpp
@@ -240,6 +240,35 @@ PUPnP::terminate()
if (logger_) logger_->debug("PUPnP: Instance {} terminated", fmt::ptr(this));
}
+void
+PUPnP::searchForDeviceAsync(const std::string& deviceType)
+{
+ // Despite its name and the claim in the libupnp documentation that it "returns immediately",
+ // the UpnpSearchAsync function isn't really async. This is because it tries to send multiple
+ // copies of each search message and waits for a certain amount of time after sending each
+ // copy. The number of copies is given by the NUM_SSDP_COPY macro, whose default value is 2,
+ // and the waiting time is determined by the SSDP_PAUSE macro, whose default value is 100 (ms).
+ // If both IPv4 and IPv6 are enabled, then UpnpSearchAsync sends 3 distinct messages (2 for IPv6
+ // and 1 for IPv4), resulting in a total of 3 * 2 * 100 = 600 ms spent waiting by default.
+ // This is why we put the call to UpnpSearchAsync on its own thread.
+ dht::ThreadPool::io().run([w = weak_from_this(), deviceType] {
+ auto sthis = std::static_pointer_cast<PUPnP>(w.lock());
+ if (!sthis)
+ return;
+
+ auto err = UpnpSearchAsync(sthis->ctrlptHandle_,
+ SEARCH_TIMEOUT,
+ deviceType.c_str(),
+ sthis.get());
+ if (err != UPNP_E_SUCCESS) {
+ if (sthis->logger_)
+ sthis->logger_->warn("PUPnP: Send search for {} failed. Error {:d}: {}",
+ deviceType,
+ err,
+ UpnpGetErrorMessage(err));
+ }
+ });
+}
void
PUPnP::searchForDevices()
{
@@ -247,34 +276,10 @@ PUPnP::searchForDevices()
// Send out search for multiple types of devices, as some routers may possibly
// only reply to one.
-
- auto err = UpnpSearchAsync(ctrlptHandle_, SEARCH_TIMEOUT, UPNP_ROOT_DEVICE, this);
- if (err != UPNP_E_SUCCESS) {
- if (logger_) logger_->warn("PUPnP: Send search for UPNP_ROOT_DEVICE failed. Error {:d}: {}",
- err,
- UpnpGetErrorMessage(err));
- }
-
- err = UpnpSearchAsync(ctrlptHandle_, SEARCH_TIMEOUT, UPNP_IGD_DEVICE, this);
- if (err != UPNP_E_SUCCESS) {
- if (logger_) logger_->warn("PUPnP: Send search for UPNP_IGD_DEVICE failed. Error {:d}: {}",
- err,
- UpnpGetErrorMessage(err));
- }
-
- err = UpnpSearchAsync(ctrlptHandle_, SEARCH_TIMEOUT, UPNP_WANIP_SERVICE, this);
- if (err != UPNP_E_SUCCESS) {
- if (logger_) logger_->warn("PUPnP: Send search for UPNP_WANIP_SERVICE failed. Error {:d}: {}",
- err,
- UpnpGetErrorMessage(err));
- }
-
- err = UpnpSearchAsync(ctrlptHandle_, SEARCH_TIMEOUT, UPNP_WANPPP_SERVICE, this);
- if (err != UPNP_E_SUCCESS) {
- if (logger_) logger_->warn("PUPnP: Send search for UPNP_WANPPP_SERVICE failed. Error {:d}: {}",
- err,
- UpnpGetErrorMessage(err));
- }
+ searchForDeviceAsync(UPNP_ROOT_DEVICE);
+ searchForDeviceAsync(UPNP_IGD_DEVICE);
+ searchForDeviceAsync(UPNP_WANIP_SERVICE);
+ searchForDeviceAsync(UPNP_WANPPP_SERVICE);
}
void
@@ -340,6 +345,7 @@ PUPnP::searchForIgd()
if (clientRegistered_) {
assert(initialized_);
searchForDevices();
+ observer_->onIgdDiscoveryStarted();
} else {
if (logger_) logger_->warn("PUPnP: PUPNP not fully setup. Skipping the IGD search");
}
diff --git a/src/upnp/protocol/pupnp/pupnp.h b/src/upnp/protocol/pupnp/pupnp.h
index 32fe19a1ed45c9898f8d8c3d38c3164fd4585e04..ae02d45ac4e07c1ac360c8233ea3ac8e996b17ae 100644
--- a/src/upnp/protocol/pupnp/pupnp.h
+++ b/src/upnp/protocol/pupnp/pupnp.h
@@ -134,6 +134,9 @@ private:
// Start search for UPNP devices
void searchForDevices();
+ // Start search for UPNP device in a different thread
+ void searchForDeviceAsync(const std::string& deviceType);
+
// Return true if it has at least one valid IGD.
bool hasValidIgd() const;
diff --git a/src/upnp/upnp_context.cpp b/src/upnp/upnp_context.cpp
index e832c65e2ffe7bf3930f016d0407373fe922cd9c..47ff9d9c3962dc26101df014b143336c26d5a08a 100644
--- a/src/upnp/upnp_context.cpp
+++ b/src/upnp/upnp_context.cpp
@@ -51,6 +51,8 @@ UPnPContext::UPnPContext(const std::shared_ptr<asio::io_context>& ioContext, con
, renewalSchedulingTimer_(*ctx)
, syncTimer_(*ctx)
, connectivityChangedTimer_(*ctx)
+ , igdDiscoveryTimer_(*ctx)
+
{
if (logger_) logger_->debug("Creating UPnPContext instance [{}]", fmt::ptr(this));
@@ -1095,6 +1097,51 @@ UPnPContext::onMappingRemoved(const std::shared_ptr<IGD>& igd, const Mapping& ma
map->getNotifyCallback()(map);
}
+void
+UPnPContext::onIgdDiscoveryStarted()
+{
+ std::lock_guard lock(igdDiscoveryMutex_);
+ igdDiscoveryInProgress_ = true;
+ if (logger_) logger_->debug("IGD Discovery started");
+ igdDiscoveryTimer_.expires_after(igdDiscoveryTimeout_);
+ igdDiscoveryTimer_.async_wait([this] (const asio::error_code& ec) {
+ if (ec != asio::error::operation_aborted && igdDiscoveryInProgress_) {
+ _endIgdDiscovery();
+ }
+ });
+}
+
+void
+UPnPContext::_endIgdDiscovery()
+{
+ std::lock_guard lockDiscovery_(igdDiscoveryMutex_);
+ igdDiscoveryInProgress_ = false;
+ if (logger_) logger_->debug("IGD Discovery ended");
+ if (isReady()) {
+ return;
+ }
+ // if there is no valid IGD, the pending mapping requests will be changed to failed
+ std::lock_guard lockMappings_(mappingMutex_);
+ PortType types[2] {PortType::TCP, PortType::UDP};
+ for (auto& type : types) {
+ const auto& mappingList = getMappingList(type);
+ for (auto const& [_, map] : mappingList) {
+ updateMappingState(map, MappingState::FAILED);
+ // Do not unregister the mapping, it's up to the controller to decide. It will be unregistered when the controller releases it.
+ // unregisterMapping(map) here will cause a deadlock because of the lock on mappingMutex_.
+ if (logger_) logger_->warn("Request for mapping {} failed, no IGD available",
+ map->toString());
+ }
+ }
+}
+
+void
+UPnPContext::setIgdDiscoveryTimeout(std::chrono::milliseconds timeout)
+{
+ std::lock_guard lock(igdDiscoveryMutex_);
+ igdDiscoveryTimeout_ = timeout;
+}
+
Mapping::sharedPtr_t
UPnPContext::registerMapping(Mapping& map)
{
@@ -1123,14 +1170,23 @@ UPnPContext::registerMapping(Mapping& map)
assert(mapPtr);
}
- // No available IGD. The pending mapping requests will be processed
- // when an IGD becomes available
if (not isReady()) {
- if (logger_) logger_->warn("No IGD available. Mapping will be requested when an IGD becomes available");
+ // There is no valid IGD available
+ std::lock_guard lock(igdDiscoveryMutex_);
+ // IGD discovery is in progress, the mapping request will be made once an IGD becomes available
+ if (igdDiscoveryInProgress_) {
+ if (logger_) logger_->debug("Request for mapping {} will be requested when an IGD becomes available",
+ map.toString());
+ } else {
+ // it's not in the IGD discovery phase, the mapping request will fail
+ if (logger_) logger_->warn("Request for mapping {} failed, no IGD available",
+ map.toString());
+ updateMappingState(mapPtr, MappingState::FAILED);
+ }
} else {
+ // There is a valid IGD available, request the mapping.
requestMapping(mapPtr);
}
-
return mapPtr;
}