From e5442ece468261ffda9b639a9459f87bdf37f8c3 Mon Sep 17 00:00:00 2001 From: sukun Date: Tue, 14 Feb 2023 14:35:59 +0530 Subject: [PATCH 1/5] do not start new relay if one already exists --- p2p/host/relaysvc/relay.go | 10 +++++++--- p2p/host/relaysvc/relay_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 3 deletions(-) create mode 100644 p2p/host/relaysvc/relay_test.go diff --git a/p2p/host/relaysvc/relay.go b/p2p/host/relaysvc/relay.go index bea8aa73a4..371bb16bc9 100644 --- a/p2p/host/relaysvc/relay.go +++ b/p2p/host/relaysvc/relay.go @@ -65,14 +65,18 @@ func (m *RelayManager) background(ctx context.Context) { func (m *RelayManager) reachabilityChanged(r network.Reachability) error { switch r { case network.ReachabilityPublic: + m.mutex.Lock() + defer m.mutex.Unlock() + // ignore events with no reachability change + if m.relay != nil { + return nil + } relay, err := relayv2.New(m.host, m.opts...) if err != nil { return err } - m.mutex.Lock() - defer m.mutex.Unlock() m.relay = relay - case network.ReachabilityPrivate: + default: m.mutex.Lock() defer m.mutex.Unlock() if m.relay != nil { diff --git a/p2p/host/relaysvc/relay_test.go b/p2p/host/relaysvc/relay_test.go new file mode 100644 index 0000000000..58506fa761 --- /dev/null +++ b/p2p/host/relaysvc/relay_test.go @@ -0,0 +1,29 @@ +package relaysvc + +import ( + "testing" + + "github.com/libp2p/go-libp2p/core/network" + bhost "github.com/libp2p/go-libp2p/p2p/host/blank" + swarmt "github.com/libp2p/go-libp2p/p2p/net/swarm/testing" + "github.com/stretchr/testify/require" +) + +func TestReachabilityChangeEvent(t *testing.T) { + h := bhost.NewBlankHost(swarmt.GenSwarm(t)) + rmgr := NewRelayManager(h) + rmgr.reachabilityChanged(network.ReachabilityPublic) + require.NotNil(t, rmgr.relay, "relay should be set on public reachability") + + rmgr.reachabilityChanged(network.ReachabilityPrivate) + require.Nil(t, rmgr.relay, "relay should be nil on private reachability") + + rmgr.reachabilityChanged(network.ReachabilityPublic) + rmgr.reachabilityChanged(network.ReachabilityUnknown) + require.Nil(t, rmgr.relay, "relay should be nil on unknown reachability") + + rmgr.reachabilityChanged(network.ReachabilityPublic) + relay := rmgr.relay + rmgr.reachabilityChanged(network.ReachabilityPublic) + require.Equal(t, relay, rmgr.relay, "relay should not be started on receiving the same event") +} From cacb452c91adc15c38bf5442095ddaf5f60276cd Mon Sep 17 00:00:00 2001 From: Sukun Date: Wed, 15 Feb 2023 12:33:12 +0530 Subject: [PATCH 2/5] Update p2p/host/relaysvc/relay.go Co-authored-by: Marten Seemann --- p2p/host/relaysvc/relay.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/p2p/host/relaysvc/relay.go b/p2p/host/relaysvc/relay.go index 371bb16bc9..f9bbc7588e 100644 --- a/p2p/host/relaysvc/relay.go +++ b/p2p/host/relaysvc/relay.go @@ -67,7 +67,8 @@ func (m *RelayManager) reachabilityChanged(r network.Reachability) error { case network.ReachabilityPublic: m.mutex.Lock() defer m.mutex.Unlock() - // ignore events with no reachability change + // This could happen if two consecutive EvtLocalReachabilityChanged report the same reachability. + // This shouldn't happen, but it's safer to double-check. if m.relay != nil { return nil } From 6be1bd4b0a6397e2df77e664180511310860c4d7 Mon Sep 17 00:00:00 2001 From: sukun Date: Wed, 15 Feb 2023 12:59:49 +0530 Subject: [PATCH 3/5] test by emitting events --- p2p/host/relaysvc/relay_test.go | 36 +++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/p2p/host/relaysvc/relay_test.go b/p2p/host/relaysvc/relay_test.go index 58506fa761..29ba592121 100644 --- a/p2p/host/relaysvc/relay_test.go +++ b/p2p/host/relaysvc/relay_test.go @@ -2,28 +2,42 @@ package relaysvc import ( "testing" + "time" + "github.com/libp2p/go-libp2p/core/event" "github.com/libp2p/go-libp2p/core/network" bhost "github.com/libp2p/go-libp2p/p2p/host/blank" + "github.com/libp2p/go-libp2p/p2p/host/eventbus" swarmt "github.com/libp2p/go-libp2p/p2p/net/swarm/testing" + relayv2 "github.com/libp2p/go-libp2p/p2p/protocol/circuitv2/relay" "github.com/stretchr/testify/require" ) func TestReachabilityChangeEvent(t *testing.T) { h := bhost.NewBlankHost(swarmt.GenSwarm(t)) rmgr := NewRelayManager(h) - rmgr.reachabilityChanged(network.ReachabilityPublic) - require.NotNil(t, rmgr.relay, "relay should be set on public reachability") + emitter, err := rmgr.host.EventBus().Emitter(new(event.EvtLocalReachabilityChanged), eventbus.Stateful) + if err != nil { + t.Fatal(err) + } + evt := event.EvtLocalReachabilityChanged{Reachability: network.ReachabilityPublic} + emitter.Emit(evt) + require.Eventually(t, func() bool { return rmgr.relay != nil }, 100*time.Millisecond, 10*time.Millisecond, "relay should be set on public reachability") - rmgr.reachabilityChanged(network.ReachabilityPrivate) - require.Nil(t, rmgr.relay, "relay should be nil on private reachability") + evt = event.EvtLocalReachabilityChanged{Reachability: network.ReachabilityPrivate} + emitter.Emit(evt) + require.Eventually(t, func() bool { return rmgr.relay == nil }, 100*time.Millisecond, 10*time.Millisecond, "relay should be nil on private reachability") - rmgr.reachabilityChanged(network.ReachabilityPublic) - rmgr.reachabilityChanged(network.ReachabilityUnknown) - require.Nil(t, rmgr.relay, "relay should be nil on unknown reachability") + evt = event.EvtLocalReachabilityChanged{Reachability: network.ReachabilityPublic} + emitter.Emit(evt) + evt = event.EvtLocalReachabilityChanged{Reachability: network.ReachabilityUnknown} + emitter.Emit(evt) + require.Eventually(t, func() bool { return rmgr.relay == nil }, 100*time.Millisecond, 10*time.Millisecond, "relay should be nil on unknown reachability") - rmgr.reachabilityChanged(network.ReachabilityPublic) - relay := rmgr.relay - rmgr.reachabilityChanged(network.ReachabilityPublic) - require.Equal(t, relay, rmgr.relay, "relay should not be started on receiving the same event") + evt = event.EvtLocalReachabilityChanged{Reachability: network.ReachabilityPublic} + emitter.Emit(evt) + var relay *relayv2.Relay + require.Eventually(t, func() bool { relay = rmgr.relay; return relay != nil }, 100*time.Millisecond, 10*time.Millisecond) + emitter.Emit(evt) + require.Never(t, func() bool { return relay != rmgr.relay }, 100*time.Millisecond, 10*time.Millisecond, "relay should not be updated on receiving the same event") } From b5bca4f7185100b80ecc943f8bdba4c64c707fde Mon Sep 17 00:00:00 2001 From: sukun Date: Wed, 15 Feb 2023 13:54:08 +0530 Subject: [PATCH 4/5] fix race condition --- p2p/host/relaysvc/relay_test.go | 34 ++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/p2p/host/relaysvc/relay_test.go b/p2p/host/relaysvc/relay_test.go index 29ba592121..ae719f555a 100644 --- a/p2p/host/relaysvc/relay_test.go +++ b/p2p/host/relaysvc/relay_test.go @@ -22,22 +22,46 @@ func TestReachabilityChangeEvent(t *testing.T) { } evt := event.EvtLocalReachabilityChanged{Reachability: network.ReachabilityPublic} emitter.Emit(evt) - require.Eventually(t, func() bool { return rmgr.relay != nil }, 100*time.Millisecond, 10*time.Millisecond, "relay should be set on public reachability") + require.Eventually( + t, + func() bool { rmgr.mutex.Lock(); defer rmgr.mutex.Unlock(); return rmgr.relay != nil }, + 200*time.Millisecond, + 100*time.Millisecond, + "relay should be set on public reachability") evt = event.EvtLocalReachabilityChanged{Reachability: network.ReachabilityPrivate} emitter.Emit(evt) - require.Eventually(t, func() bool { return rmgr.relay == nil }, 100*time.Millisecond, 10*time.Millisecond, "relay should be nil on private reachability") + require.Eventually( + t, + func() bool { rmgr.mutex.Lock(); defer rmgr.mutex.Unlock(); return rmgr.relay == nil }, + 200*time.Millisecond, + 100*time.Millisecond, + "relay should be nil on private reachability") evt = event.EvtLocalReachabilityChanged{Reachability: network.ReachabilityPublic} emitter.Emit(evt) evt = event.EvtLocalReachabilityChanged{Reachability: network.ReachabilityUnknown} emitter.Emit(evt) - require.Eventually(t, func() bool { return rmgr.relay == nil }, 100*time.Millisecond, 10*time.Millisecond, "relay should be nil on unknown reachability") + require.Eventually( + t, + func() bool { rmgr.mutex.Lock(); defer rmgr.mutex.Unlock(); return rmgr.relay == nil }, + 200*time.Millisecond, + 100*time.Millisecond, + "relay should be nil on unknown reachability") evt = event.EvtLocalReachabilityChanged{Reachability: network.ReachabilityPublic} emitter.Emit(evt) var relay *relayv2.Relay - require.Eventually(t, func() bool { relay = rmgr.relay; return relay != nil }, 100*time.Millisecond, 10*time.Millisecond) + require.Eventually( + t, + func() bool { rmgr.mutex.Lock(); defer rmgr.mutex.Unlock(); relay = rmgr.relay; return relay != nil }, + 100*time.Millisecond, + 10*time.Millisecond, + "relay should be set on public event") emitter.Emit(evt) - require.Never(t, func() bool { return relay != rmgr.relay }, 100*time.Millisecond, 10*time.Millisecond, "relay should not be updated on receiving the same event") + require.Never(t, + func() bool { rmgr.mutex.Lock(); defer rmgr.mutex.Unlock(); return relay != rmgr.relay }, + 200*time.Millisecond, + 100*time.Millisecond, + "relay should not be updated on receiving the same event") } From 80e8cc382bbccda8747145d13e062b8ddd3e73c9 Mon Sep 17 00:00:00 2001 From: sukun Date: Wed, 15 Feb 2023 17:24:05 +0530 Subject: [PATCH 5/5] increase timeout --- p2p/host/relaysvc/relay_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/p2p/host/relaysvc/relay_test.go b/p2p/host/relaysvc/relay_test.go index ae719f555a..d995cac200 100644 --- a/p2p/host/relaysvc/relay_test.go +++ b/p2p/host/relaysvc/relay_test.go @@ -25,7 +25,7 @@ func TestReachabilityChangeEvent(t *testing.T) { require.Eventually( t, func() bool { rmgr.mutex.Lock(); defer rmgr.mutex.Unlock(); return rmgr.relay != nil }, - 200*time.Millisecond, + 1*time.Second, 100*time.Millisecond, "relay should be set on public reachability") @@ -34,7 +34,7 @@ func TestReachabilityChangeEvent(t *testing.T) { require.Eventually( t, func() bool { rmgr.mutex.Lock(); defer rmgr.mutex.Unlock(); return rmgr.relay == nil }, - 200*time.Millisecond, + 1*time.Second, 100*time.Millisecond, "relay should be nil on private reachability") @@ -45,7 +45,7 @@ func TestReachabilityChangeEvent(t *testing.T) { require.Eventually( t, func() bool { rmgr.mutex.Lock(); defer rmgr.mutex.Unlock(); return rmgr.relay == nil }, - 200*time.Millisecond, + 1*time.Second, 100*time.Millisecond, "relay should be nil on unknown reachability") @@ -55,13 +55,13 @@ func TestReachabilityChangeEvent(t *testing.T) { require.Eventually( t, func() bool { rmgr.mutex.Lock(); defer rmgr.mutex.Unlock(); relay = rmgr.relay; return relay != nil }, + 1*time.Second, 100*time.Millisecond, - 10*time.Millisecond, "relay should be set on public event") emitter.Emit(evt) require.Never(t, func() bool { rmgr.mutex.Lock(); defer rmgr.mutex.Unlock(); return relay != rmgr.relay }, - 200*time.Millisecond, + 1*time.Second, 100*time.Millisecond, "relay should not be updated on receiving the same event") }