diff --git a/protocols/upnp/CHANGELOG.md b/protocols/upnp/CHANGELOG.md index 24735db8433..9d8fa14e88e 100644 --- a/protocols/upnp/CHANGELOG.md +++ b/protocols/upnp/CHANGELOG.md @@ -6,6 +6,11 @@ checks active mappings on the gateway. See [PR 6127](https://github.com/libp2p/rust-libp2p/pull/6127). +- Fix excessive retry attempts for failed port mappings by implementing exponential backoff. + Failed mappings now retry up to 5 times with increasing delays (30s to 480s) before giving up. + This prevents continuous retry loops. + See [PR 6128](https://github.com/libp2p/rust-libp2p/pull/6128). + ## 0.5.0 - update igd-next to 0.16.1 diff --git a/protocols/upnp/src/behaviour.rs b/protocols/upnp/src/behaviour.rs index 4bf80cb1c2a..3c296ff56c7 100644 --- a/protocols/upnp/src/behaviour.rs +++ b/protocols/upnp/src/behaviour.rs @@ -53,6 +53,15 @@ const MAPPING_DURATION: u32 = 3600; /// Renew the Mapping every half of `MAPPING_DURATION` to avoid the port being unmapped. const MAPPING_TIMEOUT: u64 = MAPPING_DURATION as u64 / 2; +/// Maximum number of retry attempts for failed mappings. +const MAX_RETRY_ATTEMPTS: u32 = 5; + +/// Base delay in seconds for exponential backoff (will be multiplied by 2^retry_count). +const BASE_RETRY_DELAY_SECS: u64 = 30; + +/// Maximum delay in seconds between retry attempts. +const MAX_RETRY_DELAY_SECS: u64 = 1800; + /// A [`Gateway`] Request. #[derive(Debug)] pub(crate) enum GatewayRequest { @@ -122,11 +131,19 @@ enum MappingState { /// Port mapping is inactive, will be requested or re-requested on the next iteration. Inactive, /// Port mapping/removal has been requested on the gateway. - Pending, + Pending { + /// Number of times we've tried to map this port. + retry_count: u32, + }, /// Port mapping is active with the inner timeout. Active(Delay), - /// Port mapping failed, we will try again. - Failed, + /// Port mapping failed with retry information. + Failed { + /// Number of times we've tried to map this port. + retry_count: u32, + /// When we should try again (None means no more retries). + next_retry: Option, + }, } /// Current state of the UPnP [`Gateway`]. @@ -174,7 +191,7 @@ impl MappingList { fn renew(&mut self, gateway: &mut Gateway, cx: &mut Context<'_>) { for (mapping, state) in self.iter_mut() { match state { - MappingState::Inactive | MappingState::Failed => { + MappingState::Inactive => { let duration = MAPPING_DURATION; if let Err(err) = gateway.sender.try_send(GatewayRequest::AddMapping { mapping: mapping.clone(), @@ -185,8 +202,34 @@ impl MappingList { "could not request port mapping for multiaddress on the gateway: {}", err ); + } else { + *state = MappingState::Pending { retry_count: 0 }; + } + } + MappingState::Failed { + retry_count, + next_retry, + } => { + if let Some(delay) = next_retry { + if Pin::new(delay).poll(cx).is_ready() { + let duration = MAPPING_DURATION; + if let Err(err) = gateway.sender.try_send(GatewayRequest::AddMapping { + mapping: mapping.clone(), + duration, + }) { + tracing::debug!( + multiaddress=%mapping.multiaddr, + retry_count=%retry_count, + "could not retry port mapping for multiaddress on the gateway: {}", + err + ); + } else { + *state = MappingState::Pending { + retry_count: *retry_count, + }; + } + } } - *state = MappingState::Pending; } MappingState::Active(timeout) => { if Pin::new(timeout).poll(cx).is_ready() { @@ -203,7 +246,7 @@ impl MappingList { } } } - MappingState::Pending => {} + MappingState::Pending { .. } => {} } } } @@ -317,7 +360,8 @@ impl NetworkBehaviour for Behaviour { ); } - self.mappings.insert(mapping, MappingState::Pending); + self.mappings + .insert(mapping, MappingState::Pending { retry_count: 0 }); } GatewayState::GatewayNotFound => { tracing::debug!( @@ -351,7 +395,8 @@ impl NetworkBehaviour for Behaviour { err ); } - self.mappings.insert(mapping, MappingState::Pending); + self.mappings + .insert(mapping, MappingState::Pending { retry_count: 0 }); } } } @@ -427,7 +472,7 @@ impl NetworkBehaviour for Behaviour { .insert(mapping.clone(), new_state) .expect("mapping should exist") { - MappingState::Pending => { + MappingState::Pending { .. } => { let external_multiaddr = mapping.external_addr(gateway.external_addr); self.pending_events.push_back(Event::NewExternalAddr( @@ -453,36 +498,64 @@ impl NetworkBehaviour for Behaviour { } } GatewayEvent::MapFailure(mapping, err) => { - match self - .mappings - .insert(mapping.clone(), MappingState::Failed) - .expect("mapping should exist") - { - MappingState::Active(_) => { - tracing::debug!( - address=%mapping.internal_addr, - protocol=%mapping.protocol, - "failed to remap UPnP mapped for protocol: {err}" - ); - let external_multiaddr = - mapping.external_addr(gateway.external_addr); - self.pending_events.push_back(Event::ExpiredExternalAddr( - external_multiaddr.clone(), - )); - return Poll::Ready(ToSwarm::ExternalAddrExpired( - external_multiaddr, - )); - } - MappingState::Pending => { - tracing::debug!( - address=%mapping.internal_addr, - protocol=%mapping.protocol, - "failed to map UPnP mapped for protocol: {err}" - ); - } - _ => { - unreachable!() + let prev_state = + self.mappings.get(&mapping).expect("mapping should exist"); + + let (retry_count, was_active) = match prev_state { + MappingState::Active(_) => (0, true), + MappingState::Pending { retry_count } => (*retry_count, false), + MappingState::Failed { retry_count, .. } => { + (*retry_count, false) } + _ => unreachable!(), + }; + + let new_retry_count = retry_count + 1; + let next_retry = if new_retry_count < MAX_RETRY_ATTEMPTS { + let delay_secs = std::cmp::min( + BASE_RETRY_DELAY_SECS + .saturating_mul(2_u64.pow(retry_count)), + MAX_RETRY_DELAY_SECS, + ); + Some(Delay::new(Duration::from_secs(delay_secs))) + } else { + tracing::warn!( + address=%mapping.internal_addr, + protocol=%mapping.protocol, + "giving up on UPnP mapping after {new_retry_count} attempts" + ); + None + }; + + self.mappings.insert( + mapping.clone(), + MappingState::Failed { + retry_count: new_retry_count, + next_retry, + }, + ); + + if was_active { + tracing::debug!( + address=%mapping.internal_addr, + protocol=%mapping.protocol, + "failed to remap UPnP mapped for protocol: {err}" + ); + let external_multiaddr = + mapping.external_addr(gateway.external_addr); + self.pending_events.push_back(Event::ExpiredExternalAddr( + external_multiaddr.clone(), + )); + return Poll::Ready(ToSwarm::ExternalAddrExpired( + external_multiaddr, + )); + } else { + tracing::debug!( + address=%mapping.internal_addr, + protocol=%mapping.protocol, + retry_count=%new_retry_count, + "failed to map UPnP mapped for protocol: {err}" + ); } } GatewayEvent::Removed(mapping) => {