Repository: google/go-flow-levee
Pull request #255 (2021-01-07 20:19:48)

Avoid collision of cache config.
* A collision can occur if tests are run using multiple configurations
within a single package.  This commit attaches source file to config
caching to avoid this issue.

-----

While iterating on #103 and adding tests for format of reporting with or without `ReportMessage` configured, I ran into an issue involving the config cache persisting across multiple tests within a single package.  This might not be the case in all environments, but I hit it on the command-line and in IntelliJ.

This PR separates cached config by the filename of the config's source to avoid this collision.

-----

- [x] Running against a large codebase such as [Kubernetes](https://github.com/kubernetes/kubernetes) does not error out. (See [DEVELOPING.md](https://github.com/google/go-flow-levee/blob/master/DEVELOPING.md) for instructions on how to do that.)
- [n/a] Appropriate changes to README are included in PR


mlevesquedion: Could you expand on how you discovered this, specifically what tests were failing and how? The issue seems to be that multiple tests run in the same process and therefore only the "first" config is loaded because of the cache. This is making me wonder why we haven't run into this issue before, since e.g. `levee_test.go` has two test functions that use different configs. What is different about your new tests that caused them to fail?
PurelyApplied: > Could you expand on how you discovered this, specifically what tests were failing and how? 

Double whoops.  I meant to say working on PR #204, which would address issue #103.  I've since pushed the addition of the tests that uncovered this issue.


> This is making me wonder why we haven't run into this issue before, since e.g. `levee_test.go` has two test functions that use different configs. What is different about your new tests that caused them to fail?

I think we _have_ been hitting this, but quietly.  `levee_test.go` is the only place where we have the same package invoke `Flags.Set` twice on different files.  Test order seems to be sequential as written in file.  That means `TestLeveeDoesNotCreateReportsForPanicIfPanicingOnTaintedValuesIsAllowed` is using the wrong config.

The `nopanic` doesn't have any positive assertion of `// want` in its test file, so the fact that the wrong configuration is loaded allows a false pass.  This can be observed either by removing the 
```golang
	if err := Analyzer.Flags.Set("config", dataDir+"/allowpanicontaintedvalues-config.yaml"); err != nil {
		t.Error(err)
	}
```
entirely and observing that the test passes when run at the package level.  Alternatively, set appropriate breakpoints and observe that the `config` refers to the same address.
mlevesquedion: Good investigation, thanks for explaining.

I've opened a fix for the broken tests here: #257.
vinayakankugoyal: Like @mlevesquedion mentioned this won't really work for parallel tests. If we will need to run the tests sequentially then won't it be easier to just convert `var readFileOnce sync.Once` to `var *readFileOnce sync.Once` and reset it in the tests where we need to update the config files?
PurelyApplied: > Like @mlevesquedion mentioned this won't really work for parallel tests. If we will need to run the tests sequentially then won't it be easier to just convert `var readFileOnce sync.Once` to `var *readFileOnce sync.Once` and reset it in the tests where we need to update the config files?

I don't see how that avoids our race condition.  We'd still have

* A sets config file
* B resets config file
* A reads (reset) config file

I agree this isn't a perfect solution.  I'm really just trying to unblock other work for the moment.  If we need to start running tests in parallel, we can ~~burn~~cross that bridge when we get to it.

Patch:
diff --git a/internal/pkg/config/config.go b/internal/pkg/config/config.go
index 791c3cf..94b395d 100644
--- a/internal/pkg/config/config.go
+++ b/internal/pkg/config/config.go
@@ -285,27 +285,62 @@ func (fm funcMatcher) MatchFunction(path, receiver, name string) bool {
 	return fm.Package.MatchString(path) && fm.Receiver.MatchString(receiver) && fm.Method.MatchString(name)
 }
 
-var readFileOnce sync.Once
-var readConfigCached *Config
-var readConfigCachedErr error
-
-// ReadConfig reads, parses, and validates config file.
+// ReadConfig fetches configuration from the config cache.
+// The cache reads, parses, and validates config file if necessary.
 func ReadConfig() (*Config, error) {
-	readFileOnce.Do(func() {
+	return cache.read(configFile)
+}
+
+// configCacheElement reduces disk access across multiple ReadConfig calls.
+type configCacheElement struct {
+	once       sync.Once
+	conf       *Config
+	err        error
+	sourceFile string
+}
+
+func (r *configCacheElement) readOnce() (*Config, error) {
+	r.once.Do(func() {
 		c := new(Config)
-		bytes, err := ioutil.ReadFile(configFile)
+		bytes, err := ioutil.ReadFile(r.sourceFile)
 		if err != nil {
-			readConfigCachedErr = fmt.Errorf("error reading analysis config: %v", err)
+			r.err = fmt.Errorf("error reading analysis config: %v", err)
 			return
 		}
 
 		if err := yaml.UnmarshalStrict(bytes, c); err != nil {
-			readConfigCachedErr = err
+			r.err = err
 			return
 		}
-		readConfigCached = c
+		r.conf = c
 	})
-	return readConfigCached, readConfigCachedErr
+
+	return r.conf, r.err
+}
+
+// configCache safely stores a configCacheElement per source file for concurrent access.
+type configCache struct {
+	mu    sync.Mutex
+	cache map[string]*configCacheElement
+}
+
+// Instantiates configCacheElement if file has not yet been loaded
+func (w *configCache) read(file string) (*Config, error) {
+	return w.getCacheForFile(file).readOnce()
+}
+
+func (w *configCache) getCacheForFile(file string) *configCacheElement {
+	w.mu.Lock()
+	defer w.mu.Unlock()
+
+	if _, ok := w.cache[file]; !ok {
+		w.cache[file] = &configCacheElement{sourceFile: file}
+	}
+	return w.cache[file]
+}
+
+var cache = configCache{
+	cache: make(map[string]*configCacheElement),
 }
 
 func validateFieldNames(bytes *[]byte, matcherType string, validFields []string) error {


Test patch:
diff --git a/internal/pkg/config/config_test.go b/internal/pkg/config/config_test.go
index cd7dee9..0d25027 100644
--- a/internal/pkg/config/config_test.go
+++ b/internal/pkg/config/config_test.go
@@ -61,6 +61,31 @@ func runTest(pass *analysis.Pass) (interface{}, error) {
 	return nil, nil
 }
 
+func TestConfigCache(t *testing.T) {
+	testdata := analysistest.TestData()
+	if err := FlagSet.Set("config", filepath.Join(testdata, "empty-config.yaml")); err != nil {
+		t.Fatal(err)
+	}
+
+	emptyConfig, err := ReadConfig()
+	if err != nil {
+		t.Errorf("failed to read config: %v", err)
+	}
+
+	if err := FlagSet.Set("config", filepath.Join(testdata, "test-config.yaml")); err != nil {
+		t.Fatal(err)
+	}
+
+	testConfig, err := ReadConfig()
+	if err != nil {
+		t.Errorf("failed to read config: %v", err)
+	}
+
+	if testConfig == emptyConfig {
+		t.Error("Returned the same config for separate config source files")
+	}
+}
+
 func TestConfig(t *testing.T) {
 	testdata := analysistest.TestData()
 	if err := FlagSet.Set("config", filepath.Join(testdata, "test-config.yaml")); err != nil {
diff --git a/internal/pkg/config/testdata/empty-config.yaml b/internal/pkg/config/testdata/empty-config.yaml
new file mode 100644
index 0000000..2b2fba0
--- /dev/null
+++ b/internal/pkg/config/testdata/empty-config.yaml
@@ -0,0 +1,16 @@
+# Copyright 2020 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
+#
+# https://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.
+---
+
+# This config left intentionally empty.