Remove cap for number of reconnections per hour (#120)

* Remove cap for number of reconnections per hour

* Change max backoff interval

* Add tests

* Re-enable tests

Author: cristinaleonr

connection/connection.go

@@ -17,9 +17,6 @@ import (
 )
 
 var (
-	// ErrTooManyReconnects is returned when the number of reconnections
-	// has reached MaxReconnectionsTotal within MaxElapsedTime.
-	ErrTooManyReconnects = errors.New("websocket cannot reconnect right now (too many attemps)")
 	// ErrNotDialed is returned when WriteMessage is called, but
 	// the websocket has not been created yet (call Dial).
 	ErrNotDailed = errors.New("websocket not created yet, please call Dial()")
@@ -48,36 +45,25 @@ type Conn struct {
 	// MaxElapsedTime is the amount of time after which the ExponentialBackOff
 	// returns Stop. It never stops if MaxElapsedTime == 0.
 	MaxElapsedTime time.Duration
-	// MaxReconnectionsTotal is the maximum number of reconnections that can
-	// happen within MaxReconnectionsTime.
-	MaxReconnectionsTotal int
-	// MaxReconnectionsTime is the time period during which the number of
-	// reconnections must be less than MaxReconnectionsTotal to allow a
-	// reconnection atttempt.
-	MaxReconnectionsTime time.Duration
-	dialer               websocket.Dialer
-	ws                   *websocket.Conn
-	url                  url.URL
-	header               http.Header
-	dialMessage          interface{}
-	ticker               time.Ticker
-	mu                   sync.Mutex
-	reconnections        int
-	isDialed             bool
-	isConnected          bool
-	stop                 chan bool
+	dialer         websocket.Dialer
+	ws             *websocket.Conn
+	url            url.URL
+	header         http.Header
+	dialMessage    interface{}
+	ticker         time.Ticker
+	mu             sync.Mutex
+	isDialed       bool
+	isConnected    bool
 }
 
 // NewConn creates a new Conn with default values.
 func NewConn() *Conn {
 	c := &Conn{
-		InitialInterval:       static.BackoffInitialInterval,
-		RandomizationFactor:   static.BackoffRandomizationFactor,
-		Multiplier:            static.BackoffMultiplier,
-		MaxInterval:           static.BackoffMaxInterval,
-		MaxElapsedTime:        static.BackoffMaxElapsedTime,
-		MaxReconnectionsTotal: static.MaxReconnectionsTotal,
-		MaxReconnectionsTime:  static.MaxReconnectionsTime,
+		InitialInterval:     static.BackoffInitialInterval,
+		RandomizationFactor: static.BackoffRandomizationFactor,
+		Multiplier:          static.BackoffMultiplier,
+		MaxInterval:         static.BackoffMaxInterval,
+		MaxElapsedTime:      static.BackoffMaxElapsedTime,
 	}
 	return c
 }
@@ -101,23 +87,9 @@ func (c *Conn) Dial(address string, header http.Header, dialMsg interface{}) err
 	}
 	c.url = *u
 	c.dialMessage = dialMsg
-	c.stop = make(chan bool)
 	c.header = header
 	c.dialer = websocket.Dialer{}
 	c.isDialed = true
-	c.ticker = *time.NewTicker(c.MaxReconnectionsTime)
-	go func(c *Conn) {
-		for {
-			select {
-			case <-c.stop:
-				c.ticker.Stop()
-				return
-			case <-c.ticker.C:
-				c.resetReconnections()
-			}
-		}
-	}(c)
-
 	return c.connect()
 }
 
@@ -129,8 +101,7 @@ func (c *Conn) Dial(address string, header http.Header, dialMsg interface{}) err
 // The write will fail under the following conditions:
 //  1. The client has not called Dial (ErrNotDialed).
 //  2. The connection is disconnected and it was not able to
