Skip to content

Commit

Permalink
upnp_context: unregister all FAILED mappings
Browse files Browse the repository at this point in the history
This patch fixes a bug where FAILED mappings with the "auto-update"
option enabled are left in the local list, even after a new mapping has
been requested. This can cause the list to grow substantially over time
and was partly responsible for the bug described in the following issue:
https://git.jami.net/savoirfairelinux/jami-client-qt/-/issues/1898

GitLab: #45
Change-Id: I009ff791ae90d10f64d46dede3b2cbfc9c1b7160
  • Loading branch information
François-Simon Fauteux-Chapleau committed Jan 15, 2025
1 parent b92387a commit a406c4a
Showing 1 changed file with 15 additions and 8 deletions.
23 changes: 15 additions & 8 deletions src/upnp/upnp_context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -630,12 +630,15 @@ UPnPContext::enforceAvailableMappingsLimits()
int pendingCount = 0;
int inProgressCount = 0;
int openCount = 0;
int unavailableCount = 0;
{
std::lock_guard lock(mappingMutex_);
const auto& mappingList = getMappingList(type);
for (const auto& [_, mapping] : mappingList) {
if (!mapping->isAvailable())
if (!mapping->isAvailable()) {
unavailableCount++;
continue;
}
switch (mapping->getState()) {
case MappingState::PENDING:
pendingCount++;
Expand All @@ -652,12 +655,13 @@ UPnPContext::enforceAvailableMappingsLimits()
}
}
int availableCount = openCount + pendingCount + inProgressCount;
if (logger_) logger_->debug("Number of 'available' {} mappings in the local list: {} ({} open + {} pending + {} in progress)",
if (logger_) logger_->debug("Number of {} mappings in the local list: {} available ({} open + {} pending + {} in progress), {} unavailable",
Mapping::getTypeStr(type),
availableCount,
openCount,
pendingCount,
inProgressCount);
inProgressCount,
unavailableCount);

int minAvailableMappings = getMinAvailableMappings(type);
if (minAvailableMappings > availableCount) {
Expand Down Expand Up @@ -1307,16 +1311,19 @@ UPnPContext::handleFailedMapping(const Mapping::sharedPtr_t& map)
}

if (isReady()) {
// We have a valid IGD, but the mapping request
// failed, so try again with a new port number.
Mapping newMapping(map->getType());
newMapping.enableAutoUpdate(true);
newMapping.setNotifyCallback(map->getNotifyCallback());
reserveMapping(newMapping);
if (logger_) logger_->debug("Mapping {} has auto-update enabled, a new mapping will be requested",
map->toString());

// TODO: figure out if this line is actually necessary
// (See https://review.jami.net/c/jami-daemon/+/16940)
// Remove the old (failed) mapping from the local list
map->setNotifyCallback(nullptr);
unregisterMapping(map);

if (logger_) logger_->debug("Mapping {} had auto-update enabled, a new mapping will be requested",
map->toString());
reserveMapping(newMapping);
} else {
// If there is no valid IGD, the mapping is marked as pending
// and will be requested when an IGD becomes available.
Expand Down

0 comments on commit a406c4a

Please sign in to comment.