Skip to content

Commit aefbfbd

Browse files
RolandSherwinRolandSherwin
authored
fix(upnp): add exponential backoff for failed port mappings
This commit aims to fix an infinite retry loop of failed mappings. We now have an exponential backoff and would retry any failed mappings upto 5 times before giving up. Pull-Request: #6128.
1 parent 1755bca commit aefbfbd

File tree

2 files changed

+116
-38
lines changed

2 files changed

+116
-38
lines changed

‎protocols/upnp/CHANGELOG.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,11 @@
66
checks active mappings on the gateway.
77
See [PR 6127](https://github.com/libp2p/rust-libp2p/pull/6127).
88

9+
- Fix excessive retry attempts for failed port mappings by implementing exponential backoff.
10+
Failed mappings now retry up to 5 times with increasing delays (30s to 480s) before giving up.
11+
This prevents continuous retry loops.
12+
See [PR 6128](https://github.com/libp2p/rust-libp2p/pull/6128).
13+
914
## 0.5.0
1015

1116
- update igd-next to 0.16.1

‎protocols/upnp/src/behaviour.rs

Lines changed: 111 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,15 @@ const MAPPING_DURATION: u32 = 3600;
5353
/// Renew the Mapping every half of `MAPPING_DURATION` to avoid the port being unmapped.
5454
const MAPPING_TIMEOUT: u64 = MAPPING_DURATION as u64 / 2;
5555

56+
/// Maximum number of retry attempts for failed mappings.
57+
const MAX_RETRY_ATTEMPTS: u32 = 5;
58+
59+
/// Base delay in seconds for exponential backoff (will be multiplied by 2^retry_count).
60+
const BASE_RETRY_DELAY_SECS: u64 = 30;
61+
62+
/// Maximum delay in seconds between retry attempts.
63+
const MAX_RETRY_DELAY_SECS: u64 = 1800;
64+
5665
/// A [`Gateway`] Request.
5766
#[derive(Debug)]
5867
pub(crate) enum GatewayRequest {
@@ -122,11 +131,19 @@ enum MappingState {
122131
/// Port mapping is inactive, will be requested or re-requested on the next iteration.
123132
Inactive,
124133
/// Port mapping/removal has been requested on the gateway.
125-
Pending,
134+
Pending {
135+
/// Number of times we've tried to map this port.
136+
retry_count: u32,
137+
},
126138
/// Port mapping is active with the inner timeout.
127139
Active(Delay),
128-
/// Port mapping failed, we will try again.
129-
Failed,
140+
/// Port mapping failed with retry information.
141+
Failed {
142+
/// Number of times we've tried to map this port.
143+
retry_count: u32,
144+
/// When we should try again (None means no more retries).
145+
next_retry: Option<Delay>,
146+
},
130147
}
131148

132149
/// Current state of the UPnP [`Gateway`].
@@ -174,7 +191,7 @@ impl MappingList {
174191
fn renew(&mut self, gateway: &mut Gateway, cx: &mut Context<'_>) {
175192
for (mapping, state) in self.iter_mut() {
176193
match state {
177-
MappingState::Inactive | MappingState::Failed => {
194+
MappingState::Inactive => {
178195
let duration = MAPPING_DURATION;
179196
if let Err(err) = gateway.sender.try_send(GatewayRequest::AddMapping {
180197
mapping: mapping.clone(),
@@ -185,8 +202,34 @@ impl MappingList {
185202
"could not request port mapping for multiaddress on the gateway: {}",
186203
err
187204
);
205+
} else {
206+
*state = MappingState::Pending { retry_count: 0 };
207+
}
208+
}
209+
MappingState::Failed {
210+
retry_count,
211+
next_retry,
212+
} => {
213+
if let Some(delay) = next_retry {
214+
if Pin::new(delay).poll(cx).is_ready() {
215+
let duration = MAPPING_DURATION;
216+
if let Err(err) = gateway.sender.try_send(GatewayRequest::AddMapping {
217+
mapping: mapping.clone(),
218+
duration,
219+
}) {
220+
tracing::debug!(
221+
multiaddress=%mapping.multiaddr,
222+
retry_count=%retry_count,
223+
"could not retry port mapping for multiaddress on the gateway: {}",
224+
err
225+
);
226+
} else {
227+
*state = MappingState::Pending {
228+
retry_count: *retry_count,
229+
};
230+
}
231+
}
188232
}
189-
*state = MappingState::Pending;
190233
}
191234
MappingState::Active(timeout) => {
192235
if Pin::new(timeout).poll(cx).is_ready() {
@@ -203,7 +246,7 @@ impl MappingList {
203246
}
204247
}
205248
}
206-
MappingState::Pending => {}
249+
MappingState::Pending { .. } => {}
207250
}
208251
}
209252
}
@@ -317,7 +360,8 @@ impl NetworkBehaviour for Behaviour {
317360
);
318361
}
319362

320-
self.mappings.insert(mapping, MappingState::Pending);
363+
self.mappings
364+
.insert(mapping, MappingState::Pending { retry_count: 0 });
321365
}
322366
GatewayState::GatewayNotFound => {
323367
tracing::debug!(
@@ -351,7 +395,8 @@ impl NetworkBehaviour for Behaviour {
351395
err
352396
);
353397
}
354-
self.mappings.insert(mapping, MappingState::Pending);
398+
self.mappings
399+
.insert(mapping, MappingState::Pending { retry_count: 0 });
355400
}
356401
}
357402
}
@@ -427,7 +472,7 @@ impl NetworkBehaviour for Behaviour {
427472
.insert(mapping.clone(), new_state)
428473
.expect("mapping should exist")
429474
{
430-
MappingState::Pending => {
475+
MappingState::Pending { .. } => {
431476
let external_multiaddr =
432477
mapping.external_addr(gateway.external_addr);
433478
self.pending_events.push_back(Event::NewExternalAddr(
@@ -453,36 +498,64 @@ impl NetworkBehaviour for Behaviour {
453498
}
454499
}
455500
GatewayEvent::MapFailure(mapping, err) => {
456-
match self
457-
.mappings
458-
.insert(mapping.clone(), MappingState::Failed)
459-
.expect("mapping should exist")
460-
{
461-
MappingState::Active(_) => {
462-
tracing::debug!(
463-
address=%mapping.internal_addr,
464-
protocol=%mapping.protocol,
465-
"failed to remap UPnP mapped for protocol: {err}"
466-
);
467-
let external_multiaddr =
468-
mapping.external_addr(gateway.external_addr);
469-
self.pending_events.push_back(Event::ExpiredExternalAddr(
470-
external_multiaddr.clone(),
471-
));
472-
return Poll::Ready(ToSwarm::ExternalAddrExpired(
473-
external_multiaddr,
474-
));
475-
}
476-
MappingState::Pending => {
477-
tracing::debug!(
478-
address=%mapping.internal_addr,
479-
protocol=%mapping.protocol,
480-
"failed to map UPnP mapped for protocol: {err}"
481-
);
482-
}
483-
_ => {
484-
unreachable!()
501+
let prev_state =
502+
self.mappings.get(&mapping).expect("mapping should exist");
503+
504+
let (retry_count, was_active) = match prev_state {
505+
MappingState::Active(_) => (0, true),
506+
MappingState::Pending { retry_count } => (*retry_count, false),
507+
MappingState::Failed { retry_count, .. } => {
508+
(*retry_count, false)
485509
}
510+
_ => unreachable!(),
511+
};
512+
513+
let new_retry_count = retry_count + 1;
514+
let next_retry = if new_retry_count < MAX_RETRY_ATTEMPTS {
515+
let delay_secs = std::cmp::min(
516+
BASE_RETRY_DELAY_SECS
517+
.saturating_mul(2_u64.pow(retry_count)),
518+
MAX_RETRY_DELAY_SECS,
519+
);
520+
Some(Delay::new(Duration::from_secs(delay_secs)))
521+
} else {
522+
tracing::warn!(
523+
address=%mapping.internal_addr,
524+
protocol=%mapping.protocol,
525+
"giving up on UPnP mapping after {new_retry_count} attempts"
526+
);
527+
None
528+
};
529+
530+
self.mappings.insert(
531+
mapping.clone(),
532+
MappingState::Failed {
533+
retry_count: new_retry_count,
534+
next_retry,
535+
},
536+
);
537+
538+
if was_active {
539+
tracing::debug!(
540+
address=%mapping.internal_addr,
541+
protocol=%mapping.protocol,
542+
"failed to remap UPnP mapped for protocol: {err}"
543+
);
544+
let external_multiaddr =
545+
mapping.external_addr(gateway.external_addr);
546+
self.pending_events.push_back(Event::ExpiredExternalAddr(
547+
external_multiaddr.clone(),
548+
));
549+
return Poll::Ready(ToSwarm::ExternalAddrExpired(
550+
external_multiaddr,
551+
));
552+
} else {
553+
tracing::debug!(
554+
address=%mapping.internal_addr,
555+
protocol=%mapping.protocol,
556+
retry_count=%new_retry_count,
557+
"failed to map UPnP mapped for protocol: {err}"
558+
);
486559
}
487560
}
488561
GatewayEvent::Removed(mapping) => {

0 commit comments

Comments
 (0)