Merge pull request #1024 from jmank88/source_cache_close

gps: source cache: adding close() methods

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()
 

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()
 

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

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

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)
 	}

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
 	}
 

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 }

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()
 

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()

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

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"))

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)