-//     reconnect (ErrTooManyReconnects or an internal connection
-//     error).
+//     reconnect.
 //  3. The write call in the websocket package failed
 //     (gorilla/websocket error).
 func (c *Conn) WriteMessage(messageType int, data interface{}) error {
@@ -160,39 +131,22 @@ func (c *Conn) IsConnected() bool {
 	return c.isConnected
 }
 
-// CanConnect checks whether it is possible to reconnect
-// given the recent number of attempts.
-func (c *Conn) CanConnect() bool {
-	c.mu.Lock()
-	defer c.mu.Unlock()
-	return c.reconnections < c.MaxReconnectionsTotal
-}
-
 // Close closes the network connection and cleans up private
 // resources after the connection is done.
 func (c *Conn) Close() error {
 	if c.isDialed {
 		c.isDialed = false
-		c.stop <- true
 	}
 	return c.close()
 }
 
-// resetReconnections sets the number of disconnects followed
-// by reconnects.
-func (c *Conn) resetReconnections() {
-	c.mu.Lock()
-	defer c.mu.Unlock()
-	c.reconnections = 0
-}
-
-// closeAndReconnect calls close and reconnect.
+// closeAndReconnect calls close and reconnects.
 func (c *Conn) closeAndReconnect() error {
 	err := c.close()
 	if err != nil {
 		return err
 	}
-	return c.reconnect()
+	return c.connect()
 }
 
 // close closes the underlying network connection without
@@ -207,18 +161,6 @@ func (c *Conn) close() error {
 	return nil
 }
 
-// reconnect updates the number of reconnections and
-// re-establishes the connection.
-func (c *Conn) reconnect() error {
-	if !c.CanConnect() {
-		return ErrTooManyReconnects
-	}
-	c.mu.Lock()
-	c.reconnections++
-	c.mu.Unlock()
-	return c.connect()
-}
-
 // connect creates a new client connection and sends the
 // registration message.
 // In case of failure, it uses an exponential backoff to

connection/connection_test.go

@@ -9,7 +9,6 @@ import (
 
 	"github.com/gorilla/websocket"
 	"github.com/m-lab/locate/connection/testdata"
-	"github.com/m-lab/locate/static"
 )
 
 func Test_Dial(t *testing.T) {
@@ -172,78 +171,32 @@ func Test_WriteMessage_ErrNotDailed(t *testing.T) {
 	}
 }
 
-func Test_WriteMessage_ErrTooManyReconnects(t *testing.T) {
+func TestWriteMessage_ClosedServer(t *testing.T) {
 	c := NewConn()
-	c.MaxReconnectionsTotal = 0
 	defer c.Close()
 	fh := testdata.FakeHandler{}
 	s := testdata.FakeServer(fh.Upgrade)
-	defer s.Close()
+	c.InitialInterval = 500 * time.Millisecond
+	c.MaxElapsedTime = time.Second
 	c.Dial(s.URL, http.Header{}, testdata.FakeRegistration)
-	// Close connection so writes fail.
-	fh.Close()
-
-	// This should return ErrTooManyReconnects because IsConnected is false
-	// and MaxReconnectionsTotal = 0.
-	err := c.WriteMessage(websocket.TextMessage, []byte("Health message!"))
-	if !errors.Is(err, ErrTooManyReconnects) {
-		t.Errorf("WriteMessage() incorrect error; got: %v, want: ErrTooManyReconnects", err)
-	}
 
-	// Shut server down so reconnection fails.
+	// Shut down server for testing.
+	fh.Close()
 	s.Close()
-	// Allow reconnections again.
-	c.MaxReconnectionsTotal = 1
-	c.MaxElapsedTime = 1 * time.Second
-	// Should still get an error, but it should not be ErrTooManyReconnects.
-	err = c.WriteMessage(websocket.TextMessage, []byte("Health message!"))
-	if err == nil || errors.Is(err, ErrTooManyReconnects) {
-		t.Errorf("WriteMessage() incorrect error; got: %v, want: !ErrTooManyReconnects", err)
-	}
-}
 
-func Test_CloseAndReconnect(t *testing.T) {
-	c := NewConn()
-	fh := testdata.FakeHandler{}
-	s := testdata.FakeServer(fh.Upgrade)
-	defer close(c, s)
-	// For testing, make this time window smaller.
-	c.MaxReconnectionsTime = time.Second
-	c.Dial(s.URL, http.Header{}, testdata.FakeRegistration)
-
-	for i := 0; i < static.MaxReconnectionsTotal; i++ {
-		fh.Close()
-
-		err := c.WriteMessage(websocket.TextMessage, []byte("Health message!"))
-		if err != nil {
-			t.Errorf("WriteMessage() should succeed after reconnection; err: %v", err)
-		}
-
-		if !c.IsConnected() {
-			t.Error("WriteMessage() failed to reconnect")
-		}
-	}
-
-	// It should not reconnect once it reaches the maximum number of attempts.
-	fh.Close()
+	// Write should fail and connection should become disconnected.
 	err := c.WriteMessage(websocket.TextMessage, []byte("Health message!"))
 	if err == nil {
-		t.Error("WriteMessage() should fail while disconnected")
+		t.Error("WriteMessage() should fail after server is disconnected")
 	}
 	if c.IsConnected() {
-		t.Error("WriteMessage() reconnection should fail after MaxReconnectionsTotal reached")
+		t.Errorf("IsConnected() should be false after writing to closed server.")
 	}
 
-	// It should reconnect again after calling WriteMessage once the number of reconnections
-	// is reset.
-	timer := time.NewTimer(2 * c.MaxReconnectionsTime)
-	<-timer.C
+	// Subsequent writes should fail.
 	err = c.WriteMessage(websocket.TextMessage, []byte("Health message!"))
-	if err != nil {
-		t.Errorf("WriteMessage() should succeed after MaxReconnectionsTime; err: %v", err)
-	}
-	if !c.IsConnected() {
-		t.Error("WriteMessage() failed to reconnect after MaxReconnectionsTime")
+	if err == nil {
+		t.Error("WriteMessage() should fail after client detects disconnection")
 	}
 }
 

static/configs.go

@@ -18,10 +18,8 @@ const (
 	BackoffInitialInterval     = time.Second
 	BackoffRandomizationFactor = 0.5
 	BackoffMultiplier          = 2
-	BackoffMaxInterval         = time.Hour
+	BackoffMaxInterval         = 5 * time.Minute
 	BackoffMaxElapsedTime      = 0
-	MaxReconnectionsTotal      = 10
-	MaxReconnectionsTime       = time.Hour
 	HeartbeatPeriod            = 10 * time.Second
 	MemorystoreExportPeriod    = 10 * time.Second
 	PrometheusCheckPeriod      = time.Minute