Skip to content

Commit ecc1153

Browse files
RolandSherwinRolandSherwin
committed
fix(upnp): add exponential backoff for failed port mappings
1 parent 1755bca commit ecc1153

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)