Repository: buchgr/bazel-remote
Pull request #373 (2020-10-28 23:45:16)

[disk] return an error from disk.New instead of exiting
This makes test failures have nicer error messages.


Patch:
diff --git a/cache/disk/disk.go b/cache/disk/disk.go
index a0cb13b..f782560 100644
--- a/cache/disk/disk.go
+++ b/cache/disk/disk.go
@@ -65,7 +65,7 @@ const emptySha256 = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b785
 // New returns a new instance of a filesystem-based cache rooted at `dir`,
 // with a maximum size of `maxSizeBytes` bytes and an optional backend `proxy`.
 // Cache is safe for concurrent use.
-func New(dir string, maxSizeBytes int64, proxy cache.Proxy) *Cache {
+func New(dir string, maxSizeBytes int64, proxy cache.Proxy) (*Cache, error) {
 	// Create the directory structure.
 	hexLetters := []byte("0123456789abcdef")
 	for _, c1 := range hexLetters {
@@ -73,15 +73,15 @@ func New(dir string, maxSizeBytes int64, proxy cache.Proxy) *Cache {
 			subDir := string(c1) + string(c2)
 			err := os.MkdirAll(filepath.Join(dir, cache.CAS.String(), subDir), os.ModePerm)
 			if err != nil {
-				log.Fatal(err)
+				return nil, err
 			}
 			err = os.MkdirAll(filepath.Join(dir, cache.AC.String(), subDir), os.ModePerm)
 			if err != nil {
-				log.Fatal(err)
+				return nil, err
 			}
 			err = os.MkdirAll(filepath.Join(dir, cache.RAW.String(), subDir), os.ModePerm)
 			if err != nil {
-				log.Fatal(err)
+				return nil, err
 			}
 		}
 	}
@@ -105,15 +105,14 @@ func New(dir string, maxSizeBytes int64, proxy cache.Proxy) *Cache {
 
 	err := c.migrateDirectories()
 	if err != nil {
-		log.Fatalf("Attempting to migrate the old directory structure to the new structure failed "+
-			"with error: %v", err)
+		return nil, fmt.Errorf("Attempting to migrate the old directory structure failed: %w", err)
 	}
 	err = c.loadExistingFiles()
 	if err != nil {
-		log.Fatalf("Loading of existing cache entries failed due to error: %v", err)
+		return nil, fmt.Errorf("Loading of existing cache entries failed due to error: %w", err)
 	}
 
-	return c
+	return c, nil
 }
 
 func (c *Cache) migrateDirectories() error {
diff --git a/main.go b/main.go
index e97a4ed..3e01952 100644
--- a/main.go
+++ b/main.go
@@ -357,7 +357,10 @@ func main() {
 			proxyCache = s3proxy.New(c.S3CloudStorage, accessLogger, errorLogger, c.NumUploaders, c.MaxQueuedUploads)
 		}
 
-		diskCache := disk.New(c.Dir, int64(c.MaxSize)*1024*1024*1024, proxyCache)
+		diskCache, err := disk.New(c.Dir, int64(c.MaxSize)*1024*1024*1024, proxyCache)
+		if err != nil {
+			log.Fatal(err)
+		}
 
 		var tlsConfig *tls.Config
 		if len(c.TLSCaFile) != 0 {


Test patch:
diff --git a/cache/disk/disk_test.go b/cache/disk/disk_test.go
index 42b2029..ffe6592 100644
--- a/cache/disk/disk_test.go
+++ b/cache/disk/disk_test.go
@@ -70,9 +70,12 @@ const contentsLength = int64(len(contents))
 func TestCacheBasics(t *testing.T) {
 	cacheDir := tempDir(t)
 	defer os.RemoveAll(cacheDir)
-	testCache := New(cacheDir, 100, nil)
+	testCache, err := New(cacheDir, 100, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
 
-	err := checkItems(testCache, 0, 0)
+	err = checkItems(testCache, 0, 0)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -115,7 +118,10 @@ func TestCacheBasics(t *testing.T) {
 func TestCacheEviction(t *testing.T) {
 	cacheDir := tempDir(t)
 	defer os.RemoveAll(cacheDir)
-	testCache := New(cacheDir, 10, nil)
+	testCache, err := New(cacheDir, 10, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
 
 	expectedSizesNumItems := []struct {
 		expSize int64
@@ -157,13 +163,14 @@ func TestCacheEviction(t *testing.T) {
 func TestCachePutWrongSize(t *testing.T) {
 	cacheDir := tempDir(t)
 	defer os.RemoveAll(cacheDir)
-	testCache := New(cacheDir, 100, nil)
+	testCache, err := New(cacheDir, 100, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
 
 	content := "hello"
 	hash := hashStr(content)
 
-	var err error
-
 	err = testCache.Put(cache.AC, hash, int64(len(content)), strings.NewReader(content))
 	if err != nil {
 		t.Fatal("Expected success", err)
@@ -183,12 +190,15 @@ func TestCacheGetContainsWrongSize(t *testing.T) {
 
 	cacheDir := tempDir(t)
 	defer os.RemoveAll(cacheDir)
-	testCache := New(cacheDir, 100, nil)
+	testCache, err := New(cacheDir, 100, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
 
 	var found bool
 	var rdr io.ReadCloser
 
-	err := testCache.Put(cache.CAS, contentsHash, contentsLength, strings.NewReader(contents))
+	err = testCache.Put(cache.CAS, contentsHash, contentsLength, strings.NewReader(contents))
 	if err != nil {
 		t.Fatal("Expected success", err)
 	}
@@ -218,7 +228,10 @@ func TestCacheGetContainsWrongSizeWithProxy(t *testing.T) {
 
 	cacheDir := tempDir(t)
 	defer os.RemoveAll(cacheDir)
-	testCache := New(cacheDir, 100, new(proxyStub))
+	testCache, err := New(cacheDir, 100, new(proxyStub))
+	if err != nil {
+		t.Fatal(err)
+	}
 
 	var found bool
 	var rdr io.ReadCloser
@@ -324,9 +337,11 @@ func hashStr(content string) string {
 func TestOverwrite(t *testing.T) {
 	cacheDir := tempDir(t)
 	defer os.RemoveAll(cacheDir)
-	testCache := New(cacheDir, 10, nil)
+	testCache, err := New(cacheDir, 10, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
 
-	var err error
 	err = putGetCompare(cache.CAS, hashStr("hello"), "hello", testCache)
 	if err != nil {
 		t.Fatal(err)
@@ -381,9 +396,12 @@ func TestCacheExistingFiles(t *testing.T) {
 	}
 
 	const expectedSize = 4 * int64(len(contents))
-	testCache := New(cacheDir, expectedSize, nil)
+	testCache, err := New(cacheDir, expectedSize, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
 
-	err := checkItems(testCache, expectedSize, 4)
+	err = checkItems(testCache, expectedSize, 4)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -409,7 +427,10 @@ func TestCacheExistingFiles(t *testing.T) {
 func TestCacheBlobTooLarge(t *testing.T) {
 	cacheDir := tempDir(t)
 	defer os.RemoveAll(cacheDir)
-	testCache := New(cacheDir, 100, nil)
+	testCache, err := New(cacheDir, 100, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
 
 	for k := range []cache.EntryKind{cache.AC, cache.RAW} {
 		kind := cache.EntryKind(k)
@@ -432,9 +453,12 @@ func TestCacheBlobTooLarge(t *testing.T) {
 func TestCacheCorruptedCASBlob(t *testing.T) {
 	cacheDir := tempDir(t)
 	defer os.RemoveAll(cacheDir)
-	testCache := New(cacheDir, 1000, nil)
+	testCache, err := New(cacheDir, 1000, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
 
-	err := testCache.Put(cache.CAS, hashStr("foo"), int64(len(contents)),
+	err = testCache.Put(cache.CAS, hashStr("foo"), int64(len(contents)),
 		strings.NewReader(contents))
 	if err == nil {
 		t.Fatal("expected hash mismatch error")
@@ -474,7 +498,11 @@ func TestMigrateFromOldDirectoryStructure(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	testCache := New(cacheDir, 2560, nil)
+	testCache, err := New(cacheDir, 2560, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
 	_, _, numItems := testCache.Stats()
 	if numItems != 3 {
 		t.Fatalf("Expected test cache size 3 but was %d", numItems)
@@ -518,7 +546,11 @@ func TestLoadExistingEntries(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	testCache := New(cacheDir, blobSize*numBlobs, nil)
+	testCache, err := New(cacheDir, blobSize*numBlobs, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
 	_, _, numItems := testCache.Stats()
 	if int64(numItems) != numBlobs {
 		t.Fatalf("Expected test cache size %d but was %d",
@@ -550,7 +582,10 @@ func TestDistinctKeyspaces(t *testing.T) {
 	blobSize := 1024
 	cacheSize := int64(blobSize * 3)
 
-	testCache := New(cacheDir, cacheSize, nil)
+	testCache, err := New(cacheDir, cacheSize, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
 
 	blob, casHash := testutils.RandomDataAndHash(1024)
 
@@ -558,8 +593,6 @@ func TestDistinctKeyspaces(t *testing.T) {
 	// keyspaces, and verify that we have exactly three items in
 	// the cache.
 
-	var err error
-
 	err = putGetCompareBytes(cache.CAS, casHash, blob, testCache)
 	if err != nil {
 		t.Fatal(err)
@@ -665,7 +698,10 @@ func TestHttpProxyBackend(t *testing.T) {
 
 	cacheSize := int64(1024 * 10)
 
-	testCache := New(cacheDir, cacheSize, proxy)
+	testCache, err := New(cacheDir, cacheSize, proxy)
+	if err != nil {
+		t.Fatal(err)
+	}
 
 	blobSize := int64(1024)
 	blob, casHash := testutils.RandomDataAndHash(blobSize)
@@ -700,7 +736,10 @@ func TestHttpProxyBackend(t *testing.T) {
 	// Create a new (empty) testCache, without a proxy backend.
 	cacheDir = testutils.TempDir(t)
 	defer os.RemoveAll(cacheDir)
-	testCache = New(cacheDir, cacheSize, nil)
+	testCache, err = New(cacheDir, cacheSize, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
 
 	// Confirm that it does not contain the item we added to the
 	// first testCache and the proxy backend.
@@ -759,14 +798,15 @@ func TestGetValidatedActionResult(t *testing.T) {
 	cacheDir := testutils.TempDir(t)
 	defer os.RemoveAll(cacheDir)
 
-	testCache := New(cacheDir, 1024*32, nil)
+	testCache, err := New(cacheDir, 1024*32, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
 
 	// Create a directory tree like so:
 	// /bar/foo.txt
 	// /bar/grok.txt
 
-	var err error
-
 	grokData := []byte("grok test data")
 	grokHash := sha256.Sum256(grokData)
 	grokHashStr := hex.EncodeToString(grokHash[:])
diff --git a/cache/httpproxy/httpproxy_test.go b/cache/httpproxy/httpproxy_test.go
index ca85a9f..18ae7a7 100644
--- a/cache/httpproxy/httpproxy_test.go
+++ b/cache/httpproxy/httpproxy_test.go
@@ -103,7 +103,10 @@ func TestEverything(t *testing.T) {
 
 	proxyCache := New(url, &http.Client{}, accessLogger, errorLogger, 100, 10000)
 	diskCacheSize := int64(len(casData) + 1024)
-	diskCache := disk.New(cacheDir, diskCacheSize, proxyCache)
+	diskCache, err := disk.New(cacheDir, diskCacheSize, proxyCache)
+	if err != nil {
+		t.Fatal(err)
+	}
 
 	// PUT two different values with the same key in ac and cas.
 
@@ -226,7 +229,10 @@ func TestEverything(t *testing.T) {
 	cacheDir2 := testutils.TempDir(t)
 	defer os.RemoveAll(cacheDir2)
 
-	diskCache = disk.New(cacheDir2, diskCacheSize, proxyCache)
+	diskCache, err = disk.New(cacheDir2, diskCacheSize, proxyCache)
+	if err != nil {
+		t.Fatal(err)
+	}
 
 	_, _, numItems := diskCache.Stats()
 	if numItems != 0 {
diff --git a/server/grpc_test.go b/server/grpc_test.go
index 2804869..85acb78 100644
--- a/server/grpc_test.go
+++ b/server/grpc_test.go
@@ -69,7 +69,11 @@ func TestMain(m *testing.M) {
 	}
 	defer os.RemoveAll(dir)
 
-	diskCache = disk.New(dir, int64(10*maxChunkSize), nil)
+	diskCache, err = disk.New(dir, int64(10*maxChunkSize), nil)
+	if err != nil {
+		fmt.Println("Test setup failed")
+		os.Exit(1)
+	}
 
 	accessLogger := testutils.NewSilentLogger()
 	errorLogger := testutils.NewSilentLogger()
diff --git a/server/http_test.go b/server/http_test.go
index f4384ae..64db420 100644
--- a/server/http_test.go
+++ b/server/http_test.go
@@ -35,7 +35,10 @@ func TestDownloadFile(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	c := disk.New(cacheDir, blobSize, nil)
+	c, err := disk.New(cacheDir, blobSize, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
 	h := NewHTTPCache(c, testutils.NewSilentLogger(), testutils.NewSilentLogger(), true, false, "")
 
 	req, err := http.NewRequest("GET", "/cas/"+hash, bytes.NewReader([]byte{}))
@@ -98,7 +101,10 @@ func TestUploadFilesConcurrently(t *testing.T) {
 		requests[i] = r
 	}
 
-	c := disk.New(cacheDir, 1000*1024, nil)
+	c, err := disk.New(cacheDir, 1000*1024, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
 	h := NewHTTPCache(c, testutils.NewSilentLogger(), testutils.NewSilentLogger(), true, false, "")
 	handler := http.HandlerFunc(h.CacheHandler)
 
@@ -156,7 +162,10 @@ func TestUploadSameFileConcurrently(t *testing.T) {
 
 	numWorkers := 100
 
-	c := disk.New(cacheDir, int64(len(data)*numWorkers), nil)
+	c, err := disk.New(cacheDir, int64(len(data)*numWorkers), nil)
+	if err != nil {
+		t.Fatal(err)
+	}
 	h := NewHTTPCache(c, testutils.NewSilentLogger(), testutils.NewSilentLogger(), true, false, "")
 	handler := http.HandlerFunc(h.CacheHandler)
 
@@ -202,7 +211,10 @@ func TestUploadCorruptedFile(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	c := disk.New(cacheDir, 2048, nil)
+	c, err := disk.New(cacheDir, 2048, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
 	h := NewHTTPCache(c, testutils.NewSilentLogger(), testutils.NewSilentLogger(), true, false, "")
 	rr := httptest.NewRecorder()
 	handler := http.HandlerFunc(h.CacheHandler)
@@ -242,7 +254,10 @@ func TestUploadEmptyActionResult(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	c := disk.New(cacheDir, 2048, nil)
+	c, err := disk.New(cacheDir, 2048, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
 	validate := true
 	mangle := false
 	h := NewHTTPCache(c, testutils.NewSilentLogger(), testutils.NewSilentLogger(), validate, mangle, "")
@@ -299,7 +314,10 @@ func testEmptyBlobAvailable(t *testing.T, method string) {
 		t.Fatal(err)
 	}
 
-	c := disk.New(cacheDir, 2048, nil)
+	c, err := disk.New(cacheDir, 2048, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
 	validate := true
 	mangle := false
 	h := NewHTTPCache(c, testutils.NewSilentLogger(), testutils.NewSilentLogger(), validate, mangle, "")
@@ -324,7 +342,10 @@ func TestStatusPage(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	c := disk.New(cacheDir, 2048, nil)
+	c, err := disk.New(cacheDir, 2048, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
 	h := NewHTTPCache(c, testutils.NewSilentLogger(), testutils.NewSilentLogger(), true, false, "")
 	rr := httptest.NewRecorder()
 	handler := http.HandlerFunc(h.StatusPageHandler)
@@ -464,7 +485,10 @@ func TestRemoteReturnsNotFound(t *testing.T) {
 		t.Fatal(err)
 	}
 	defer os.RemoveAll(cacheDir)
-	emptyCache := disk.New(cacheDir, 1024, nil)
+	emptyCache, err := disk.New(cacheDir, 1024, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
 
 	h := NewHTTPCache(emptyCache, testutils.NewSilentLogger(), testutils.NewSilentLogger(), true, false, "")
 	// create a fake http.Request