transports/tcp: Fix port reuse using Arc<RwLock> for listen_addrs #2670

stormshield-pj50 · 2022-05-23T11:52:42Z

Description

See reported issue #2651 for details.

Change checklist

two tests in transports/tcp (port_reuse_dialing and port_reuse_listening) have been enhanced for testing fixed regression.
protocols/dcutr/examples test has been run successfully with the fix.
I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

mxinden

Thank you @Pj50 for providing the patch as well as augmenting the test.

With this change, I find the cloning behavior of GenTcpConfig non-intuitive, given that the clone would share the listen_addrs field when PortReuse::Enabled.

What do you think of removing the derived Clone on GenTcpConfig?

diff --git a/transports/tcp/src/lib.rs b/transports/tcp/src/lib.rs
index 6e6eb771..0badcc24 100644
--- a/transports/tcp/src/lib.rs
+++ b/transports/tcp/src/lib.rs
@@ -66,7 +66,7 @@ use std::{
 use provider::{IfEvent, Provider};
 
 /// The configuration for a TCP/IP transport capability for libp2p.
-#[derive(Clone, Debug)]
+#[derive(Debug)]
 pub struct GenTcpConfig<T> {
     /// The type of the I/O provider.
     _impl: std::marker::PhantomData<T>,
@@ -247,7 +247,7 @@ where
     /// let listen_addr2: Multiaddr = "/ip4/127.0.0.1/tcp/9002".parse().unwrap();
     ///
     /// let mut tcp1 = TcpConfig::new().port_reuse(true);
-    /// let mut listener1 = tcp1.clone().listen_on(listen_addr1.clone()).expect("listener");
+    /// let mut listener1 = tcp1.listen_on(listen_addr1.clone()).expect("listener");
     /// match listener1.next().await.expect("event")? {
     ///     ListenerEvent::NewAddress(listen_addr) => {
     ///         println!("Listening on {:?}", listen_addr);
@@ -258,7 +258,7 @@ where
     /// }
     ///
     /// let mut tcp2 = TcpConfig::new().port_reuse(true);
-    /// let mut listener2 = tcp2.clone().listen_on(listen_addr2).expect("listener");
+    /// let mut listener2 = tcp2.listen_on(listen_addr2).expect("listener");
     /// match listener2.next().await.expect("event")? {
     ///     ListenerEvent::NewAddress(listen_addr) => {
     ///         println!("Listening on {:?}", listen_addr);

mxinden · 2022-05-24T15:27:03Z

transports/tcp/src/lib.rs

-            listen_addrs.insert((ip, port));
+            listen_addrs
+                .write()
+                .expect("`register()` and `unregister()` never panic while holding the lock")


I will update these in a follow up pull request.

If you remove Clone on GenTcpConfig what about GenDnsConfig<GenTcpConfig, ...> which also implements Clone ?

I would advocate for removing Clone from GenDnsConfig as well, though I would prefer that to happen in a separate pull request.

Can GenDnsConfig be Clone even if it is wrapping GenTcpConfig which is not Clone?

Yes, given that GenDnsConfig wraps the GenTcpConfig in an Arc<Mutex<_>>.

Hope I am not missing something @Pj50.

I tried removing Clone from GenDnsConfig, however

rust-libp2p/src/lib.rs

Line 206 in 4aa84bf

let ws_dns_tcp = websocket::WsConfig::new(dns_tcp.clone());

does not compile any more. I don't know how to fix it properly, so I leave it to you, and you're right I think it would be much better to fix it in another pull request.

mxinden · 2022-05-24T15:29:49Z

transports/tcp/src/lib.rs

                    // Obtain a future socket through dialing
+                    // FIXME: Check that the port used by socket is the same as the port returned by local_dial_addr


How about checking the ListenerEvent::Upgrade { remote_addr, ..} in async fn listener above instead?

OK I updated the code using an mpsc channel to pass the reused port between the dialer and the listener, and cheking in the listener upgrade event that the remote port is the same as the one received in the channel.

mxinden · 2022-05-24T15:38:09Z

Sponsored by: Stormshield

You asked whether this can be included in the git history. I am not sure what legal implications this might have. For the sake of simplicity I would not include it. Is that fine by you?

That is not to say, that I don't appreciate the sponsorship! Thanks for the help.

mxinden

Thanks for the addition to the tests!

Followed up on the Clone discussion. What do you think?

mxinden · 2022-05-29T15:46:03Z

transports/tcp/src/lib.rs

@@ -943,8 +985,9 @@ mod tests {
            #[cfg(feature = "async-io")]
            {
                let (ready_tx, ready_rx) = mpsc::channel(1);
-                let listener = listener::<async_io::Tcp>(addr.clone(), ready_tx);
-                let dialer = dialer::<async_io::Tcp>(addr.clone(), ready_rx);
+                let (port_reuse_tx, port_reuse_rx) = mpsc::channel(1);


How about using a oneshot here?

https://docs.rs/futures/latest/futures/channel/oneshot/fn.channel.html

OK. Updated the commit with a oneshot.

mxinden · 2022-05-29T15:46:17Z

transports/tcp/src/lib.rs

+                        // Receive the dialer tcp port reuse
+                        let remote_port_reuse = port_reuse_rx.next().await.unwrap();
+                        // And check it is the same as the remote port used for upgrade
+                        assert_eq!(remote_addr.pop().unwrap(), remote_port_reuse);


mxinden · 2022-05-29T15:49:35Z

transports/tcp/src/lib.rs

-            listen_addrs.insert((ip, port));
+            listen_addrs
+                .write()
+                .expect("`register()` and `unregister()` never panic while holding the lock")


I would advocate for removing Clone from GenDnsConfig as well, though I would prefer that to happen in a separate pull request.

Can GenDnsConfig be Clone even if it is wrapping GenTcpConfig which is not Clone?

Yes, given that GenDnsConfig wraps the GenTcpConfig in an Arc<Mutex<_>>.

Hope I am not missing something @Pj50.

listen_addrs

transports/tcp/src/lib.rs

mxinden

Thanks @Pj50 for the help here, going all the way from debugging to proposing the fix 🙏

mxinden reviewed May 24, 2022

View reviewed changes

stormshield-pj50 force-pushed the issue_2651_portreuse_dialing branch 2 times, most recently from 3b0c773 to fb6e3be Compare May 25, 2022 09:35

mxinden reviewed May 29, 2022

View reviewed changes

Fix issue libp2p#2651 - Make port reuse work again using Arc<RwLock> for

6864348

listen_addrs

stormshield-pj50 force-pushed the issue_2651_portreuse_dialing branch from fb6e3be to 6864348 Compare May 30, 2022 08:09

mxinden changed the title ~~Fix issue #2651 - Make port reuse work again using Arc<RwLock> for listen_addrs~~ transports/tcp: Make port reuse work again using Arc<RwLock> for listen_addrs May 30, 2022

mxinden changed the title ~~transports/tcp: Make port reuse work again using Arc<RwLock> for listen_addrs~~ transports/tcp: Fix port reuse using Arc<RwLock> for listen_addrs May 30, 2022

Merge branch 'master' into issue_2651_portreuse_dialing

ec69d2a

mxinden reviewed May 30, 2022

View reviewed changes

transports/tcp/src/lib.rs Outdated Show resolved Hide resolved

Update transports/tcp/src/lib.rs

0d1267b

mxinden approved these changes May 30, 2022

View reviewed changes

mxinden merged commit 2804756 into libp2p:master May 30, 2022

This was referenced May 30, 2022

transports/{tcp,dns,websocket}: Remove Clone implementation for *Config #2682

Merged

Tcp transport port_reuse_dialing test missing some assert #2651

Closed

Can't get hole-punching tutorial to work reliably #2609

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

transports/tcp: Fix port reuse using Arc<RwLock> for listen_addrs #2670

transports/tcp: Fix port reuse using Arc<RwLock> for listen_addrs #2670

stormshield-pj50 commented May 23, 2022 •

edited

mxinden left a comment

mxinden May 24, 2022

stormshield-pj50 May 25, 2022

mxinden May 29, 2022

stormshield-pj50 May 30, 2022 •

edited

mxinden May 24, 2022

stormshield-pj50 May 25, 2022

mxinden commented May 24, 2022

mxinden left a comment

mxinden May 29, 2022

stormshield-pj50 May 30, 2022

mxinden May 29, 2022

mxinden May 29, 2022

mxinden left a comment

		// Obtain a future socket through dialing
		// FIXME: Check that the port used by socket is the same as the port returned by local_dial_addr

transports/tcp: Fix port reuse using Arc<RwLock> for listen_addrs #2670

transports/tcp: Fix port reuse using Arc<RwLock> for listen_addrs #2670

Conversation

stormshield-pj50 commented May 23, 2022 • edited

Description

Change checklist

mxinden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stormshield-pj50 May 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented May 24, 2022

mxinden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden left a comment

Choose a reason for hiding this comment

stormshield-pj50 commented May 23, 2022 •

edited

stormshield-pj50 May 30, 2022 •

edited