From b9134b0aa4abbc40908cef9643ec75129c1eac8f Mon Sep 17 00:00:00 2001 From: Jordan Krage Date: Thu, 17 Aug 2017 08:26:17 -0500 Subject: [PATCH] gps: source cache: adding close() method to singleSourceCache, sourceGateway, and sourceCoordinator --- cmd/dep/godep_importer_test.go | 5 ++++- cmd/dep/govend_importer_test.go | 2 +- context.go | 5 ++++- internal/gps/example.go | 2 +- internal/gps/manager_test.go | 29 ++++++++++++++++++++------ internal/gps/result_test.go | 9 ++++++-- internal/gps/solve_test.go | 25 +++------------------- internal/gps/solver_inputs_test.go | 5 ++++- internal/gps/source.go | 19 +++++++++++++++-- internal/gps/source_cache.go | 5 +++++ internal/gps/source_manager.go | 32 ++++++++++++++++++++--------- internal/gps/source_manager_test.go | 6 +++++- internal/gps/source_test.go | 4 +++- internal/test/writer.go | 31 ++++++++++++++++++++++++++++ 14 files changed, 130 insertions(+), 49 deletions(-) create mode 100644 internal/test/writer.go diff --git a/cmd/dep/godep_importer_test.go b/cmd/dep/godep_importer_test.go index 08c3e5018e..fee86e60db 100644 --- a/cmd/dep/godep_importer_test.go +++ b/cmd/dep/godep_importer_test.go @@ -153,7 +153,10 @@ func TestGodepConfig_Import(t *testing.T) { h.TempCopy(filepath.Join(testProjectRoot, godepPath), "godep/Godeps.json") projectRoot := h.Path(testProjectRoot) - sm, err := gps.NewSourceManager(h.Path(cacheDir)) + sm, err := gps.NewSourceManager(gps.SourceManagerConfig{ + Cachedir: h.Path(cacheDir), + Logger: log.New(test.Writer{t}, "", 0), + }) h.Must(err) defer sm.Release() diff --git a/cmd/dep/govend_importer_test.go b/cmd/dep/govend_importer_test.go index eedb2e113c..6db48221d2 100644 --- a/cmd/dep/govend_importer_test.go +++ b/cmd/dep/govend_importer_test.go @@ -97,7 +97,7 @@ func TestGovendConfig_Import(t *testing.T) { h.TempCopy(filepath.Join(testProjectRoot, govendYAMLName), "govend/vendor.yml") projectRoot := h.Path(testProjectRoot) - sm, err := gps.NewSourceManager(h.Path(cacheDir)) + sm, err := gps.NewSourceManager(gps.SourceManagerConfig{Cachedir: h.Path(cacheDir)}) h.Must(err) defer sm.Release() diff --git a/context.go b/context.go index f149df106b..d9cd7f91fa 100644 --- a/context.go +++ b/context.go @@ -84,7 +84,10 @@ func defaultGOPATH() string { } func (c *Ctx) SourceManager() (*gps.SourceMgr, error) { - return gps.NewSourceManager(filepath.Join(c.GOPATH, "pkg", "dep")) + return gps.NewSourceManager(gps.SourceManagerConfig{ + Cachedir: filepath.Join(c.GOPATH, "pkg", "dep"), + Logger: c.Out, + }) } // LoadProject starts from the current working directory and searches up the diff --git a/internal/gps/example.go b/internal/gps/example.go index e91db30a79..6882d57726 100644 --- a/internal/gps/example.go +++ b/internal/gps/example.go @@ -44,7 +44,7 @@ func main() { // Set up a SourceManager. This manages interaction with sources (repositories). tempdir, _ := ioutil.TempDir("", "gps-repocache") - sourcemgr, _ := gps.NewSourceManager(filepath.Join(tempdir)) + sourcemgr, _ := gps.NewSourceManager(gps.SourceManagerConfig{Cachedir: filepath.Join(tempdir)}) defer sourcemgr.Release() // Prep and run the solver diff --git a/internal/gps/manager_test.go b/internal/gps/manager_test.go index b5c15794a8..8023474e97 100644 --- a/internal/gps/manager_test.go +++ b/internal/gps/manager_test.go @@ -8,6 +8,7 @@ import ( "context" "fmt" "io/ioutil" + "log" "os" "path" "path/filepath" @@ -16,6 +17,8 @@ import ( "sync/atomic" "testing" "time" + + "github.com/golang/dep/internal/test" ) // An analyzer that passes nothing back, but doesn't error. This is the naive @@ -40,7 +43,10 @@ func mkNaiveSM(t *testing.T) (*SourceMgr, func()) { t.Fatalf("Failed to create temp dir: %s", err) } - sm, err := NewSourceManager(cpath) + sm, err := NewSourceManager(SourceManagerConfig{ + Cachedir: cpath, + Logger: log.New(test.Writer{t}, "", 0), + }) if err != nil { t.Fatalf("Unexpected error on SourceManager creation: %s", err) } @@ -58,7 +64,10 @@ func remakeNaiveSM(osm *SourceMgr, t *testing.T) (*SourceMgr, func()) { cpath := osm.cachedir osm.Release() - sm, err := NewSourceManager(cpath) + sm, err := NewSourceManager(SourceManagerConfig{ + Cachedir: cpath, + Logger: log.New(test.Writer{t}, "", 0), + }) if err != nil { t.Fatalf("unexpected error on SourceManager recreation: %s", err) } @@ -77,13 +86,18 @@ func TestSourceManagerInit(t *testing.T) { if err != nil { t.Errorf("Failed to create temp dir: %s", err) } - sm, err := NewSourceManager(cpath) + cfg := SourceManagerConfig{ + Cachedir: cpath, + Logger: log.New(test.Writer{t}, "", 0), + } + + sm, err := NewSourceManager(cfg) if err != nil { t.Errorf("Unexpected error on SourceManager creation: %s", err) } - _, err = NewSourceManager(cpath) + _, err = NewSourceManager(cfg) if err == nil { t.Errorf("Creating second SourceManager should have failed due to file lock contention") } else if te, ok := err.(CouldNotCreateLockError); !ok { @@ -105,7 +119,7 @@ func TestSourceManagerInit(t *testing.T) { } // Set another one up at the same spot now, just to be sure - sm, err = NewSourceManager(cpath) + sm, err = NewSourceManager(cfg) if err != nil { t.Errorf("Creating a second SourceManager should have succeeded when the first was released, but failed with err %s", err) } @@ -128,7 +142,10 @@ func TestSourceInit(t *testing.T) { t.Fatalf("Failed to create temp dir: %s", err) } - sm, err := NewSourceManager(cpath) + sm, err := NewSourceManager(SourceManagerConfig{ + Cachedir: cpath, + Logger: log.New(test.Writer{t}, "", 0), + }) if err != nil { t.Fatalf("Unexpected error on SourceManager creation: %s", err) } diff --git a/internal/gps/result_test.go b/internal/gps/result_test.go index 0385d48270..40b767895b 100644 --- a/internal/gps/result_test.go +++ b/internal/gps/result_test.go @@ -11,6 +11,8 @@ import ( "path" "path/filepath" "testing" + + "github.com/golang/dep/internal/test" ) var discardLogger = log.New(ioutil.Discard, "", 0) @@ -122,9 +124,12 @@ func BenchmarkCreateVendorTree(b *testing.B) { tmp := path.Join(os.TempDir(), "vsolvtest") clean := true - sm, err := NewSourceManager(path.Join(tmp, "cache")) + sm, err := NewSourceManager(SourceManagerConfig{ + Cachedir: path.Join(tmp, "cache"), + Logger: log.New(test.Writer{b}, "", 0), + }) if err != nil { - b.Errorf("NewSourceManager errored unexpectedly: %q", err) + b.Errorf("failed to create SourceManager: %q", err) clean = false } diff --git a/internal/gps/solve_test.go b/internal/gps/solve_test.go index b61343b807..99744e8f1d 100644 --- a/internal/gps/solve_test.go +++ b/internal/gps/solve_test.go @@ -10,9 +10,9 @@ import ( "log" "reflect" "sort" - "strings" "testing" - "unicode" + + "github.com/golang/dep/internal/test" ) // overrideMkBridge overrides the base bridge with the depspecBridge that skips @@ -21,30 +21,11 @@ func overrideMkBridge(s *solver, sm SourceManager, down bool) sourceBridge { return &depspecBridge{mkBridge(s, sm, down)} } -type testlogger struct { - *testing.T -} - -func (t testlogger) Write(b []byte) (n int, err error) { - str := string(b) - if len(str) == 0 { - return 0, nil - } - - for _, part := range strings.Split(str, "\n") { - str := strings.TrimRightFunc(part, unicode.IsSpace) - if len(str) != 0 { - t.T.Log(str) - } - } - return len(b), err -} - func fixSolve(params SolveParameters, sm SourceManager, t *testing.T) (Solution, error) { // Trace unconditionally; by passing the trace through t.Log(), the testing // system will decide whether or not to actually show the output (based on // -v, or selectively on test failure). - params.TraceLogger = log.New(testlogger{T: t}, "", 0) + params.TraceLogger = log.New(test.Writer{t}, "", 0) // always return false, otherwise it would identify pretty much all of // our fixtures as being stdlib and skip everything params.stdLibFn = func(string) bool { return false } diff --git a/internal/gps/solver_inputs_test.go b/internal/gps/solver_inputs_test.go index b6aad11f2a..a3c71adf13 100644 --- a/internal/gps/solver_inputs_test.go +++ b/internal/gps/solver_inputs_test.go @@ -169,7 +169,10 @@ func TestValidateParams(t *testing.T) { cacheDir := "gps-cache" h.TempDir(cacheDir) - sm, err := NewSourceManager(h.Path(cacheDir)) + sm, err := NewSourceManager(SourceManagerConfig{ + Cachedir: h.Path(cacheDir), + Logger: log.New(test.Writer{t}, "", 0), + }) h.Must(err) defer sm.Release() diff --git a/internal/gps/source.go b/internal/gps/source.go index 1ef3e42a6c..f0839d9a43 100644 --- a/internal/gps/source.go +++ b/internal/gps/source.go @@ -6,11 +6,12 @@ package gps import ( "context" - "errors" "fmt" + "log" "sync" "github.com/golang/dep/internal/gps/pkgtree" + "github.com/pkg/errors" ) // sourceState represent the states that a source can be in, depending on how @@ -49,19 +50,29 @@ type sourceCoordinator struct { protoSrcs map[string][]srcReturnChans deducer deducer cachedir string + logger *log.Logger } -func newSourceCoordinator(superv *supervisor, deducer deducer, cachedir string) *sourceCoordinator { +func newSourceCoordinator(superv *supervisor, deducer deducer, cachedir string, logger *log.Logger) *sourceCoordinator { return &sourceCoordinator{ supervisor: superv, deducer: deducer, cachedir: cachedir, + logger: logger, srcs: make(map[string]*sourceGateway), nameToURL: make(map[string]string), protoSrcs: make(map[string][]srcReturnChans), } } +func (sc *sourceCoordinator) close() { + for k, v := range sc.srcs { + if err := v.close(); err != nil { + sc.logger.Println(errors.Wrapf(err, "error closing source gateway for %q", k)) + } + } +} + func (sc *sourceCoordinator) getSourceGatewayFor(ctx context.Context, id ProjectIdentifier) (*sourceGateway, error) { if sc.supervisor.getLifetimeContext().Err() != nil { return nil, errors.New("sourceCoordinator has been terminated") @@ -200,6 +211,10 @@ func newSourceGateway(maybe maybeSource, superv *supervisor, cachedir string) *s return sg } +func (sg *sourceGateway) close() error { + return errors.Wrap(sg.cache.close(), "error closing cache") +} + func (sg *sourceGateway) syncLocal(ctx context.Context) error { sg.mu.Lock() defer sg.mu.Unlock() diff --git a/internal/gps/source_cache.go b/internal/gps/source_cache.go index 00c521b94c..e57e63a0c1 100644 --- a/internal/gps/source_cache.go +++ b/internal/gps/source_cache.go @@ -58,6 +58,9 @@ type singleSourceCache interface { // If the input is a revision and multiple UnpairedVersions are associated // with it, whatever happens to be the first is returned. toUnpaired(v Version) (UnpairedVersion, bool) + + // close closes the cache and any backing resources. + close() error } type singleSourceCacheMemory struct { @@ -77,6 +80,8 @@ func newMemoryCache() singleSourceCache { } } +func (*singleSourceCacheMemory) close() error { return nil } + type projectInfo struct { Manifest Lock diff --git a/internal/gps/source_manager.go b/internal/gps/source_manager.go index eb619dc158..51092d05e4 100644 --- a/internal/gps/source_manager.go +++ b/internal/gps/source_manager.go @@ -7,6 +7,8 @@ package gps import ( "context" "fmt" + "io/ioutil" + "log" "os" "os/signal" "path/filepath" @@ -43,7 +45,6 @@ type SourceManager interface { // ListVersions retrieves a list of the available versions for a given // repository name. - // TODO convert to []PairedVersion ListVersions(ProjectIdentifier) ([]PairedVersion, error) // RevisionPresentIn indicates whether the provided Version is present in @@ -133,9 +134,13 @@ func (smIsReleased) Error() string { var _ SourceManager = &SourceMgr{} -// NewSourceManager produces an instance of gps's built-in SourceManager. It -// takes a cache directory, where local instances of upstream sources are -// stored. +// SourceManagerConfig holds configuration information for creating SourceMgrs. +type SourceManagerConfig struct { + Cachedir string // Where to store local instances of upstream sources. + Logger *log.Logger // Optional info/warn logger. Discards if nil. +} + +// NewSourceManager produces an instance of gps's built-in SourceManager. // // The returned SourceManager aggressively caches information wherever possible. // If tools need to do preliminary work involving upstream repository analysis @@ -146,8 +151,12 @@ var _ SourceManager = &SourceMgr{} // gps's SourceManager is intended to be threadsafe (if it's not, please file a // bug!). It should be safe to reuse across concurrent solving runs, even on // unrelated projects. -func NewSourceManager(cachedir string) (*SourceMgr, error) { - err := os.MkdirAll(filepath.Join(cachedir, "sources"), 0777) +func NewSourceManager(c SourceManagerConfig) (*SourceMgr, error) { + if c.Logger == nil { + c.Logger = log.New(ioutil.Discard, "", 0) + } + + err := os.MkdirAll(filepath.Join(c.Cachedir, "sources"), 0777) if err != nil { return nil, err } @@ -159,7 +168,7 @@ func NewSourceManager(cachedir string) (*SourceMgr, error) { // a process keeping the lock busy, it will pass back a temporary error that // we can spin on. - glpath := filepath.Join(cachedir, "sm.lock") + glpath := filepath.Join(c.Cachedir, "sm.lock") lockfile, err := lockfile.New(glpath) if err != nil { return nil, CouldNotCreateLockError{ @@ -222,12 +231,12 @@ func NewSourceManager(cachedir string) (*SourceMgr, error) { deducer := newDeductionCoordinator(superv) sm := &SourceMgr{ - cachedir: cachedir, + cachedir: c.Cachedir, lf: &lockfile, suprvsr: superv, cancelAll: cf, deduceCoord: deducer, - srcCoord: newSourceCoordinator(superv, deducer, cachedir), + srcCoord: newSourceCoordinator(superv, deducer, c.Cachedir, c.Logger), qch: make(chan struct{}), } @@ -348,7 +357,7 @@ func (sm *SourceMgr) Release() { // until after the doRelease process is done, as doing so could cause the // process to terminate before a signal-driven doRelease() call has a chance // to finish its cleanup. - sm.relonce.Do(func() { sm.doRelease() }) + sm.relonce.Do(sm.doRelease) } // doRelease actually releases physical resources (files on disk, etc.). @@ -360,6 +369,9 @@ func (sm *SourceMgr) doRelease() { sm.cancelAll() sm.suprvsr.wait() + // Close the source coordinator. + sm.srcCoord.close() + // Close the file handle for the lock file and remove it from disk sm.lf.Unlock() os.Remove(filepath.Join(sm.cachedir, "sm.lock")) diff --git a/internal/gps/source_manager_test.go b/internal/gps/source_manager_test.go index 83df379446..75468cd1dc 100644 --- a/internal/gps/source_manager_test.go +++ b/internal/gps/source_manager_test.go @@ -5,6 +5,7 @@ package gps import ( + "log" "reflect" "testing" @@ -87,7 +88,10 @@ func TestSourceManager_InferConstraint(t *testing.T) { cacheDir := "gps-repocache" h.TempDir(cacheDir) - sm, err := NewSourceManager(h.Path(cacheDir)) + sm, err := NewSourceManager(SourceManagerConfig{ + Cachedir: h.Path(cacheDir), + Logger: log.New(test.Writer{t}, "", 0), + }) h.Must(err) got, err := sm.InferConstraint(tc.str, tc.project) diff --git a/internal/gps/source_test.go b/internal/gps/source_test.go index b5679761a8..cf05cda2cd 100644 --- a/internal/gps/source_test.go +++ b/internal/gps/source_test.go @@ -8,10 +8,12 @@ import ( "context" "fmt" "io/ioutil" + "log" "reflect" "testing" "github.com/golang/dep/internal/gps/pkgtree" + "github.com/golang/dep/internal/test" ) // Executed in parallel by TestSlowVcs @@ -37,7 +39,7 @@ func testSourceGateway(t *testing.T) { do := func(wantstate sourceState) func(t *testing.T) { return func(t *testing.T) { superv := newSupervisor(ctx) - sc := newSourceCoordinator(superv, newDeductionCoordinator(superv), cachedir) + sc := newSourceCoordinator(superv, newDeductionCoordinator(superv), cachedir, log.New(test.Writer{t}, "", 0)) id := mkPI("github.com/sdboyer/deptest") sg, err := sc.getSourceGatewayFor(ctx, id) diff --git a/internal/test/writer.go b/internal/test/writer.go new file mode 100644 index 0000000000..7fd3a4d948 --- /dev/null +++ b/internal/test/writer.go @@ -0,0 +1,31 @@ +// Copyright 2017 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package test + +import ( + "strings" + "testing" + "unicode" +) + +// Writer adapts a testing.TB to the io.Writer interface +type Writer struct { + testing.TB +} + +func (t Writer) Write(b []byte) (n int, err error) { + str := string(b) + if len(str) == 0 { + return 0, nil + } + + for _, part := range strings.Split(str, "\n") { + str := strings.TrimRightFunc(part, unicode.IsSpace) + if len(str) != 0 { + t.Log(str) + } + } + return len(b), err +}