Add Cacher, which invalidates a cache when config changes (#931)

I will use this in the evaluator_client, where we want to re-use a client if possible, but get a new client if the used values change. This solves the generic problem on a meta level, instead of having to manually remember and compare the config values used. It also prevents programming errors where a new config value is read, but the code doesn't properly detect if it has changed.

The ForceReset method will be used when the evaluator client has an error, so that the system will recover from a client which is stuck in an error state on the next call.

I anticipate there will be other places to use this inside open match.

.golangci.yaml

@@ -167,20 +167,21 @@ linters-settings:
 linters:
   enable-all: true
   disable:
-    - goimports
-    - stylecheck
-    - gocritic
     - dupl
-    - gocyclo
-    - gosec
-    - lll
-    - staticcheck
-    - scopelint
-    - prealloc
-    - gofmt
-    - interfacer # deprecated - "A tool that suggests interfaces is prone to bad suggestions"
-    - gochecknoglobals
     - funlen
+    - gochecknoglobals
+    - goconst
+    - gocritic
+    - gocyclo
+    - gofmt
+    - goimports
+    - gosec
+    - interfacer # deprecated - "A tool that suggests interfaces is prone to bad suggestions"
+    - lll
+    - prealloc
+    - scopelint
+    - staticcheck
+    - stylecheck
 
 #linters:
 #  enable-all: true

internal/config/cacher.go

@@ -0,0 +1,211 @@
+// Copyright 2019 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package config
+
+import (
+	"sync"
+	"time"
+)
+
+// Cacher is used to cache the construction of an object, such as a connection.
+// It will detect which config values are read when constructing the object.
+// Then, when further requests are made, it will return the same object as long
+// as the config values which were used haven't changed.
+type Cacher struct {
+	cfg         View
+	newInstance func(cfg View) (interface{}, error)
+	m           sync.Mutex
+
+	r *viewChangeDetector
+	v interface{}
+}
+
+// NewCacher returns a cacher which uses cfg to detect relevant changes, and
+// newInstance to construct the object when nessisary.  newInstance MUST use the
+// provided View when constructing the object.
+func NewCacher(cfg View, newInstance func(cfg View) (interface{}, error)) *Cacher {
+	return &Cacher{
+		cfg:         cfg,
+		newInstance: newInstance,
+	}
+}
+
+// Get returns the cached object if possible, otherwise it calls newInstance to
+// construct the new cached object.  When Get is next called, it will detect if
+// any of the configuration values which were used to construct the object have
+// changed. If they have, the cache is invalidated, and a new object is
+// constructed. If newInstance returns an error, Get returns that error and the
+// object will not be cached or returned.
+func (c *Cacher) Get() (interface{}, error) {
+	c.m.Lock()
+	defer c.m.Unlock()
+
+	if c.r == nil || c.r.hasChanges() {
+		c.r = newViewChangeDetector(c.cfg)
+		var err error
+		c.v, err = c.newInstance(c.r)
+		if err != nil {
+			c.r = nil
+			c.v = nil
+			return nil, err
+		}
+	}
+
+	return c.v, nil
+}
+
+// ForceReset causes Cacher to forget the cached object.  The next call to Get
+// will again use newInstance to create a new object.
+func (c *Cacher) ForceReset() {
+	c.m.Lock()
+	defer c.m.Unlock()
+	c.r = nil
+	c.v = nil
+}
+
+// Remember each value as it is read, and can detect if a value has been changed
+// since it was last read.
+type viewChangeDetector struct {
+	cfg            View
+	isSet          map[string]bool
+	getString      map[string]string
+	getInt         map[string]int
+	getInt64       map[string]int64
+	getFloat64     map[string]float64
+	getStringSlice map[string][]string
+	getBool        map[string]bool
+	getDuration    map[string]time.Duration
+}
+
+func newViewChangeDetector(cfg View) *viewChangeDetector {
+	return &viewChangeDetector{
+		cfg:            cfg,
+		isSet:          make(map[string]bool),
+		getString:      make(map[string]string),
+		getInt:         make(map[string]int),
+		getInt64:       make(map[string]int64),
+		getFloat64:     make(map[string]float64),
+		getStringSlice: make(map[string][]string),
+		getBool:        make(map[string]bool),
+		getDuration:    make(map[string]time.Duration),
+	}
+}
+
+func (r *viewChangeDetector) IsSet(k string) bool {
+	v := r.cfg.IsSet(k)
+	r.isSet[k] = v
+	return v
+}
+
+func (r *viewChangeDetector) GetString(k string) string {
+	v := r.cfg.GetString(k)
+	r.getString[k] = v
+	return v
+}
+
+func (r *viewChangeDetector) GetInt(k string) int {
+	v := r.cfg.GetInt(k)
+	r.getInt[k] = v
+	return v
+}
+
+func (r *viewChangeDetector) GetInt64(k string) int64 {
+	v := r.cfg.GetInt64(k)
+	r.getInt64[k] = v
+	return v
+}
+
+func (r *viewChangeDetector) GetFloat64(k string) float64 {
+	v := r.cfg.GetFloat64(k)
+	r.getFloat64[k] = v
+	return v
+}
+
+func (r *viewChangeDetector) GetStringSlice(k string) []string {
+	v := r.cfg.GetStringSlice(k)
+	r.getStringSlice[k] = v
+	return v
+}
+
+func (r *viewChangeDetector) GetBool(k string) bool {
+	v := r.cfg.GetBool(k)
+	r.getBool[k] = v
+	return v
+}
+
+func (r *viewChangeDetector) GetDuration(k string) time.Duration {
+	v := r.cfg.GetDuration(k)
+	r.getDuration[k] = v
+	return v
+}
+
+func (r *viewChangeDetector) hasChanges() bool {
+	for k, v := range r.isSet {
+		if r.cfg.IsSet(k) != v {
+			return true
+		}
+	}
+
+	for k, v := range r.getString {
+		if r.cfg.GetString(k) != v {
+			return true
+		}
+	}
+
+	for k, v := range r.getInt {
+		if r.cfg.GetInt(k) != v {
+			return true
+		}
+	}
+
+	for k, v := range r.getInt64 {
+		if r.cfg.GetInt64(k) != v {
+			return true
+		}
+	}
+
+	for k, v := range r.getFloat64 {
+		if r.cfg.GetFloat64(k) != v {
+			return true
+		}
+	}
+
+	for k, v := range r.getStringSlice {
+		actual := r.cfg.GetStringSlice(k)
+		if len(actual) != len(v) {
+			return true
+		}
+
+		for i := range v {
+			if v[i] != actual[i] {
+				return true
+			}
+		}
+	}
+
+	for k, v := range r.getBool {
+		if r.cfg.GetBool(k) != v {
+			return true
+		}
+	}
+
+	for k, v := range r.getDuration {
+		if r.cfg.GetDuration(k) != v {
+			return true
+		}
+	}
+
+	return false
+}

internal/config/cacher_test.go

@@ -0,0 +1,279 @@
+// Copyright 2019 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package config
+
+import (
+	"fmt"
+	"testing"
+	"time"
+
+	"github.com/spf13/viper"
+)
+
+var getTests = []struct {
+	name           string
+	firstValue     interface{}
+	firstExpected  interface{}
+	secondValue    interface{}
+	secondExpected interface{}
+	getValue       func(cfg View) interface{}
+	verifySame     func(a, b interface{}) bool
+}{
+	{
+		name:           "IsSet",
+		firstValue:     nil,
+		firstExpected:  false,
+		secondValue:    "bar",
+		secondExpected: true,
+		getValue: func(cfg View) interface{} {
+			return cfg.IsSet("foo")
+		},
+	},
+	{
+		name:           "GetString",
+		firstValue:     "bar",
+		firstExpected:  "bar",
+		secondValue:    "baz",
+		secondExpected: "baz",
+		getValue: func(cfg View) interface{} {
+			return cfg.GetString("foo")
+		},
+	},
+	{
+		name:           "GetInt",
+		firstValue:     int(1),
+		firstExpected:  int(1),
+		secondValue:    int(2),
+		secondExpected: int(2),
+		getValue: func(cfg View) interface{} {
+			return cfg.GetInt("foo")
+		},
+	},
+	{
+		name:           "GetInt64",
+		firstValue:     int64(1),
+		firstExpected:  int64(1),
+		secondValue:    int64(2),
+		secondExpected: int64(2),
+		getValue: func(cfg View) interface{} {
+			return cfg.GetInt64("foo")
+		},
+	},
+	{
+		name:           "GetFloat64",
+		firstValue:     float64(1),
+		firstExpected:  float64(1),
+		secondValue:    float64(2),
+		secondExpected: float64(2),
+		getValue: func(cfg View) interface{} {
+			return cfg.GetFloat64("foo")
+		},
+	},
+	{
+		name:           "GetStringSlice",
+		firstValue:     []string{"1", "2", "3"},
+		firstExpected:  "2",
+		secondValue:    []string{"1", "4", "3"},
+		secondExpected: "4",
+		getValue: func(cfg View) interface{} {
+			return cfg.GetStringSlice("foo")[1]
+		},
+	},
+	{
+		name:           "GetStringSliceFirstShorter",
+		firstValue:     []string{"1"},
+		firstExpected:  1,
+		secondValue:    []string{"1", "4", "3"},
+		secondExpected: 3,
+		getValue: func(cfg View) interface{} {
+			return len(cfg.GetStringSlice("foo"))
+		},
+	},
+	{
+		name:           "GetStringSliceSecondShorter",
+		firstValue:     []string{"1", "2"},
+		firstExpected:  2,
+		secondValue:    []string{"1"},
+		secondExpected: 1,
+		getValue: func(cfg View) interface{} {
+			return len(cfg.GetStringSlice("foo"))
+		},
+	},
+	{
+		name:           "GetBool",
+		firstValue:     true,
+		firstExpected:  true,
+		secondValue:    false,
+		secondExpected: false,
+		getValue: func(cfg View) interface{} {
+			return cfg.GetBool("foo")
+		},
+	},
+	{
+		name:           "GetDuration",
+		firstValue:     time.Second,
+		firstExpected:  time.Second,
+		secondValue:    time.Minute,
+		secondExpected: time.Minute,
+		getValue: func(cfg View) interface{} {
+			return cfg.GetDuration("foo")
+		},
+	},
+}
+
+func Test_Get(t *testing.T) {
+	for _, tt := range getTests {
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.verifySame == nil {
+				tt.verifySame = func(a, b interface{}) bool {
+					return a == b
+				}
+			}
+
+			if tt.firstExpected == tt.secondExpected {
+				t.Errorf("Expected that first value and second expected would be not equal")
+			}
+
+			if tt.firstExpected != tt.firstExpected {
+				t.Errorf("Expected that first value would be equal with itself")
+			}
+
+			if tt.secondExpected != tt.secondExpected {
+				t.Errorf("Expected that first value would be equal with itself")
+			}
+
+			cfg := viper.New()
+			calls := 0
+
+			cfg.Set("foo", tt.firstValue)
+			c := NewCacher(cfg, func(cfg View) (interface{}, error) {
+				calls++
+				return tt.getValue(cfg), nil
+			})
+
+			v, err := c.Get()
+			if v != tt.firstExpected {
+				t.Errorf("expected %v, got %v", tt.firstExpected, v)
+			}
+			if calls != 1 {
+				t.Errorf("expected 1 call, got %d", calls)
+			}
+			if err != nil {
+				t.Errorf("expected nil error, got %v", err)
+			}
+
+			cfg.Set("foo", tt.firstValue)
+
+			v, err = c.Get()
+			if v != tt.firstExpected {
+				t.Errorf("expected %v, got %v", tt.firstExpected, v)
+			}
+			if calls != 1 {
+				t.Errorf("expected 1 call, got %d", calls)
+			}
+			if err != nil {
+				t.Errorf("expected nil error, got %v", err)
+			}
+
+			cfg.Set("foo", tt.secondValue)
+
+			v, err = c.Get()
+			if v != tt.secondExpected {
+				t.Errorf("expected %v, got %v", tt.secondExpected, v)
+			}
+			if calls != 2 {
+				t.Errorf("expected 2 calls, got %d", calls)
+			}
+			if err != nil {
+				t.Errorf("expected nil error, got %v", err)
+			}
+		})
+	}
+}
+
+func Test_Get_Error(t *testing.T) {
+	returnError := true
+
+	cfg := viper.New()
+	c := NewCacher(cfg, func(cfg View) (interface{}, error) {
+		// Contrived for tests, in real usage outside values shouldn't be used like this.
+		if returnError {
+			return "foo", fmt.Errorf("bad")
+		}
+		return "foo", nil
+	})
+
+	v, err := c.Get()
+
+	if v != nil {
+		t.Errorf("Expected value to be nil")
+	}
+	if err.Error() != "bad" {
+		t.Errorf("Expected error \"bad\", got %v", err)
+	}
+
+	// No config values changed, still trying.
+	returnError = false // Emulating the environment changing.
+	v, err = c.Get()
+	if v != "foo" {
+		t.Errorf("Expected foo, got %v", v)
+	}
+	if err != nil {
+		t.Errorf("Expected nil, got %v", err)
+	}
+}
+
+func Test_ForceReset(t *testing.T) {
+	returnValue := "foo"
+
+	cfg := viper.New()
+	c := NewCacher(cfg, func(cfg View) (interface{}, error) {
+		// Contrived for tests, in real usage outside values shouldn't be used like this.
+		return returnValue, nil
+	})
+
+	v, err := c.Get()
+	if v != "foo" {
+		t.Errorf("Expected foo, got %v", v)
+	}
+	if err != nil {
+		t.Errorf("Expected nil, got %v", err)
+	}
+
+	// Environment has changed, eg a server connection has broken and needs to be
+	// recreated.  The change is detected with some other means (eg, connection
+	// returning errors), and a ForceReset is required.
+	returnValue = "bar"
+
+	// Sanity check: value doesn't change because config hasn't.
+	v, err = c.Get()
+	if v != "foo" {
+		t.Errorf("Expected foo, got %v", v)
+	}
+	if err != nil {
+		t.Errorf("Expected nil, got %v", err)
+	}
+
+	c.ForceReset()
+
+	// Same thing as above, but ForceReset has been called.
+	v, err = c.Get()
+	if v != "bar" {
+		t.Errorf("Expected bar, got %v", v)
+	}
+	if err != nil {
+		t.Errorf("Expected nil, got %v", err)
+	}
+}

internal/config/view.go

@@ -31,7 +31,6 @@ type View interface {
 	GetStringSlice(string) []string
 	GetBool(string) bool
 	GetDuration(string) time.Duration
-	GetStringMap(string) map[string]interface{}
 }
 
 // Mutable is a read-write view of the Open Match configuration.