From 5f617f7304b3c3aa74f906069bb316674d83e9e5 Mon Sep 17 00:00:00 2001 From: ramin Date: Wed, 10 Jul 2024 14:56:17 +0100 Subject: [PATCH] move some timeout options to Params so we can change params, but also modify in test environments in a sane way --- share/p2p/discovery/discovery.go | 11 +--- share/p2p/discovery/discovery_test.go | 6 +-- share/p2p/discovery/options.go | 36 ++++++++++++- share/p2p/discovery/options_test.go | 78 +++++++++++++++++++++++++++ share/p2p/peers/manager_test.go | 6 +-- 5 files changed, 118 insertions(+), 19 deletions(-) create mode 100644 share/p2p/discovery/options_test.go diff --git a/share/p2p/discovery/discovery.go b/share/p2p/discovery/discovery.go index 0849985368..592ea4f8dd 100644 --- a/share/p2p/discovery/discovery.go +++ b/share/p2p/discovery/discovery.go @@ -28,18 +28,11 @@ const ( // findPeersTimeout limits the FindPeers operation in time findPeersTimeout = time.Minute - // advertiseRetryTimeout defines time interval between advertise attempts. - advertiseRetryTimeout = time.Second * 1 - // logInterval defines the time interval at which a warning message will be logged // if the desired number of nodes is not detected. logInterval = 5 * time.Minute ) -// discoveryRetryTimeout defines time interval between discovery attempts -// this is set independently for tests in discover_test.go -var DiscoveryRetryTimeout = advertiseRetryTimeout * 60 - // Discovery combines advertise and discover services and allows to store discovered nodes. // TODO: The code here gets horribly hairy, so we should refactor this at some point type Discovery struct { @@ -183,7 +176,7 @@ func (d *Discovery) Advertise(ctx context.Context) { // we don't want retry indefinitely in busy loop // internal discovery mechanism may need some time before attempts select { - case <-time.After(advertiseRetryTimeout): + case <-time.After(d.params.AdvertiseRetryTimeout): if !timer.Stop() { <-timer.C } @@ -210,7 +203,7 @@ func (d *Discovery) Advertise(ctx context.Context) { // It initiates peer discovery upon request and restarts the process until the soft limit is // reached. func (d *Discovery) discoveryLoop(ctx context.Context) { - t := time.NewTicker(DiscoveryRetryTimeout) + t := time.NewTicker(d.params.DiscoveryRetryTimeout) defer t.Stop() warnTicker := time.NewTicker(logInterval) diff --git a/share/p2p/discovery/discovery_test.go b/share/p2p/discovery/discovery_test.go index e4422b1c14..88cf0ec08e 100644 --- a/share/p2p/discovery/discovery_test.go +++ b/share/p2p/discovery/discovery_test.go @@ -26,8 +26,6 @@ const ( func TestDiscovery(t *testing.T) { const nodes = 10 // higher number brings higher coverage - DiscoveryRetryTimeout = time.Millisecond * 100 // defined in discovery.go - ctx, cancel := context.WithTimeout(context.Background(), time.Second*20) t.Cleanup(cancel) @@ -43,7 +41,7 @@ func TestDiscovery(t *testing.T) { } host, routingDisc := tn.peer() - params := DefaultParameters() + params := TestParameters() params.PeersLimit = nodes // start discovery listener service for peerA @@ -103,7 +101,7 @@ func TestDiscoveryTagged(t *testing.T) { // sub will discover both peers, but on different tags sub, routingDisc := tn.peer() - params := DefaultParameters() + params := TestParameters() // create 2 discovery services for sub, each with a different tag done1 := make(chan struct{}) diff --git a/share/p2p/discovery/options.go b/share/p2p/discovery/options.go index 8515bcbe00..b9dd9bbdcd 100644 --- a/share/p2p/discovery/options.go +++ b/share/p2p/discovery/options.go @@ -7,6 +7,11 @@ import ( "github.com/libp2p/go-libp2p/core/peer" ) +const ( + defaultAdvertiseRetryTimeout = time.Second + defaultDiscoveryRetryTimeout = defaultAdvertiseRetryTimeout * 60 +) + // Parameters is the set of Parameters that must be configured for the Discovery module type Parameters struct { // PeersLimit defines the soft limit of FNs to connect to via discovery. @@ -15,6 +20,13 @@ type Parameters struct { // AdvertiseInterval is a interval between advertising sessions. // NOTE: only full and bridge can advertise themselves. AdvertiseInterval time.Duration + + // advertiseRetryTimeout defines time interval between advertise attempts. + AdvertiseRetryTimeout time.Duration + + // DiscoveryRetryTimeout defines time interval between discovery attempts + // this is set independently for tests in discover_test.go + DiscoveryRetryTimeout time.Duration } // options is the set of options that can be configured for the Discovery module @@ -33,13 +45,33 @@ type Option func(*options) // for the Discovery module func DefaultParameters() *Parameters { return &Parameters{ - PeersLimit: 5, - AdvertiseInterval: time.Hour, + PeersLimit: 5, + AdvertiseInterval: time.Hour, + AdvertiseRetryTimeout: defaultAdvertiseRetryTimeout, + DiscoveryRetryTimeout: defaultDiscoveryRetryTimeout, } } +// TestParameters returns the default Parameters' configuration values +// for the Discovery module, with some changes for configuration +// during tests +func TestParameters() *Parameters { + p := DefaultParameters() + p.AdvertiseInterval = time.Second * 1 + p.DiscoveryRetryTimeout = time.Millisecond * 50 + return p +} + // Validate validates the values in Parameters func (p *Parameters) Validate() error { + if p.AdvertiseRetryTimeout <= 0 { + return fmt.Errorf("discovery: advertise retry timeout cannot be zero or negative") + } + + if p.DiscoveryRetryTimeout <= 0 { + return fmt.Errorf("discovery: discovery retry timeout cannot be zero or negative") + } + if p.PeersLimit <= 0 { return fmt.Errorf("discovery: peers limit cannot be zero or negative") } diff --git a/share/p2p/discovery/options_test.go b/share/p2p/discovery/options_test.go new file mode 100644 index 0000000000..b4aa40a55d --- /dev/null +++ b/share/p2p/discovery/options_test.go @@ -0,0 +1,78 @@ +package discovery + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestValidate(t *testing.T) { + tests := []struct { + name string + params Parameters + wantErr bool + }{ + { + name: "valid parameters", + params: Parameters{ + PeersLimit: 5, + AdvertiseInterval: time.Hour, + AdvertiseRetryTimeout: time.Second, + DiscoveryRetryTimeout: time.Minute, + }, + wantErr: false, + }, + { + name: "negative PeersLimit", + params: Parameters{ + PeersLimit: 0, + AdvertiseInterval: time.Hour, + AdvertiseRetryTimeout: time.Second, + DiscoveryRetryTimeout: time.Minute, + }, + wantErr: true, + }, + { + name: "negative AdvertiseInterval", + params: Parameters{ + PeersLimit: 5, + AdvertiseInterval: -time.Hour, + AdvertiseRetryTimeout: time.Second, + DiscoveryRetryTimeout: time.Minute, + }, + wantErr: true, + }, + { + name: "negative AdvertiseRetryTimeout", + params: Parameters{ + PeersLimit: 5, + AdvertiseInterval: time.Hour, + AdvertiseRetryTimeout: -time.Second, + DiscoveryRetryTimeout: time.Minute, + }, + wantErr: true, + }, + { + name: "negative DiscoveryRetryTimeout", + params: Parameters{ + PeersLimit: 5, + AdvertiseInterval: time.Hour, + AdvertiseRetryTimeout: time.Second, + DiscoveryRetryTimeout: -time.Minute, + }, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.params.Validate() + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/share/p2p/peers/manager_test.go b/share/p2p/peers/manager_test.go index 524acf33b6..ece4e70cab 100644 --- a/share/p2p/peers/manager_test.go +++ b/share/p2p/peers/manager_test.go @@ -395,11 +395,9 @@ func TestIntegration(t *testing.T) { bnRouter, err := dht.New(ctx, bnHost, opts...) require.NoError(t, err) - // hack - discovery.DiscoveryRetryTimeout = time.Millisecond * 100 - params := discovery.DefaultParameters() params.AdvertiseInterval = time.Second + params.DiscoveryRetryTimeout = time.Millisecond * 100 bnDisc, err := discovery.NewDiscovery( params, @@ -433,7 +431,7 @@ func TestIntegration(t *testing.T) { } // set up discovery for full node with hook to peer manager and check discovered peer - params = discovery.DefaultParameters() + params = discovery.TestParameters() params.AdvertiseInterval = time.Second params.PeersLimit = 10