From e9a6da651142ce1ced69d13dca4272351013b658 Mon Sep 17 00:00:00 2001
From: Mohamed Chibani <mohamed.chibani@savoirfairelinux.com>
Date: Fri, 29 Jan 2021 17:33:51 -0500
Subject: [PATCH] upnp: unconditionally remove failed mappings
In the previous implementation, a failed mapping was not removed if it's
beeing used. The responsability was left to the owner to release it when
done with it. This requires that all the owners implement the mapping
callback and properly release the mapping. This might be somehow cumbersome
and prune to error.
Now, a failed mapping is automatically removed, and no further action is
required from the owner. The mapping's owner will still receive a notification
callback (if registered) when the state of the mapping changes to failed (or
to any other state).
Change-Id: I014ba3906b836efce3173fb22fbe298a4023db7e
Gitlab: #335
---
src/ice_transport.cpp | 1 +
src/upnp/upnp_context.cpp | 36 +++++++++++++++++-------------------
2 files changed, 18 insertions(+), 19 deletions(-)
diff --git a/src/ice_transport.cpp b/src/ice_transport.cpp
index 093e9a431c..914b756806 100644
--- a/src/ice_transport.cpp
+++ b/src/ice_transport.cpp
@@ -800,6 +800,7 @@ IceTransport::Impl::requestUpnpMappings()
}
} else {
JAMI_ERR("[ice:%p]: UPNP mapping request failed!", this);
+ upnp_->releaseMapping(requestedMap);
}
}
}
diff --git a/src/upnp/upnp_context.cpp b/src/upnp/upnp_context.cpp
index 70024ddae6..7c88161624 100644
--- a/src/upnp/upnp_context.cpp
+++ b/src/upnp/upnp_context.cpp
@@ -115,9 +115,7 @@ UPnPContext::StopUpnp()
for (auto const& map : toRemoveList) {
map->cancelTimeoutTimer();
updateMappingState(map, MappingState::FAILED);
- if (map->isAvailable()) {
- unregisterMapping(map);
- }
+ unregisterMapping(map);
}
for (auto const& [_, protocol] : protocolList_) {
@@ -529,6 +527,18 @@ UPnPContext::updateMappingList(bool async)
status.inProgressCount_,
status.failedCount_);
+ if (status.failedCount_ > 0) {
+ std::lock_guard<std::mutex> lock(mappingMutex_);
+ auto const& mappingList = getMappingList(type);
+ for (auto const& [_, map] : mappingList) {
+ if (map->getState() == MappingState::FAILED) {
+ JAMI_DBG("Mapping status [%s] - Available [%s]",
+ map->toString(true).c_str(),
+ map->isAvailable() ? "YES" : "NO");
+ }
+ }
+ }
+
int toRequestCount = (int) minOpenPortLimit_[idx]
- (int) (status.readyCount_ + status.inProgressCount_
+ status.pendingCount_);
@@ -626,10 +636,7 @@ UPnPContext::pruneUnMatchedMappings(const std::shared_ptr<IGD>& igd,
for (auto const& map : toRemoveList) {
updateMappingState(map, MappingState::FAILED);
- // Remove if not used.
- if (map->isAvailable()) {
- unregisterMapping(map);
- }
+ unregisterMapping(map);
}
}
}
@@ -695,9 +702,7 @@ UPnPContext::pruneMappingsWithInvalidIgds(const std::shared_ptr<IGD>& igd)
igd->getProtocolName());
map->cancelTimeoutTimer();
updateMappingState(map, MappingState::FAILED);
- if (map->isAvailable()) {
- unregisterMapping(map);
- }
+ unregisterMapping(map);
}
}
@@ -922,10 +927,7 @@ UPnPContext::onMappingAdded(const std::shared_ptr<IGD>& igd, const Mapping& mapR
map->toString().c_str(),
igd->getLocalIp().toString().c_str());
updateMappingState(map, MappingState::FAILED);
- // Remove if not used.
- if (map->isAvailable()) {
- unregisterMapping(map);
- }
+ unregisterMapping(map);
}
// We have a response, so cancel the time-out.
@@ -1195,11 +1197,7 @@ UPnPContext::onRequestTimeOut(const std::shared_ptr<IGD>& igd, const Mapping::sh
// Considere time-out as failure.
updateMappingState(map, MappingState::FAILED);
-
- // Remove it if not used.
- if (map->isAvailable()) {
- unregisterMapping(map);
- }
+ unregisterMapping(map);
}
void
--
GitLab