upnp: Panics when defaulting for Toggle

[dariusc93]

Summary

When using Toggle for upnp behaviour, it panics due to the oneshot receiver dropping. This happens when performing something along the lines of

let enable_upnp = false;
let behaviour = Toggle::from(enable_upnp.then_some(libp2p::upnp::tokio::Behaviour::default()));

which would be drop. While the solution to get around this would to maybe use Toggle::from(protocols.upnp.then(libp2p::upnp::tokio::Behaviour::default)) to have it lazily evaluated or expand on the check, I do feel this is a bad behaviour for it to be doing this on default while using then_some

Expected behavior

Not to panic when behaviour is disabled

Actual behavior

Panics due to a dropped oneshot receiver

Relevant log output

thread 'tokio-runtime-worker' panicked at protocols/upnp/src/tokio.rs:126:14:
receiver shouldn't have been dropped: Ok(Gateway { sender: Sender { closed: false }, receiver: Receiver { closed: false }, external_addr: XXX.XXX.XXX.XXX })

Possible Solution

1. Check the oneshot sender and dont panic if there is an error; or
2. Add GatewayState::Pending (as an example) and change it to GatewayState::Searching on the first poll, which would prevent the panic that occurs due to the oneshot receiver dropping. The following should suffice

diff --git a/protocols/upnp/src/behaviour.rs b/protocols/upnp/src/behaviour.rs
index a94ef9526..ed783cbd7 100644
--- a/protocols/upnp/src/behaviour.rs
+++ b/protocols/upnp/src/behaviour.rs
@@ -126,6 +126,7 @@ enum MappingState {
 
 /// Current state of the UPnP [`Gateway`].
 enum GatewayState {
+    Pending,
     Searching(oneshot::Receiver<Result<Gateway, Box<dyn std::error::Error + Send + Sync>>>),
     Available(Gateway),
     GatewayNotFound,
@@ -220,7 +221,7 @@ pub struct Behaviour {
 impl Default for Behaviour {
     fn default() -> Self {
         Self {
-            state: GatewayState::Searching(crate::tokio::search_gateway()),
+            state: GatewayState::Pending,
             mappings: Default::default(),
             pending_events: VecDeque::new(),
         }
@@ -280,7 +281,7 @@ impl NetworkBehaviour for Behaviour {
                 }
 
                 match &mut self.state {
-                    GatewayState::Searching(_) => {
+                    GatewayState::Pending | GatewayState::Searching(_) => {
                         // As the gateway is not yet available we add the mapping with `MappingState::Inactive`
                         // so that when and if it becomes available we map it.
                         self.mappings.insert(
@@ -378,6 +379,9 @@ impl NetworkBehaviour for Behaviour {
         // we poll the pending mapping requests.
         loop {
             match self.state {
+                GatewayState::Pending => {
+                    self.state = GatewayState::Searching(crate::tokio::search_gateway())
+                }
                 GatewayState::Searching(ref mut fut) => match Pin::new(fut).poll(cx) {
                     Poll::Ready(result) => {
                         match result.expect("sender shouldn't have been dropped") {

Version

0.53.2

Would you like to work on fixing this bug ?

Yes

[jxs]

Hi Darius, and thanks for reporting this, nice catch.
The proposed solution is very elegant, the only downside would be that if someone can still poll the behaviour manually and then drop it right after triggering the panics. We could just address the panics themselves:

diff --git a/protocols/upnp/src/tokio.rs b/protocols/upnp/src/tokio.rs
index c6a40182b..9c8b2cafe 100644
--- a/protocols/upnp/src/tokio.rs
+++ b/protocols/upnp/src/tokio.rs
@@ -100,9 +100,7 @@ pub(crate) fn search_gateway() -> oneshot::Receiver<Result<Gateway, Box<dyn Erro
         let gateway = match igd_next::aio::tokio::search_gateway(SearchOptions::default()).await {
             Ok(gateway) => gateway,
             Err(err) => {
-                search_result_sender
-                    .send(Err(err.into()))
-                    .expect("receiver shouldn't have been dropped");
+                let _ = search_result_sender.send(Err(err.into()));
                 return;
             }
         };
@@ -110,20 +108,22 @@ pub(crate) fn search_gateway() -> oneshot::Receiver<Result<Gateway, Box<dyn Erro
         let external_addr = match gateway.get_external_ip().await {
             Ok(addr) => addr,
             Err(err) => {
-                search_result_sender
-                    .send(Err(err.into()))
-                    .expect("receiver shouldn't have been dropped");
+                let _ = search_result_sender.send(Err(err.into()));
                 return;
             }
         };
 
-        search_result_sender
+        // Check if receiver dropped.
+        if search_result_sender
             .send(Ok(Gateway {
                 sender: events_sender,
                 receiver: events_queue,
                 external_addr,
             }))
-            .expect("receiver shouldn't have been dropped");
+            .is_err()
+        {
+            return;
+        }
 
         loop {
             // The task sender has dropped so we can return.

wdyt?

[dariusc93]

Hi Darius, and thanks for reporting this, nice catch. The proposed solution is very elegant, the only downside would be that if someone can still poll the behaviour manually and then drop it right after triggering the panics. We could just address the panics themselves:

diff --git a/protocols/upnp/src/tokio.rs b/protocols/upnp/src/tokio.rs
index c6a40182b..9c8b2cafe 100644
--- a/protocols/upnp/src/tokio.rs
+++ b/protocols/upnp/src/tokio.rs
@@ -100,9 +100,7 @@ pub(crate) fn search_gateway() -> oneshot::Receiver<Result<Gateway, Box<dyn Erro
         let gateway = match igd_next::aio::tokio::search_gateway(SearchOptions::default()).await {
             Ok(gateway) => gateway,
             Err(err) => {
-                search_result_sender
-                    .send(Err(err.into()))
-                    .expect("receiver shouldn't have been dropped");
+                let _ = search_result_sender.send(Err(err.into()));
                 return;
             }
         };
@@ -110,20 +108,22 @@ pub(crate) fn search_gateway() -> oneshot::Receiver<Result<Gateway, Box<dyn Erro
         let external_addr = match gateway.get_external_ip().await {
             Ok(addr) => addr,
             Err(err) => {
-                search_result_sender
-                    .send(Err(err.into()))
-                    .expect("receiver shouldn't have been dropped");
+                let _ = search_result_sender.send(Err(err.into()));
                 return;
             }
         };
 
-        search_result_sender
+        // Check if receiver dropped.
+        if search_result_sender
             .send(Ok(Gateway {
                 sender: events_sender,
                 receiver: events_queue,
                 external_addr,
             }))
-            .expect("receiver shouldn't have been dropped");
+            .is_err()
+        {
+            return;
+        }
 
         loop {
             // The task sender has dropped so we can return.

wdyt?

Looks good to me :) Although i dont know of a case where one would poll it manually, however I do like the idea of addressing it directly