tune implementation / add test-case

Signed-off-by: Dirk Moermans <dirk.moermans@telekom.de>

pkg/downloader/chart_downloader.go

@@ -239,7 +239,7 @@ func (c *ChartDownloader) ResolveChartVersion(ref, version string) (*url.URL, er
 
 	// Next, we need to load the index, and actually look up the chart.
 	idxFile := filepath.Join(c.RepositoryCache, helmpath.CacheIndexFile(r.Config.Name))
-	i, err := repo.LoadIndexFile(idxFile)
+	i, err := repo.LoadIndexFileWithCaching(idxFile)
 	if err != nil {
 		return u, errors.Wrap(err, "no cached repo found. (try 'helm repo update')")
 	}

pkg/repo/chartrepo.go

@@ -100,7 +100,7 @@ func (r *ChartRepository) Load() error {
 	filepath.Walk(r.Config.Name, func(path string, f os.FileInfo, err error) error {
 		if !f.IsDir() {
 			if strings.Contains(f.Name(), "-index.yaml") {
-				i, err := LoadIndexFile(path)
+				i, err := LoadIndexFileWithCaching(path)
 				if err != nil {
 					return err
 				}
@@ -255,7 +255,7 @@ func FindChartInAuthAndTLSAndPassRepoURL(repoURL, username, password, chartName,
 	}
 
 	// Read the index file for the repository to get chart information and return chart URL
-	repoIndex, err := LoadIndexFile(idx)
+	repoIndex, err := LoadIndexFileWithCaching(idx)
 	if err != nil {
 		return "", err
 	}

pkg/repo/chartrepo_test.go

@@ -93,7 +93,7 @@ func TestIndex(t *testing.T) {
 	}
 
 	tempIndexPath := filepath.Join(testRepository, indexPath)
-	actual, err := LoadIndexFile(tempIndexPath)
+	actual, err := LoadIndexFileWithCaching(tempIndexPath)
 	defer os.Remove(tempIndexPath) // clean up
 	if err != nil {
 		t.Errorf("Error loading index file %v", err)
@@ -105,7 +105,7 @@ func TestIndex(t *testing.T) {
 	if err != nil {
 		t.Errorf("Error performing re-index: %s\n", err)
 	}
-	second, err := LoadIndexFile(tempIndexPath)
+	second, err := LoadIndexFileWithCaching(tempIndexPath)
 	if err != nil {
 		t.Errorf("Error re-loading index file %v", err)
 	}

pkg/repo/index.go

@@ -116,28 +116,38 @@ func LoadIndexFile(path string) (*IndexFile, error) {
 	return i, nil
 }
 
-var cache = make(map[string]*IndexFile)
-var cacheLock sync.RWMutex
+type cacheEntry struct {
+	indexFile *IndexFile
+	err       error
+	lock      sync.RWMutex
+}
+
+var cache sync.Map
 
+// LoadIndexFileWithCaching is a wrapper around LoadIndexFile
+// it adds an internal global cache to avoid reading the same file multiple times
 func LoadIndexFileWithCaching(path string) (*IndexFile, error) {
-	// if already in cache, return cached entry
-	cacheLock.RLock()
-	if idx, ok := cache[path]; ok {
-		cacheLock.RUnlock()
-		// safe to return a pointer to the cached entry here since once in cache
-		// entry will never be overwritten
-		return idx, nil
-	}
-	cacheLock.RUnlock()
-	// not in cache, need to load from disk
-	idx, err := LoadIndexFile(path)
-	if err == nil {
-		// we fetched the index successfully. Store it in cache.
-		cacheLock.Lock()
-		cache[path] = idx
-		cacheLock.Unlock()
-	}
-	return idx, err
+	// assume the entry is not cached, prepare a newEntry
+	newEntry := &cacheEntry{}
+	// lock to avoid threading issues in case of multiple loads in parallel
+	newEntry.lock.Lock()
+
+	// check if index already in cache, add newEntry atomically if not-cached
+	if v, loaded := cache.LoadOrStore(path, newEntry); loaded {
+		// We hit the cache. newEntry is not needed anymore
+		newEntry.lock.Unlock()
+		entry := v.(*cacheEntry)
+		// wait for RLock on cached entry
+		// LoadIndexFile might not have completed yet in another go-routine
+		entry.lock.RLock()
+		defer entry.lock.RUnlock()
+		return entry.indexFile, entry.err
+	}
+	// no entry found in cache
+	// LoadIndexFile while holding the write-lock
+	newEntry.indexFile, newEntry.err = LoadIndexFile(path)
+	defer newEntry.lock.Unlock()
+	return newEntry.indexFile, newEntry.err
 }
 
 // MustAdd adds a file to the index

pkg/repo/index_test.go

@@ -27,6 +27,7 @@ import (
 	"strings"
 	"testing"
 
+	"golang.org/x/sync/errgroup"
 	"helm.sh/helm/v3/pkg/chart"
 	"helm.sh/helm/v3/pkg/cli"
 	"helm.sh/helm/v3/pkg/getter"
@@ -145,6 +146,45 @@ func TestLoadIndex(t *testing.T) {
 	}
 }
 
+func TestLoadIndexWithCaching(t *testing.T) {
+	var eg errgroup.Group
+
+	const nrParallelTests = 10
+
+	var indexFiles [nrParallelTests]*IndexFile
+
+	// Load same index multiple times in parallel
+	for i := 0; i < nrParallelTests; i++ {
+		indexNr := i
+		eg.Go(func() (err error) {
+			indexFiles[indexNr], err = LoadIndexFileWithCaching(chartmuseumtestfile)
+			return err
+		})
+	}
+
+	if err := eg.Wait(); err != nil {
+		t.Fatal(err)
+	}
+
+	// we check if the same cache entry is used by comparing pointers to indexFiles
+	// if the pointers are the same, we assume the same cache entry is used.
+	for i := 1; i < nrParallelTests; i++ {
+		if indexFiles[i] != indexFiles[0] {
+			t.Fatal("not all indices retrieved from same cache entry")
+		}
+	}
+
+	// load another index, and check if new_entry is used
+	otherFile, err := LoadIndexFileWithCaching(testfile)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if otherFile == indexFiles[0] {
+		t.Fatal("same index used for different files")
+	}
+}
+
 // TestLoadIndex_Duplicates is a regression to make sure that we don't non-deterministically allow duplicate packages.
 func TestLoadIndex_Duplicates(t *testing.T) {
 	if _, err := loadIndex([]byte(indexWithDuplicates), "indexWithDuplicates"); err == nil {