From c573bb43eb6ba6302d1ce9fe3baf49902b1a729d Mon Sep 17 00:00:00 2001 From: michael-wozniak Date: Fri, 21 Jul 2023 06:31:19 -0700 Subject: [PATCH 1/2] singleflight go commands --- pkg/module/go_get_fetcher.go | 99 ++++++++++++++++++---------------- pkg/module/go_vcs_lister.go | 100 +++++++++++++++++++++-------------- 2 files changed, 113 insertions(+), 86 deletions(-) diff --git a/pkg/module/go_get_fetcher.go b/pkg/module/go_get_fetcher.go index e43d3529d..3e14746b3 100644 --- a/pkg/module/go_get_fetcher.go +++ b/pkg/module/go_get_fetcher.go @@ -14,6 +14,7 @@ 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 { @@ -21,6 +22,7 @@ type goGetFetcher struct { 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: new(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 index a6c1c611a..f3b1b9753 100644 --- 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: new(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 } From 3eb1ee61c0323e1ae366ce7a5a2782f71aa3cf33 Mon Sep 17 00:00:00 2001 From: wozz Date: Wed, 20 Dec 2023 10:40:08 -0500 Subject: [PATCH 2/2] Apply suggestions from code review Co-authored-by: Brendan Le Glaunec --- pkg/module/go_get_fetcher.go | 2 +- pkg/module/go_vcs_lister.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/module/go_get_fetcher.go b/pkg/module/go_get_fetcher.go index 3e14746b3..c9e59d292 100644 --- a/pkg/module/go_get_fetcher.go +++ b/pkg/module/go_get_fetcher.go @@ -48,7 +48,7 @@ func NewGoGetFetcher(goBinaryName, gogetDir string, envVars []string, fs afero.F goBinaryName: goBinaryName, envVars: envVars, gogetDir: gogetDir, - sfg: new(singleflight.Group), + sfg: &singleflight.Group{}, }, nil } diff --git a/pkg/module/go_vcs_lister.go b/pkg/module/go_vcs_lister.go index f3b1b9753..86abcc38c 100644 --- a/pkg/module/go_vcs_lister.go +++ b/pkg/module/go_vcs_lister.go @@ -36,7 +36,7 @@ func NewVCSLister(goBinPath string, env []string, fs afero.Fs) UpstreamLister { goBinPath: goBinPath, env: env, fs: fs, - sfg: new(singleflight.Group), + sfg: &singleflight.Group{}, } }