Repository: buchgr/bazel-remote
Pull request #129 (2019-11-28 12:46:02)

fix stats race condition
If we lock the cache twice to get two pieces of data, the cache might
change in the middle. We should instead just obtain one lock, get both
data items we want and then unlock.

(This has conflicts with #126, one of the two will need to be rebased on the other.)


Patch:
diff --git a/cache/cache.go b/cache/cache.go
index 01558fa..c3ccae8 100644
--- a/cache/cache.go
+++ b/cache/cache.go
@@ -69,11 +69,9 @@ type Cache interface {
 	// MaxSize returns the maximum cache size in bytes.
 	MaxSize() int64
 
-	// CurrentSize returns the current cache size in bytes.
-	CurrentSize() int64
-
-	// NumItems returns the number of items stored in the cache.
-	NumItems() int
+	// Return the current size of the cache in bytes, and the number of
+	// items stored in the cache.
+	Stats() (int64, int)
 }
 
 // If `hash` refers to a valid ActionResult with all the dependencies
diff --git a/cache/disk/disk.go b/cache/disk/disk.go
index 5abc9a9..559a065 100644
--- a/cache/disk/disk.go
+++ b/cache/disk/disk.go
@@ -290,21 +290,16 @@ func (c *diskCache) Contains(kind cache.EntryKind, hash string) (ok bool) {
 	return found && val.(*lruItem).committed
 }
 
-func (c *diskCache) NumItems() int {
-	c.mu.Lock()
-	defer c.mu.Unlock()
-	return c.lru.Len()
-}
-
 func (c *diskCache) MaxSize() int64 {
 	// The underlying value is never modified, no need to lock.
 	return c.lru.MaxSize()
 }
 
-func (c *diskCache) CurrentSize() int64 {
+func (c *diskCache) Stats() (currentSize int64, numItems int) {
 	c.mu.Lock()
 	defer c.mu.Unlock()
-	return c.lru.CurrentSize()
+
+	return c.lru.CurrentSize(), c.lru.Len()
 }
 
 func ensureDirExists(path string) {
diff --git a/cache/http/http.go b/cache/http/http.go
index 33f3ed5..34864fe 100644
--- a/cache/http/http.go
+++ b/cache/http/http.go
@@ -165,12 +165,8 @@ func (r *remoteHTTPProxyCache) MaxSize() int64 {
 	return r.local.MaxSize()
 }
 
-func (r *remoteHTTPProxyCache) CurrentSize() int64 {
-	return r.local.CurrentSize()
-}
-
-func (r *remoteHTTPProxyCache) NumItems() int {
-	return r.local.NumItems()
+func (r *remoteHTTPProxyCache) Stats() (currentSize int64, numItems int) {
+	return r.local.Stats()
 }
 
 func requestURL(baseURL *url.URL, hash string, kind cache.EntryKind) string {
diff --git a/cache/s3/s3.go b/cache/s3/s3.go
index 238a143..23acca0 100644
--- a/cache/s3/s3.go
+++ b/cache/s3/s3.go
@@ -161,10 +161,6 @@ func (c *s3Cache) MaxSize() int64 {
 	return c.local.MaxSize()
 }
 
-func (c *s3Cache) CurrentSize() int64 {
-	return c.local.CurrentSize()
-}
-
-func (c *s3Cache) NumItems() int {
-	return c.local.NumItems()
+func (c *s3Cache) Stats() (currentSize int64, numItems int) {
+	return c.local.Stats()
 }
diff --git a/server/http.go b/server/http.go
index 5e61dc4..64d6feb 100644
--- a/server/http.go
+++ b/server/http.go
@@ -47,7 +47,10 @@ type statusPageData struct {
 // errorLogger will print unexpected server errors. Inexistent files and malformed URLs will not
 // be reported.
 func NewHTTPCache(cache cache.Cache, accessLogger cache.Logger, errorLogger cache.Logger, validateAC bool, commit string) HTTPCache {
-	errorLogger.Printf("Loaded %d existing disk cache items.", cache.NumItems())
+
+	_, numItems := cache.Stats()
+
+	errorLogger.Printf("Loaded %d existing disk cache items.", numItems)
 
 	hc := &httpCache{
 		cache:        cache,
@@ -308,13 +311,15 @@ func addWorkerMetadataHTTP(addr string, orig []byte) (data []byte, code int, err
 func (h *httpCache) StatusPageHandler(w http.ResponseWriter, r *http.Request) {
 	defer r.Body.Close()
 
+	currentSize, numItems := h.cache.Stats()
+
 	w.Header().Set("Content-Type", "application/json")
 	enc := json.NewEncoder(w)
 	enc.SetIndent("", " ")
 	enc.Encode(statusPageData{
-		CurrSize:   h.cache.CurrentSize(),
 		MaxSize:    h.cache.MaxSize(),
-		NumFiles:   h.cache.NumItems(),
+		CurrSize:   currentSize,
+		NumFiles:   numItems,
 		ServerTime: time.Now().Unix(),
 		GitCommit:  h.gitCommit,
 	})


Test patch:
diff --git a/cache/disk/disk_test.go b/cache/disk/disk_test.go
index 96ac903..d854c0d 100644
--- a/cache/disk/disk_test.go
+++ b/cache/disk/disk_test.go
@@ -340,8 +340,9 @@ func TestMigrateFromOldDirectoryStructure(t *testing.T) {
 		t.Fatal(err)
 	}
 	testCache := New(cacheDir, 2560)
-	if testCache.NumItems() != 3 {
-		t.Fatalf("Expected test cache size 3 but was %d", testCache.NumItems())
+	_, numItems := testCache.Stats()
+	if numItems != 3 {
+		t.Fatalf("Expected test cache size 3 but was %d", numItems)
 	}
 	if !testCache.Contains(cache.AC, acHash) {
 		t.Fatalf("Expected cache to contain AC entry '%s'", acHash)
@@ -376,9 +377,10 @@ func TestLoadExistingEntries(t *testing.T) {
 	}
 
 	testCache := New(cacheDir, blobSize*numBlobs)
-	if int64(testCache.NumItems()) != numBlobs {
+	_, numItems := testCache.Stats()
+	if int64(numItems) != numBlobs {
 		t.Fatalf("Expected test cache size %d but was %d",
-			numBlobs, testCache.NumItems())
+			numBlobs, numItems)
 	}
 	if !testCache.Contains(cache.AC, acHash) {
 		t.Fatalf("Expected cache to contain AC entry '%s'", acHash)
@@ -423,8 +425,9 @@ func TestDistinctKeyspaces(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	if testCache.NumItems() != 3 {
+	_, numItems := testCache.Stats()
+	if numItems != 3 {
 		t.Fatalf("Expected test cache size 3 but was %d",
-			testCache.NumItems())
+			numItems)
 	}
 }
diff --git a/server/http_test.go b/server/http_test.go
index 9d72e80..a57105f 100644
--- a/server/http_test.go
+++ b/server/http_test.go
@@ -385,11 +385,8 @@ func (f *fakeCache) MaxSize() int64 {
 	return 0
 }
 
-func (f *fakeCache) CurrentSize() int64 {
-	return 0
-}
-func (f *fakeCache) NumItems() int {
-	return 0
+func (f *fakeCache) Stats() (int64, int) {
+	return 0, 0
 }
 
 type fakeResponseWriter struct {