diff --git a/cmd/dep/ensure.go b/cmd/dep/ensure.go index 63e4868a..1a5cc465 100644 --- a/cmd/dep/ensure.go +++ b/cmd/dep/ensure.go @@ -6,6 +6,7 @@ package main import ( "bytes" + "context" "flag" "fmt" "go/build" @@ -277,10 +278,9 @@ func (cmd *ensureCommand) runDefault(ctx *dep.Ctx, args []string, p *dep.Project return errors.New("Gopkg.lock was not up to date") } - solution, err := solver.Solve() + solution, err := solver.Solve(context.TODO()) if err != nil { - handleAllTheFailuresOfTheWorld(err) - return errors.Wrap(err, "ensure Solve()") + return handleAllTheFailuresOfTheWorld(err) } vendorBehavior := dep.VendorOnChanged @@ -369,13 +369,12 @@ func (cmd *ensureCommand) runUpdate(ctx *dep.Ctx, args []string, p *dep.Project, if err != nil { return errors.Wrap(err, "fastpath solver prepare") } - solution, err := solver.Solve() + solution, err := solver.Solve(context.TODO()) if err != nil { // TODO(sdboyer) special handling for warning cases as described in spec // - e.g., named projects did not upgrade even though newer versions // were available. - handleAllTheFailuresOfTheWorld(err) - return errors.Wrap(err, "ensure Solve()") + return handleAllTheFailuresOfTheWorld(err) } sw, err := dep.NewSafeWriter(nil, p.Lock, dep.LockFromSolution(solution), dep.VendorOnChanged) @@ -631,11 +630,10 @@ func (cmd *ensureCommand) runAdd(ctx *dep.Ctx, args []string, p *dep.Project, sm if err != nil { return errors.Wrap(err, "fastpath solver prepare") } - solution, err := solver.Solve() + solution, err := solver.Solve(context.TODO()) if err != nil { // TODO(sdboyer) detect if the failure was specifically about some of the -add arguments - handleAllTheFailuresOfTheWorld(err) - return errors.Wrap(err, "ensure Solve()") + return handleAllTheFailuresOfTheWorld(err) } // Prep post-actions and feedback from adds. diff --git a/cmd/dep/init.go b/cmd/dep/init.go index a4f6637d..3071ca80 100644 --- a/cmd/dep/init.go +++ b/cmd/dep/init.go @@ -5,6 +5,7 @@ package main import ( + "context" "flag" "io/ioutil" "log" @@ -183,10 +184,9 @@ func (cmd *initCommand) Run(ctx *dep.Ctx, args []string) error { return errors.Wrap(err, "prepare solver") } - soln, err := s.Solve() + soln, err := s.Solve(context.TODO()) if err != nil { - handleAllTheFailuresOfTheWorld(err) - return err + return handleAllTheFailuresOfTheWorld(err) } p.Lock = dep.LockFromSolution(soln) @@ -247,5 +247,11 @@ func getDirectDependencies(sm gps.SourceManager, p *dep.Project) (pkgtree.Packag // TODO solve failures can be really creative - we need to be similarly creative // in handling them and informing the user appropriately -func handleAllTheFailuresOfTheWorld(err error) { +func handleAllTheFailuresOfTheWorld(err error) error { + switch errors.Cause(err) { + case context.Canceled, context.DeadlineExceeded, gps.ErrSourceManagerIsReleased: + return nil + } + + return errors.Wrap(err, "Solving failure") } diff --git a/internal/gps/bridge.go b/internal/gps/bridge.go index 4ffdcb49..a221ccd3 100644 --- a/internal/gps/bridge.go +++ b/internal/gps/bridge.go @@ -67,6 +67,12 @@ type bridge struct { // Whether to sort version lists for downgrade. down bool + + // The cancellation context provided to the solver. Threading it through the + // various solver methods is needlessly verbose so long as we maintain the + // lifetime guarantees that a solver can only be run once. + // TODO(sdboyer) uncomment this and thread it through SourceManager methods + //ctx context.Context } // mkBridge creates a bridge diff --git a/internal/gps/manager_test.go b/internal/gps/manager_test.go index 3ccfa490..9da9519c 100644 --- a/internal/gps/manager_test.go +++ b/internal/gps/manager_test.go @@ -818,57 +818,57 @@ func TestErrAfterRelease(t *testing.T) { _, err := sm.SourceExists(id) if err == nil { t.Errorf("SourceExists did not error after calling Release()") - } else if terr, ok := err.(smIsReleased); !ok { - t.Errorf("SourceExists errored after Release(), but with unexpected error: %T %s", terr, terr.Error()) + } else if err != ErrSourceManagerIsReleased { + t.Errorf("SourceExists errored after Release(), but with unexpected error: %T %s", err, err.Error()) } err = sm.SyncSourceFor(id) if err == nil { t.Errorf("SyncSourceFor did not error after calling Release()") - } else if terr, ok := err.(smIsReleased); !ok { - t.Errorf("SyncSourceFor errored after Release(), but with unexpected error: %T %s", terr, terr.Error()) + } else if err != ErrSourceManagerIsReleased { + t.Errorf("SyncSourceFor errored after Release(), but with unexpected error: %T %s", err, err.Error()) } _, err = sm.ListVersions(id) if err == nil { t.Errorf("ListVersions did not error after calling Release()") - } else if terr, ok := err.(smIsReleased); !ok { - t.Errorf("ListVersions errored after Release(), but with unexpected error: %T %s", terr, terr.Error()) + } else if err != ErrSourceManagerIsReleased { + t.Errorf("ListVersions errored after Release(), but with unexpected error: %T %s", err, err.Error()) } _, err = sm.RevisionPresentIn(id, "") if err == nil { t.Errorf("RevisionPresentIn did not error after calling Release()") - } else if terr, ok := err.(smIsReleased); !ok { - t.Errorf("RevisionPresentIn errored after Release(), but with unexpected error: %T %s", terr, terr.Error()) + } else if err != ErrSourceManagerIsReleased { + t.Errorf("RevisionPresentIn errored after Release(), but with unexpected error: %T %s", err, err.Error()) } _, err = sm.ListPackages(id, nil) if err == nil { t.Errorf("ListPackages did not error after calling Release()") - } else if terr, ok := err.(smIsReleased); !ok { - t.Errorf("ListPackages errored after Release(), but with unexpected error: %T %s", terr, terr.Error()) + } else if err != ErrSourceManagerIsReleased { + t.Errorf("ListPackages errored after Release(), but with unexpected error: %T %s", err, err.Error()) } _, _, err = sm.GetManifestAndLock(id, nil, naiveAnalyzer{}) if err == nil { t.Errorf("GetManifestAndLock did not error after calling Release()") - } else if terr, ok := err.(smIsReleased); !ok { - t.Errorf("GetManifestAndLock errored after Release(), but with unexpected error: %T %s", terr, terr.Error()) + } else if err != ErrSourceManagerIsReleased { + t.Errorf("GetManifestAndLock errored after Release(), but with unexpected error: %T %s", err, err.Error()) } err = sm.ExportProject(context.Background(), id, nil, "") if err == nil { t.Errorf("ExportProject did not error after calling Release()") - } else if terr, ok := err.(smIsReleased); !ok { - t.Errorf("ExportProject errored after Release(), but with unexpected error: %T %s", terr, terr.Error()) + } else if err != ErrSourceManagerIsReleased { + t.Errorf("ExportProject errored after Release(), but with unexpected error: %T %s", err, err.Error()) } _, err = sm.DeduceProjectRoot("") if err == nil { t.Errorf("DeduceProjectRoot did not error after calling Release()") - } else if terr, ok := err.(smIsReleased); !ok { - t.Errorf("DeduceProjectRoot errored after Release(), but with unexpected error: %T %s", terr, terr.Error()) + } else if err != ErrSourceManagerIsReleased { + t.Errorf("DeduceProjectRoot errored after Release(), but with unexpected error: %T %s", err, err.Error()) } } diff --git a/internal/gps/solve_test.go b/internal/gps/solve_test.go index c80d7d7c..607bcd7e 100644 --- a/internal/gps/solve_test.go +++ b/internal/gps/solve_test.go @@ -6,6 +6,7 @@ package gps import ( "bytes" + "context" "fmt" "log" "reflect" @@ -35,7 +36,7 @@ func fixSolve(params SolveParameters, sm SourceManager, t *testing.T) (Solution, return nil, err } - return s.Solve() + return s.Solve(context.Background()) } // Test all the basic table fixtures. diff --git a/internal/gps/solver.go b/internal/gps/solver.go index a325101a..78fce517 100644 --- a/internal/gps/solver.go +++ b/internal/gps/solver.go @@ -6,15 +6,18 @@ package gps import ( "container/heap" + "context" "fmt" "log" "sort" "strings" "sync" + "sync/atomic" "github.com/armon/go-radix" "github.com/golang/dep/internal/gps/paths" "github.com/golang/dep/internal/gps/pkgtree" + "github.com/pkg/errors" ) var rootRev = Revision("") @@ -160,6 +163,10 @@ type solver struct { // metrics for the current solve run. mtr *metrics + + // Indicates whether the solver has been run. It is invalid to run this type + // of solver more than once. + hasrun int32 } func (params SolveParameters) toRootdata() (rootdata, error) { @@ -332,15 +339,18 @@ type Solver interface { // In such a case, it may not be necessary to run Solve() at all. HashInputs() []byte - // Solve initiates a solving run. It will either complete successfully with - // a Solution, or fail with an informative error. - Solve() (Solution, error) + // Solve initiates a solving run. It will either abort due to a canceled + // Context, complete successfully with a Solution, or fail with an + // informative error. + // + // It is generally not allowed that this method be called twice for any + // given solver. + Solve(context.Context) (Solution, error) // Name returns a string identifying the particular solver backend. // // Different solvers likely have different invariants, and likely will not - // have identical possible result sets for any particular inputs; in some - // cases, they may even be disjoint. + // have the same result sets for any particular inputs. Name() string // Version returns an int indicating the version of the solver of the given @@ -427,7 +437,14 @@ func ValidateParams(params SolveParameters, sm SourceManager) error { // represented by the SolveParameters with which this Solver was created. // // This is the entry point to the main gps workhorse. -func (s *solver) Solve() (Solution, error) { +func (s *solver) Solve(ctx context.Context) (Solution, error) { + // Solving can only be run once per solver. + if !atomic.CompareAndSwapInt32(&s.hasrun, 0, 1) { + return nil, errors.New("solve method can only be run once per instance") + } + // Make sure the bridge has the context before we start. + //s.b.ctx = ctx + // Set up a metrics object s.mtr = newMetrics() s.vUnify.mtr = s.mtr @@ -437,7 +454,7 @@ func (s *solver) Solve() (Solution, error) { return nil, err } - all, err := s.solve() + all, err := s.solve(ctx) s.mtr.pop() var soln solution @@ -466,9 +483,19 @@ func (s *solver) Solve() (Solution, error) { } // solve is the top-level loop for the solving process. -func (s *solver) solve() (map[atom]map[string]struct{}, error) { +func (s *solver) solve(ctx context.Context) (map[atom]map[string]struct{}, error) { + // Pull out the donechan once up front so that we're not potentially + // triggering mutex cycling and channel creation on each iteration. + donechan := ctx.Done() + // Main solving loop for { + select { + case <-donechan: + return nil, ctx.Err() + default: + } + bmi, has := s.nextUnselected() if !has { @@ -489,10 +516,13 @@ func (s *solver) solve() (map[atom]map[string]struct{}, error) { // to create a version queue. queue, err := s.createVersionQueue(bmi) if err != nil { + s.mtr.pop() // Err means a failure somewhere down the line; try backtracking. s.traceStartBacktrack(bmi, err, false) - s.mtr.pop() - if s.backtrack() { + success, berr := s.backtrack(ctx) + if berr != nil { + err = berr + } else if success { // backtracking succeeded, move to the next unselected id continue } @@ -510,9 +540,14 @@ func (s *solver) solve() (map[atom]map[string]struct{}, error) { }, pl: bmi.pl, } - s.selectAtom(awp, false) - s.vqs = append(s.vqs, queue) + err = s.selectAtom(awp, false) s.mtr.pop() + if err != nil { + // Only a released SourceManager should be able to cause this. + return nil, err + } + + s.vqs = append(s.vqs, queue) } else { s.mtr.push("add-atom") // We're just trying to add packages to an already-selected project. @@ -537,20 +572,28 @@ func (s *solver) solve() (map[atom]map[string]struct{}, error) { s.traceCheckPkgs(bmi) err := s.check(nawp, true) if err != nil { + s.mtr.pop() // Err means a failure somewhere down the line; try backtracking. s.traceStartBacktrack(bmi, err, true) - if s.backtrack() { + success, berr := s.backtrack(ctx) + if berr != nil { + err = berr + } else if success { // backtracking succeeded, move to the next unselected id continue } - s.mtr.pop() return nil, err } - s.selectAtom(nawp, true) + err = s.selectAtom(nawp, true) + s.mtr.pop() + if err != nil { + // Only a released SourceManager should be able to cause this. + return nil, err + } + // We don't add anything to the stack of version queues because the // backtracker knows not to pop the vqstack if it backtracks // across a pure-package addition. - s.mtr.pop() } } @@ -583,11 +626,14 @@ func (s *solver) selectRoot() error { s.sel.pushSelection(awp, false) // If we're looking for root's deps, get it from opts and local root - // analysis, rather than having the sm do it + // analysis, rather than having the sm do it. deps, err := s.intersectConstraintsWithImports(s.rd.combineConstraints(), s.rd.externalImportList(s.stdLibFn)) if err != nil { + if contextCanceledOrSMReleased(err) { + return err + } // TODO(sdboyer) this could well happen; handle it with a more graceful error - panic(fmt.Sprintf("shouldn't be possible %s", err)) + panic(fmt.Sprintf("canary - shouldn't be possible %s", err)) } for _, dep := range deps { @@ -997,18 +1043,26 @@ func (s *solver) getLockVersionIfValid(id ProjectIdentifier) (Version, error) { // backtrack works backwards from the current failed solution to find the next // solution to try. -func (s *solver) backtrack() bool { +func (s *solver) backtrack(ctx context.Context) (bool, error) { if len(s.vqs) == 0 { // nothing to backtrack to - return false + return false, nil } + donechan := ctx.Done() s.mtr.push("backtrack") + defer s.mtr.pop() for { for { + select { + case <-donechan: + return false, ctx.Err() + default: + } + if len(s.vqs) == 0 { // no more versions, nowhere further to backtrack - return false + return false, nil } if s.vqs[len(s.vqs)-1].failed { break @@ -1020,7 +1074,14 @@ func (s *solver) backtrack() bool { var proj bool var awp atomWithPackages for !proj { - awp, proj = s.unselectLast() + var err error + awp, proj, err = s.unselectLast() + if err != nil { + if !contextCanceledOrSMReleased(err) { + panic(fmt.Sprintf("canary - should only have been able to get a context cancellation or SM release, got %T %s", err, err)) + } + return false, err + } s.traceBacktrack(awp.bmi(), !proj) } } @@ -1033,7 +1094,14 @@ func (s *solver) backtrack() bool { var proj bool var awp atomWithPackages for !proj { - awp, proj = s.unselectLast() + var err error + awp, proj, err = s.unselectLast() + if err != nil { + if !contextCanceledOrSMReleased(err) { + panic(fmt.Sprintf("canary - should only have been able to get a context cancellation or SM release, got %T %s", err, err)) + } + return false, err + } s.traceBacktrack(awp.bmi(), !proj) } @@ -1052,13 +1120,18 @@ func (s *solver) backtrack() bool { // reusing the old awp is fine awp.a.v = q.current() - s.selectAtom(awp, false) + err := s.selectAtom(awp, false) + if err != nil { + if !contextCanceledOrSMReleased(err) { + panic(fmt.Sprintf("canary - should only have been able to get a context cancellation or SM release, got %T %s", err, err)) + } + return false, err + } break } } s.traceBacktrack(awp.bmi(), false) - //s.traceInfo("no more versions of %s, backtracking", q.id.errString()) // No solution found; continue backtracking after popping the queue // we just inspected off the list @@ -1066,13 +1139,12 @@ func (s *solver) backtrack() bool { s.vqs, s.vqs[len(s.vqs)-1] = s.vqs[:len(s.vqs)-1], nil } - s.mtr.pop() // Backtracking was successful if loop ended before running out of versions if len(s.vqs) == 0 { - return false + return false, nil } s.attempts++ - return true + return true, nil } func (s *solver) nextUnselected() (bimodalIdentifier, bool) { @@ -1175,7 +1247,7 @@ func (s *solver) fail(id ProjectIdentifier) { // the unselected priority queue. // // Behavior is slightly diffferent if pkgonly is true. -func (s *solver) selectAtom(a atomWithPackages, pkgonly bool) { +func (s *solver) selectAtom(a atomWithPackages, pkgonly bool) error { s.mtr.push("select-atom") s.unsel.remove(bimodalIdentifier{ id: a.a.id, @@ -1184,6 +1256,9 @@ func (s *solver) selectAtom(a atomWithPackages, pkgonly bool) { pl, deps, err := s.getImportsAndConstraintsOf(a) if err != nil { + if contextCanceledOrSMReleased(err) { + return err + } // This shouldn't be possible; other checks should have ensured all // packages and deps are present for any argument passed to this method. panic(fmt.Sprintf("canary - shouldn't be possible %s", err)) @@ -1267,15 +1342,21 @@ func (s *solver) selectAtom(a atomWithPackages, pkgonly bool) { s.traceSelect(a, pkgonly) s.mtr.pop() + + return nil } -func (s *solver) unselectLast() (atomWithPackages, bool) { +func (s *solver) unselectLast() (atomWithPackages, bool, error) { s.mtr.push("unselect") + defer s.mtr.pop() awp, first := s.sel.popSelection() heap.Push(s.unsel, bimodalIdentifier{id: awp.a.id, pl: awp.pl}) _, deps, err := s.getImportsAndConstraintsOf(awp) if err != nil { + if contextCanceledOrSMReleased(err) { + return atomWithPackages{}, false, err + } // This shouldn't be possible; other checks should have ensured all // packages and deps are present for any argument passed to this method. panic(fmt.Sprintf("canary - shouldn't be possible %s", err)) @@ -1296,8 +1377,7 @@ func (s *solver) unselectLast() (atomWithPackages, bool) { } } - s.mtr.pop() - return awp, first + return awp, first, nil } // simple (temporary?) helper just to convert atoms into locked projects @@ -1335,3 +1415,7 @@ func pa2lp(pa atom, pkgs map[string]struct{}) LockedProject { return lp } + +func contextCanceledOrSMReleased(err error) bool { + return err == context.Canceled || err == context.DeadlineExceeded || err == ErrSourceManagerIsReleased +} diff --git a/internal/gps/source_manager.go b/internal/gps/source_manager.go index 926c6232..021fca8f 100644 --- a/internal/gps/source_manager.go +++ b/internal/gps/source_manager.go @@ -167,14 +167,13 @@ type SourceMgr struct { releasing int32 // flag indicating release of sm has begun } -type smIsReleased struct{} - -func (smIsReleased) Error() string { - return "this SourceMgr has been released, its methods can no longer be called" -} - var _ SourceManager = &SourceMgr{} +// ErrSourceManagerIsReleased is the error returned by any SourceManager method +// called after the SourceManager has been released, rendering its methods no +// longer safe to call. +var ErrSourceManagerIsReleased error = fmt.Errorf("this SourceManager has been released, its methods can no longer be called") + // SourceManagerConfig holds configuration information for creating SourceMgrs. type SourceManagerConfig struct { Cachedir string // Where to store local instances of upstream sources. @@ -406,7 +405,7 @@ func (sm *SourceMgr) Release() { // DeriveManifestAndLock() method. func (sm *SourceMgr) GetManifestAndLock(id ProjectIdentifier, v Version, an ProjectAnalyzer) (Manifest, Lock, error) { if atomic.LoadInt32(&sm.releasing) == 1 { - return nil, nil, smIsReleased{} + return nil, nil, ErrSourceManagerIsReleased } srcg, err := sm.srcCoord.getSourceGatewayFor(context.TODO(), id) @@ -421,7 +420,7 @@ func (sm *SourceMgr) GetManifestAndLock(id ProjectIdentifier, v Version, an Proj // of the given ProjectIdentifier, at the given version. func (sm *SourceMgr) ListPackages(id ProjectIdentifier, v Version) (pkgtree.PackageTree, error) { if atomic.LoadInt32(&sm.releasing) == 1 { - return pkgtree.PackageTree{}, smIsReleased{} + return pkgtree.PackageTree{}, ErrSourceManagerIsReleased } srcg, err := sm.srcCoord.getSourceGatewayFor(context.TODO(), id) @@ -446,7 +445,7 @@ func (sm *SourceMgr) ListPackages(id ProjectIdentifier, v Version) (pkgtree.Pack // went away), an error will be returned. func (sm *SourceMgr) ListVersions(id ProjectIdentifier) ([]PairedVersion, error) { if atomic.LoadInt32(&sm.releasing) == 1 { - return nil, smIsReleased{} + return nil, ErrSourceManagerIsReleased } srcg, err := sm.srcCoord.getSourceGatewayFor(context.TODO(), id) @@ -462,7 +461,7 @@ func (sm *SourceMgr) ListVersions(id ProjectIdentifier) ([]PairedVersion, error) // repository. func (sm *SourceMgr) RevisionPresentIn(id ProjectIdentifier, r Revision) (bool, error) { if atomic.LoadInt32(&sm.releasing) == 1 { - return false, smIsReleased{} + return false, ErrSourceManagerIsReleased } srcg, err := sm.srcCoord.getSourceGatewayFor(context.TODO(), id) @@ -478,7 +477,7 @@ func (sm *SourceMgr) RevisionPresentIn(id ProjectIdentifier, r Revision) (bool, // for the provided ProjectIdentifier. func (sm *SourceMgr) SourceExists(id ProjectIdentifier) (bool, error) { if atomic.LoadInt32(&sm.releasing) == 1 { - return false, smIsReleased{} + return false, ErrSourceManagerIsReleased } srcg, err := sm.srcCoord.getSourceGatewayFor(context.TODO(), id) @@ -496,7 +495,7 @@ func (sm *SourceMgr) SourceExists(id ProjectIdentifier) (bool, error) { // The primary use case for this is prefetching. func (sm *SourceMgr) SyncSourceFor(id ProjectIdentifier) error { if atomic.LoadInt32(&sm.releasing) == 1 { - return smIsReleased{} + return ErrSourceManagerIsReleased } srcg, err := sm.srcCoord.getSourceGatewayFor(context.TODO(), id) @@ -511,7 +510,7 @@ func (sm *SourceMgr) SyncSourceFor(id ProjectIdentifier) error { // ProjectRoot, at the provided version, to the provided directory. func (sm *SourceMgr) ExportProject(ctx context.Context, id ProjectIdentifier, v Version, to string) error { if atomic.LoadInt32(&sm.releasing) == 1 { - return smIsReleased{} + return ErrSourceManagerIsReleased } srcg, err := sm.srcCoord.getSourceGatewayFor(ctx, id) @@ -531,7 +530,7 @@ func (sm *SourceMgr) ExportProject(ctx context.Context, id ProjectIdentifier, v // activity, as its behavior is well-structured) func (sm *SourceMgr) DeduceProjectRoot(ip string) (ProjectRoot, error) { if atomic.LoadInt32(&sm.releasing) == 1 { - return "", smIsReleased{} + return "", ErrSourceManagerIsReleased } pd, err := sm.deduceCoord.deduceRootPath(context.TODO(), ip)