diff --git a/internal/resolver/resolver.go b/internal/resolver/resolver.go index b6f45da9e..f5c0fa801 100644 --- a/internal/resolver/resolver.go +++ b/internal/resolver/resolver.go @@ -122,7 +122,7 @@ func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string var ok bool found := true if !registry.IsOCI(d.Repository) { - repoIndex, err := repo.LoadIndexFile(filepath.Join(r.cachepath, helmpath.CacheIndexFile(repoName))) + repoIndex, err := repo.LoadIndexFileWithCaching(filepath.Join(r.cachepath, helmpath.CacheIndexFile(repoName))) if err != nil { return nil, errors.Wrapf(err, "no cached repository for %s found. (try 'helm repo update')", repoName) } diff --git a/pkg/downloader/chart_downloader.go b/pkg/downloader/chart_downloader.go index dde6a1057..f6dbbba56 100644 --- a/pkg/downloader/chart_downloader.go +++ b/pkg/downloader/chart_downloader.go @@ -281,7 +281,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')") } @@ -380,7 +380,7 @@ func (c *ChartDownloader) scanReposForURL(u string, rf *repo.File) (*repo.Entry, } idxFile := filepath.Join(c.RepositoryCache, helmpath.CacheIndexFile(r.Config.Name)) - i, err := repo.LoadIndexFile(idxFile) + i, err := repo.LoadIndexFileWithCaching(idxFile) if err != nil { return nil, errors.Wrap(err, "no cached repo found. (try 'helm repo update')") } diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go index ec4056d27..8ec6e865c 100644 --- a/pkg/downloader/manager.go +++ b/pkg/downloader/manager.go @@ -826,7 +826,7 @@ func (m *Manager) loadChartRepositories() (map[string]*repo.ChartRepository, err for _, re := range rf.Repositories { lname := re.Name idxFile := filepath.Join(m.RepositoryCache, helmpath.CacheIndexFile(lname)) - index, err := repo.LoadIndexFile(idxFile) + index, err := repo.LoadIndexFileWithCaching(idxFile) if err != nil { return indices, err } diff --git a/pkg/repo/chartrepo.go b/pkg/repo/chartrepo.go index 970e96da2..f63538965 100644 --- a/pkg/repo/chartrepo.go +++ b/pkg/repo/chartrepo.go @@ -99,7 +99,7 @@ func (r *ChartRepository) Load() error { filepath.Walk(r.Config.Name, func(path string, f os.FileInfo, _ 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 } @@ -254,7 +254,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 } diff --git a/pkg/repo/chartrepo_test.go b/pkg/repo/chartrepo_test.go index 0a658c0c2..5a178a689 100644 --- a/pkg/repo/chartrepo_test.go +++ b/pkg/repo/chartrepo_test.go @@ -92,7 +92,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) @@ -104,7 +104,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) } diff --git a/pkg/repo/index.go b/pkg/repo/index.go index e1ce3c62d..2acd62cfb 100644 --- a/pkg/repo/index.go +++ b/pkg/repo/index.go @@ -25,6 +25,7 @@ import ( "path/filepath" "sort" "strings" + "sync" "time" "github.com/Masterminds/semver/v3" @@ -115,6 +116,40 @@ func LoadIndexFile(path string) (*IndexFile, error) { return i, nil } +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) { + // 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 // This can leave the index in an unsorted state func (i IndexFile) MustAdd(md *chart.Metadata, filename, baseURL, digest string) error { diff --git a/pkg/repo/index_test.go b/pkg/repo/index_test.go index a1e3e9438..d5af7a236 100644 --- a/pkg/repo/index_test.go +++ b/pkg/repo/index_test.go @@ -28,6 +28,8 @@ 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" @@ -171,6 +173,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 {