use singleflight and sync.Map

pkg/skaffold/docker/dependencies.go

@@ -22,22 +22,22 @@ import (
 	"os"
 	"path/filepath"
 	"sort"
+	"sync"
 
 	"github.com/docker/docker/builder/dockerignore"
 	"github.com/golang/groupcache/singleflight"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/walk"
 )
 
-// dependency represents computed dependency for a dockerfile.
-type dependency struct {
-	files []string
-	err   error
-}
-
-// sfDependencyCache ensures  `GetDependencies` is called only once for  individual dockerfile.
 var (
-	sfDependencyCache = singleflight.Group{}
+	// sfGetDependencies ensures `GetDependencies` is called only once at any time
+	// for a given dockerfile.
+	// sfGetDependencies along with sync.Map will ensure no two concurrent processes read or
+	// write dependencies for a given dockerfile.
+	sfGetDependencies = singleflight.Group{}
+
+	dependencyCache = sync.Map{}
 )
 
 // NormalizeDockerfilePath returns the absolute path to the dockerfile.
@@ -62,35 +62,40 @@ func GetDependencies(ctx context.Context, workspace string, dockerfilePath strin
 		return nil, fmt.Errorf("normalizing dockerfile path: %w", err)
 	}
 
-	if paths, err := sfDependencyCache.Do(absDockerfilePath, func() (interface{}, error) {
-		paths, err := getDependencies(workspace, dockerfilePath, absDockerfilePath, buildArgs, cfg)
-		return paths, err
-	}); err != nil {
+	deps, _ := sfGetDependencies.Do(absDockerfilePath, func() (interface{}, error) {
+		if dep, ok := dependencyCache.Load(absDockerfilePath); ok {
+			return dep, nil
+		}
+		dep := getDependencies(workspace, dockerfilePath, absDockerfilePath, buildArgs, cfg)
+		dependencyCache.Store(absDockerfilePath, dep)
+		return dep, nil
+	})
+
+	if paths, ok := deps.([]string); ok {
+		return paths, nil
+	} else if err, ok := deps.(error); ok {
 		return nil, err
-	} else if s, ok := paths.([]string); !ok {
-		return nil, fmt.Errorf("unexpected skaffold internal error encountered converting dependencies to []string")
-	} else {
-		return s, nil
 	}
+	return nil, fmt.Errorf("unexpected skaffold internal error encountered converting dependencies to []string")
 }
 
-func getDependencies(workspace string, dockerfilePath string, absDockerfilePath string, buildArgs map[string]*string, cfg Config) ([]string, error) {
+func getDependencies(workspace string, dockerfilePath string, absDockerfilePath string, buildArgs map[string]*string, cfg Config) interface{} {
 	// If the Dockerfile doesn't exist, we can't compute the dependency.
 	// But since we know the Dockerfile is a dependency, let's return a list
 	// with only that file. It makes errors down the line more actionable
 	// than returning an error now.
 	if _, err := os.Stat(absDockerfilePath); os.IsNotExist(err) {
-		return []string{dockerfilePath}, nil
+		return []string{dockerfilePath}
 	}
 
 	fts, err := readCopyCmdsFromDockerfile(false, absDockerfilePath, workspace, buildArgs, cfg)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	excludes, err := readDockerignore(workspace, absDockerfilePath)
 	if err != nil {
-		return nil, fmt.Errorf("reading .dockerignore: %w", err)
+		return fmt.Errorf("reading .dockerignore: %w", err)
 	}
 
 	deps := make([]string, 0, len(fts))
@@ -100,7 +105,7 @@ func getDependencies(workspace string, dockerfilePath string, absDockerfilePath
 
 	files, err := WalkWorkspace(workspace, excludes, deps)
 	if err != nil {
-		return nil, fmt.Errorf("walking workspace: %w", err)
+		return fmt.Errorf("walking workspace: %w", err)
 	}
 
 	// Always add dockerfile even if it's .dockerignored. The daemon will need it anyways.
@@ -119,7 +124,7 @@ func getDependencies(workspace string, dockerfilePath string, absDockerfilePath
 	}
 	sort.Strings(dependencies)
 
-	return dependencies, nil
+	return dependencies
 }
 
 // readDockerignore reads patterns to ignore

pkg/skaffold/docker/dependencies_test.go

@@ -660,29 +660,37 @@ func TestGetDependenciesCached(t *testing.T) {
 	tests := []struct {
 		description     string
 		retrieveImgMock func(_ string, _ Config) (*v1.ConfigFile, error)
-		dependencyCache map[string]dependency
+		dependencyCache map[string]interface{}
 		expected        []string
 		shouldErr       bool
 	}{
 		{
-			description:     "with no cached results, getDependencies will retrieve image",
+			description:     "with no cached results getDependencies will retrieve image",
 			retrieveImgMock: imageFetcher.fetch,
-			dependencyCache: map[string]dependency{},
+			dependencyCache: map[string]interface{}{},
 			expected:        []string{"Dockerfile", "server.go"},
 		},
 		{
-			description: "with cached results, getDependencies should read from cache",
+			description: "with cached results getDependencies should read from cache",
 			retrieveImgMock: func(_ string, _ Config) (*v1.ConfigFile, error) {
 				return nil, fmt.Errorf("unexpected call")
 			},
-			dependencyCache: map[string]dependency{"Dockerfile": {[]string{"random.go"}, nil}},
+			dependencyCache: map[string]interface{}{"Dockerfile": []string{"random.go"}},
 			expected:        []string{"random.go"},
 		},
+		{
+			description: "with cached results is error getDependencies should read from cache",
+			retrieveImgMock: func(_ string, _ Config) (*v1.ConfigFile, error) {
+				return &v1.ConfigFile{}, nil
+			},
+			dependencyCache: map[string]interface{}{"Dockerfile": fmt.Errorf("remote manifest fetch")},
+			shouldErr:       true,
+		},
 		{
 			description:     "with cached results for dockerfile in another app",
 			retrieveImgMock: imageFetcher.fetch,
-			dependencyCache: map[string]dependency{
-				filepath.Join("app", "Dockerfile"): {[]string{"random.go"}, nil}},
+			dependencyCache: map[string]interface{}{
+				filepath.Join("app", "Dockerfile"): []string{"random.go"}},
 			expected: []string{"Dockerfile", "server.go"},
 		},
 	}
@@ -701,9 +709,6 @@ func TestGetDependenciesCached(t *testing.T) {
 			}
 			deps, err := GetDependencies(context.Background(), tmpDir.Root(), "Dockerfile", map[string]*string{}, nil)
 			t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expected, deps)
-			if _, ok := dependencyCache.Load(tmpDir.Path("Dockerfile")); !ok {
-				t.Fatal("expected a cache entry for Dockerfile, did not found")
-			}
 		})
 	}
 }