singleflight go commands (gomods#1877)

* singleflight go commands * Apply suggestions from code review Co-authored-by: Brendan Le Glaunec <[email protected]> --------- Co-authored-by: michael-wozniak <[email protected]> Co-authored-by: Brendan Le Glaunec <[email protected]>
chriscoffee · Dec 21, 2023 · 3af0d00 · 3af0d00
1 parent 640cc67
commit 3af0d00
Show file tree

Hide file tree

Showing 2 changed files with 113 additions and 86 deletions.
diff --git a/pkg/module/go_get_fetcher.go b/pkg/module/go_get_fetcher.go
@@ -14,13 +14,15 @@ import (
 	"github.com/gomods/athens/pkg/observ"
 	"github.com/gomods/athens/pkg/storage"
 	"github.com/spf13/afero"
+	"golang.org/x/sync/singleflight"
 )
 
 type goGetFetcher struct {
 	fs           afero.Fs
 	goBinaryName string
 	envVars      []string
 	gogetDir     string
+	sfg          *singleflight.Group
 }
 
 type goModule struct {
@@ -46,6 +48,7 @@ func NewGoGetFetcher(goBinaryName, gogetDir string, envVars []string, fs afero.F
 		goBinaryName: goBinaryName,
 		envVars:      envVars,
 		gogetDir:     gogetDir,
+		sfg:          &singleflight.Group{},
 	}, nil
 }
 
@@ -56,57 +59,63 @@ func (g *goGetFetcher) Fetch(ctx context.Context, mod, ver string) (*storage.Ver
 	ctx, span := observ.StartSpan(ctx, op.String())
 	defer span.End()
 
-	// setup the GOPATH
-	goPathRoot, err := afero.TempDir(g.fs, g.gogetDir, "athens")
-	if err != nil {
-		return nil, errors.E(op, err)
-	}
-	sourcePath := filepath.Join(goPathRoot, "src")
-	modPath := filepath.Join(sourcePath, getRepoDirName(mod, ver))
-	if err := g.fs.MkdirAll(modPath, os.ModeDir|os.ModePerm); err != nil {
-		_ = clearFiles(g.fs, goPathRoot)
-		return nil, errors.E(op, err)
-	}
+	resp, err, _ := g.sfg.Do(mod+"###"+ver, func() (any, error) {
+		// setup the GOPATH
+		goPathRoot, err := afero.TempDir(g.fs, g.gogetDir, "athens")
+		if err != nil {
+			return nil, errors.E(op, err)
+		}
+		sourcePath := filepath.Join(goPathRoot, "src")
+		modPath := filepath.Join(sourcePath, getRepoDirName(mod, ver))
+		if err := g.fs.MkdirAll(modPath, os.ModeDir|os.ModePerm); err != nil {
+			_ = clearFiles(g.fs, goPathRoot)
+			return nil, errors.E(op, err)
+		}
 
-	m, err := downloadModule(
-		ctx,
-		g.goBinaryName,
-		g.envVars,
-		goPathRoot,
-		modPath,
-		mod,
-		ver,
-	)
-	if err != nil {
-		_ = clearFiles(g.fs, goPathRoot)
-		return nil, errors.E(op, err)
-	}
+		m, err := downloadModule(
+			ctx,
+			g.goBinaryName,
+			g.envVars,
+			goPathRoot,
+			modPath,
+			mod,
+			ver,
+		)
+		if err != nil {
+			_ = clearFiles(g.fs, goPathRoot)
+			return nil, errors.E(op, err)
+		}
 
-	var storageVer storage.Version
-	storageVer.Semver = m.Version
-	info, err := afero.ReadFile(g.fs, m.Info)
-	if err != nil {
-		return nil, errors.E(op, err)
-	}
-	storageVer.Info = info
+		var storageVer storage.Version
+		storageVer.Semver = m.Version
+		info, err := afero.ReadFile(g.fs, m.Info)
+		if err != nil {
+			return nil, errors.E(op, err)
+		}
+		storageVer.Info = info
 
-	gomod, err := afero.ReadFile(g.fs, m.GoMod)
-	if err != nil {
-		return nil, errors.E(op, err)
-	}
-	storageVer.Mod = gomod
+		gomod, err := afero.ReadFile(g.fs, m.GoMod)
+		if err != nil {
+			return nil, errors.E(op, err)
+		}
+		storageVer.Mod = gomod
 
-	zip, err := g.fs.Open(m.Zip)
+		zip, err := g.fs.Open(m.Zip)
+		if err != nil {
+			return nil, errors.E(op, err)
+		}
+		// note: don't close zip here so that the caller can read directly from disk.
+		//
+		// if we close, then the caller will panic, and the alternative to make this work is
+		// that we read into memory and return an io.ReadCloser that reads out of memory
+		storageVer.Zip = &zipReadCloser{zip, g.fs, goPathRoot}
+
+		return &storageVer, nil
+	})
 	if err != nil {
-		return nil, errors.E(op, err)
+		return nil, err
 	}
-	// note: don't close zip here so that the caller can read directly from disk.
-	//
-	// if we close, then the caller will panic, and the alternative to make this work is
-	// that we read into memory and return an io.ReadCloser that reads out of memory
-	storageVer.Zip = &zipReadCloser{zip, g.fs, goPathRoot}
-
-	return &storageVer, nil
+	return resp.(*storage.Version), nil
 }
 
 // given a filesystem, gopath, repository root, module and version, runs 'go mod download -json'

diff --git a/pkg/module/go_vcs_lister.go b/pkg/module/go_vcs_lister.go
@@ -13,6 +13,7 @@ import (
 	"github.com/gomods/athens/pkg/observ"
 	"github.com/gomods/athens/pkg/storage"
 	"github.com/spf13/afero"
+	"golang.org/x/sync/singleflight"
 )
 
 type listResp struct {
@@ -26,6 +27,7 @@ type vcsLister struct {
 	goBinPath string
 	env       []string
 	fs        afero.Fs
+	sfg       *singleflight.Group
 }
 
 // NewVCSLister creates an UpstreamLister which uses VCS to fetch a list of available versions.
@@ -34,58 +36,74 @@ func NewVCSLister(goBinPath string, env []string, fs afero.Fs) UpstreamLister {
 		goBinPath: goBinPath,
 		env:       env,
 		fs:        fs,
+		sfg:       &singleflight.Group{},
 	}
 }
 
+type listSFResp struct {
+	rev      *storage.RevInfo
+	versions []string
+}
+
 func (l *vcsLister) List(ctx context.Context, module string) (*storage.RevInfo, []string, error) {
 	const op errors.Op = "vcsLister.List"
 	_, span := observ.StartSpan(ctx, op.String())
 	defer span.End()
-	tmpDir, err := afero.TempDir(l.fs, "", "go-list")
-	if err != nil {
-		return nil, nil, errors.E(op, err)
-	}
-	defer func() { _ = l.fs.RemoveAll(tmpDir) }()
+	sfResp, err, _ := l.sfg.Do(module, func() (any, error) {
+		tmpDir, err := afero.TempDir(l.fs, "", "go-list")
+		if err != nil {
+			return nil, errors.E(op, err)
+		}
+		defer func() { _ = l.fs.RemoveAll(tmpDir) }()
 
-	cmd := exec.Command(
-		l.goBinPath,
-		"list", "-m", "-versions", "-json",
-		config.FmtModVer(module, "latest"),
-	)
-	cmd.Dir = tmpDir
-	stdout := &bytes.Buffer{}
-	stderr := &bytes.Buffer{}
-	cmd.Stdout = stdout
-	cmd.Stderr = stderr
+		cmd := exec.Command(
+			l.goBinPath,
+			"list", "-m", "-versions", "-json",
+			config.FmtModVer(module, "latest"),
+		)
+		cmd.Dir = tmpDir
+		stdout := &bytes.Buffer{}
+		stderr := &bytes.Buffer{}
+		cmd.Stdout = stdout
+		cmd.Stderr = stderr
 
-	gopath, err := afero.TempDir(l.fs, "", "athens")
-	if err != nil {
-		return nil, nil, errors.E(op, err)
-	}
-	defer func() { _ = clearFiles(l.fs, gopath) }()
-	cmd.Env = prepareEnv(gopath, l.env)
+		gopath, err := afero.TempDir(l.fs, "", "athens")
+		if err != nil {
+			return nil, errors.E(op, err)
+		}
+		defer func() { _ = clearFiles(l.fs, gopath) }()
+		cmd.Env = prepareEnv(gopath, l.env)
 
-	err = cmd.Run()
-	if err != nil {
-		err = fmt.Errorf("%w: %s", err, stderr)
-		// as of now, we can't recognize between a true NotFound
-		// and an unexpected error, so we choose the more
-		// hopeful path of NotFound. This way the Go command
-		// will not log en error and we still get to log
-		// what happened here if someone wants to dig in more.
-		// Once, https://github.com/golang/go/issues/30134 is
-		// resolved, we can hopefully differentiate.
-		return nil, nil, errors.E(op, err, errors.KindNotFound)
-	}
+		err = cmd.Run()
+		if err != nil {
+			err = fmt.Errorf("%w: %s", err, stderr)
+			// as of now, we can't recognize between a true NotFound
+			// and an unexpected error, so we choose the more
+			// hopeful path of NotFound. This way the Go command
+			// will not log en error and we still get to log
+			// what happened here if someone wants to dig in more.
+			// Once, https://github.com/golang/go/issues/30134 is
+			// resolved, we can hopefully differentiate.
+			return nil, errors.E(op, err, errors.KindNotFound)
+		}
 
-	var lr listResp
-	err = json.NewDecoder(stdout).Decode(&lr)
+		var lr listResp
+		err = json.NewDecoder(stdout).Decode(&lr)
+		if err != nil {
+			return nil, errors.E(op, err)
+		}
+		rev := storage.RevInfo{
+			Time:    lr.Time,
+			Version: lr.Version,
+		}
+		return listSFResp{
+			rev:      &rev,
+			versions: lr.Versions,
+		}, nil
+	})
 	if err != nil {
-		return nil, nil, errors.E(op, err)
-	}
-	rev := storage.RevInfo{
-		Time:    lr.Time,
-		Version: lr.Version,
+		return nil, nil, err
 	}
-	return &rev, lr.Versions, nil
+	ret := sfResp.(listSFResp)
+	return ret.rev, ret.versions, nil
 }