From 6ad7ad2ce657b2ac478b4732047891c7dc9d0d86 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Sun, 1 Oct 2017 22:17:48 -0400 Subject: [PATCH 01/12] gps: Add lowest-level cancellation handling This softens all the spots where canary panics were used to indicate that, barring a released SourceManager, an error was indicative of a problem with the solver's invariants. --- internal/gps/solver.go | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/internal/gps/solver.go b/internal/gps/solver.go index a325101a..4508475c 100644 --- a/internal/gps/solver.go +++ b/internal/gps/solver.go @@ -583,11 +583,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 { @@ -1175,7 +1178,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 +1187,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)) @@ -1269,13 +1275,16 @@ func (s *solver) selectAtom(a atomWithPackages, pkgonly bool) { s.mtr.pop() } -func (s *solver) unselectLast() (atomWithPackages, bool) { +func (s *solver) unselectLast() (atomWithPackages, bool, error) { s.mtr.push("unselect") 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)) @@ -1335,3 +1344,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 == smIsReleased{} +} From e1701ef034cc658bbcd58b5be2e25520ec75f259 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Sun, 1 Oct 2017 22:52:30 -0400 Subject: [PATCH 02/12] gps: Add err handling to those reliant on roots --- internal/gps/bridge.go | 6 ++ internal/gps/solve_test.go | 3 +- internal/gps/solver.go | 112 +++++++++++++++++++++++++++++-------- 3 files changed, 97 insertions(+), 24 deletions(-) diff --git a/internal/gps/bridge.go b/internal/gps/bridge.go index 4ffdcb49..374d33d5 100644 --- a/internal/gps/bridge.go +++ b/internal/gps/bridge.go @@ -5,6 +5,7 @@ package gps import ( + "context" "fmt" "os" "path/filepath" @@ -67,6 +68,11 @@ 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, so we + ctx context.Context } // mkBridge creates a bridge 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 4508475c..6e692ec0 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,9 +339,13 @@ 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. // @@ -427,7 +438,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.LoadInt32(&s.hasrun) == 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 +455,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 +484,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 { @@ -491,11 +519,14 @@ func (s *solver) solve() (map[atom]map[string]struct{}, error) { if err != nil { // 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 } + s.mtr.pop() return nil, err } @@ -510,7 +541,12 @@ func (s *solver) solve() (map[atom]map[string]struct{}, error) { }, pl: bmi.pl, } - s.selectAtom(awp, false) + err = s.selectAtom(awp, false) + if err != nil { + // Only a released SourceManager should be able to cause this. + return nil, err + } + s.vqs = append(s.vqs, queue) s.mtr.pop() } else { @@ -539,14 +575,22 @@ func (s *solver) solve() (map[atom]map[string]struct{}, error) { if err != nil { // 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, false) + 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. @@ -1000,18 +1044,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 @@ -1023,7 +1075,12 @@ 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 { + // Should only be possible if SourceManager was released + return false, err + } s.traceBacktrack(awp.bmi(), !proj) } } @@ -1036,7 +1093,12 @@ 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 { + // Should only be possible if SourceManager was released + return false, err + } s.traceBacktrack(awp.bmi(), !proj) } @@ -1055,13 +1117,16 @@ 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 { + // Only a released SourceManager should be able to cause this. + 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 @@ -1069,13 +1134,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) { @@ -1273,10 +1337,13 @@ func (s *solver) selectAtom(a atomWithPackages, pkgonly bool) error { s.traceSelect(a, pkgonly) s.mtr.pop() + + return nil } 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}) @@ -1305,8 +1372,7 @@ func (s *solver) unselectLast() (atomWithPackages, bool, error) { } } - s.mtr.pop() - return awp, first + return awp, first, nil } // simple (temporary?) helper just to convert atoms into locked projects From 8656e387a53ee04ef963e831b1f164748d678f56 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Mon, 2 Oct 2017 20:07:57 -0400 Subject: [PATCH 03/12] gps: Fix metrics start/stop sequencing --- internal/gps/solver.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/gps/solver.go b/internal/gps/solver.go index 6e692ec0..c5c65a54 100644 --- a/internal/gps/solver.go +++ b/internal/gps/solver.go @@ -517,6 +517,7 @@ func (s *solver) solve(ctx context.Context) (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) success, berr := s.backtrack(ctx) @@ -526,7 +527,6 @@ func (s *solver) solve(ctx context.Context) (map[atom]map[string]struct{}, error // backtracking succeeded, move to the next unselected id continue } - s.mtr.pop() return nil, err } @@ -542,13 +542,13 @@ func (s *solver) solve(ctx context.Context) (map[atom]map[string]struct{}, error pl: bmi.pl, } 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) - s.mtr.pop() } else { s.mtr.push("add-atom") // We're just trying to add packages to an already-selected project. @@ -573,6 +573,7 @@ func (s *solver) solve(ctx context.Context) (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) success, berr := s.backtrack(ctx) @@ -582,10 +583,10 @@ func (s *solver) solve(ctx context.Context) (map[atom]map[string]struct{}, error // backtracking succeeded, move to the next unselected id continue } - s.mtr.pop() return nil, err } err = s.selectAtom(nawp, false) + s.mtr.pop() if err != nil { // Only a released SourceManager should be able to cause this. return nil, err @@ -594,7 +595,6 @@ func (s *solver) solve(ctx context.Context) (map[atom]map[string]struct{}, error // 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() } } From bab9ecfd98bca4b004e3fbbe545b7239b607a136 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Tue, 24 Oct 2017 15:28:12 -0400 Subject: [PATCH 04/12] dep: Plumb context through init and ensure --- cmd/dep/ensure.go | 7 ++++--- cmd/dep/init.go | 3 ++- internal/gps/solver.go | 3 +-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/cmd/dep/ensure.go b/cmd/dep/ensure.go index 63e4868a..2049fb6b 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,7 +278,7 @@ 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()") @@ -369,7 +370,7 @@ 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 @@ -631,7 +632,7 @@ 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) diff --git a/cmd/dep/init.go b/cmd/dep/init.go index a4f6637d..91e4f046 100644 --- a/cmd/dep/init.go +++ b/cmd/dep/init.go @@ -5,6 +5,7 @@ package main import ( + "context" "flag" "io/ioutil" "log" @@ -183,7 +184,7 @@ 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 diff --git a/internal/gps/solver.go b/internal/gps/solver.go index c5c65a54..93e933b1 100644 --- a/internal/gps/solver.go +++ b/internal/gps/solver.go @@ -350,8 +350,7 @@ type Solver interface { // 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 From b1876d8c00de02120502d1a6eee4cfdd5b84d663 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Tue, 24 Oct 2017 15:28:35 -0400 Subject: [PATCH 05/12] gps: Use CAS to enforce once-only run of solver --- internal/gps/solver.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/gps/solver.go b/internal/gps/solver.go index 93e933b1..dddfba89 100644 --- a/internal/gps/solver.go +++ b/internal/gps/solver.go @@ -439,7 +439,7 @@ func ValidateParams(params SolveParameters, sm SourceManager) error { // This is the entry point to the main gps workhorse. func (s *solver) Solve(ctx context.Context) (Solution, error) { // Solving can only be run once per solver. - if atomic.LoadInt32(&s.hasrun) == 1 { + 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. From a39ec77cc7a7467544dd91ee2d3ab6cdd5ebbf84 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Tue, 24 Oct 2017 15:29:38 -0400 Subject: [PATCH 06/12] gps: More aggressive canary panics in backtrack --- internal/gps/solver.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/internal/gps/solver.go b/internal/gps/solver.go index dddfba89..0d6fabcc 100644 --- a/internal/gps/solver.go +++ b/internal/gps/solver.go @@ -1077,7 +1077,9 @@ func (s *solver) backtrack(ctx context.Context) (bool, error) { var err error awp, proj, err = s.unselectLast() if err != nil { - // Should only be possible if SourceManager was released + 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) @@ -1095,7 +1097,9 @@ func (s *solver) backtrack(ctx context.Context) (bool, error) { var err error awp, proj, err = s.unselectLast() if err != nil { - // Should only be possible if SourceManager was released + 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) @@ -1118,7 +1122,9 @@ func (s *solver) backtrack(ctx context.Context) (bool, error) { awp.a.v = q.current() err := s.selectAtom(awp, false) if err != nil { - // Only a released SourceManager should be able to cause this. + 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 From f3c5e8310f7c5d28c3219e70ca3ae46151f8cfa3 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Tue, 24 Oct 2017 15:44:36 -0400 Subject: [PATCH 07/12] gps: Fix flipped bool on selectAtom() This was a bit of copy-pasta from an earlier commit, and caused the solver's internal state to be thrown entirely out of whack. --- internal/gps/solver.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/gps/solver.go b/internal/gps/solver.go index 0d6fabcc..1254688b 100644 --- a/internal/gps/solver.go +++ b/internal/gps/solver.go @@ -584,7 +584,7 @@ func (s *solver) solve(ctx context.Context) (map[atom]map[string]struct{}, error } return nil, err } - err = s.selectAtom(nawp, false) + err = s.selectAtom(nawp, true) s.mtr.pop() if err != nil { // Only a released SourceManager should be able to cause this. From 3a5d5b0e236f03d6154d6fba625d076c6f324d67 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Tue, 24 Oct 2017 15:47:11 -0400 Subject: [PATCH 08/12] gps: Chop off incomplete comment --- internal/gps/bridge.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/gps/bridge.go b/internal/gps/bridge.go index 374d33d5..1bd3f26f 100644 --- a/internal/gps/bridge.go +++ b/internal/gps/bridge.go @@ -71,7 +71,7 @@ type bridge struct { // 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, so we + // lifetime guarantees that a solver can only be run once. ctx context.Context } From 1bd138dc20946be3399014f35384fcb8f0ef7781 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Tue, 24 Oct 2017 20:39:05 -0400 Subject: [PATCH 09/12] gps: Comment out ctx on bridge Not ready to use this just quite yet. --- internal/gps/bridge.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/gps/bridge.go b/internal/gps/bridge.go index 1bd3f26f..a221ccd3 100644 --- a/internal/gps/bridge.go +++ b/internal/gps/bridge.go @@ -5,7 +5,6 @@ package gps import ( - "context" "fmt" "os" "path/filepath" @@ -72,7 +71,8 @@ type bridge struct { // 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. - ctx context.Context + // TODO(sdboyer) uncomment this and thread it through SourceManager methods + //ctx context.Context } // mkBridge creates a bridge From b1d298a68832a91fb74b58ce2f0c407a642e14ec Mon Sep 17 00:00:00 2001 From: sam boyer Date: Tue, 24 Oct 2017 21:27:07 -0400 Subject: [PATCH 10/12] gps: Use public sentinel error for released SM Sentinel errors like this are less than great, but they'll allow the public to sniff it, which is still an improvement. --- internal/gps/manager_test.go | 32 ++++++++++++++++---------------- internal/gps/solver.go | 2 +- internal/gps/source_manager.go | 27 +++++++++++++-------------- 3 files changed, 30 insertions(+), 31 deletions(-) diff --git a/internal/gps/manager_test.go b/internal/gps/manager_test.go index cb9d8819..a0619b32 100644 --- a/internal/gps/manager_test.go +++ b/internal/gps/manager_test.go @@ -810,57 +810,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 != SourceManagerIsReleased { + 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 != SourceManagerIsReleased { + 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 != SourceManagerIsReleased { + 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 != SourceManagerIsReleased { + 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 != SourceManagerIsReleased { + 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 != SourceManagerIsReleased { + 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 != SourceManagerIsReleased { + 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 != SourceManagerIsReleased { + t.Errorf("DeduceProjectRoot errored after Release(), but with unexpected error: %T %s", err, err.Error()) } } diff --git a/internal/gps/solver.go b/internal/gps/solver.go index 1254688b..8625b1c2 100644 --- a/internal/gps/solver.go +++ b/internal/gps/solver.go @@ -1417,5 +1417,5 @@ func pa2lp(pa atom, pkgs map[string]struct{}) LockedProject { } func contextCanceledOrSMReleased(err error) bool { - return err == context.Canceled || err == context.DeadlineExceeded || err == smIsReleased{} + return err == context.Canceled || err == context.DeadlineExceeded || err == SourceManagerIsReleased } diff --git a/internal/gps/source_manager.go b/internal/gps/source_manager.go index 926c6232..c0a2b80c 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{} +// SourceManagerIsReleased is the error returned by any SourceManager method +// called after the SourceManager has been released, rendering its methods no +// longer safe to call. +var SourceManagerIsReleased 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, SourceManagerIsReleased } 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{}, SourceManagerIsReleased } 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, SourceManagerIsReleased } 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, SourceManagerIsReleased } 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, SourceManagerIsReleased } 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 SourceManagerIsReleased } 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 SourceManagerIsReleased } 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 "", SourceManagerIsReleased } pd, err := sm.deduceCoord.deduceRootPath(context.TODO(), ip) From be7b31d63139768f593ce3444d31916e44030fef Mon Sep 17 00:00:00 2001 From: sam boyer Date: Tue, 24 Oct 2017 21:31:38 -0400 Subject: [PATCH 11/12] dep: Sniff errors to avoid misleading output The user doesn't really need to know that, for example, a context cancellation occurred when they're likely the ones that just sent an interrupt via Ctrl-C. --- cmd/dep/ensure.go | 9 +++------ cmd/dep/init.go | 11 ++++++++--- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/cmd/dep/ensure.go b/cmd/dep/ensure.go index 2049fb6b..1a5cc465 100644 --- a/cmd/dep/ensure.go +++ b/cmd/dep/ensure.go @@ -280,8 +280,7 @@ func (cmd *ensureCommand) runDefault(ctx *dep.Ctx, args []string, p *dep.Project solution, err := solver.Solve(context.TODO()) if err != nil { - handleAllTheFailuresOfTheWorld(err) - return errors.Wrap(err, "ensure Solve()") + return handleAllTheFailuresOfTheWorld(err) } vendorBehavior := dep.VendorOnChanged @@ -375,8 +374,7 @@ func (cmd *ensureCommand) runUpdate(ctx *dep.Ctx, args []string, p *dep.Project, // 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) @@ -635,8 +633,7 @@ func (cmd *ensureCommand) runAdd(ctx *dep.Ctx, args []string, p *dep.Project, sm 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 91e4f046..80a1bf3e 100644 --- a/cmd/dep/init.go +++ b/cmd/dep/init.go @@ -186,8 +186,7 @@ func (cmd *initCommand) Run(ctx *dep.Ctx, args []string) error { soln, err := s.Solve(context.TODO()) if err != nil { - handleAllTheFailuresOfTheWorld(err) - return err + return handleAllTheFailuresOfTheWorld(err) } p.Lock = dep.LockFromSolution(soln) @@ -248,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.SourceManagerIsReleased: + return nil + } + + return errors.Wrap(err, "Solving failure") } From 2ade7e7139c2e4f20e107d409ef2386659379fc9 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Fri, 27 Oct 2017 00:10:38 -0400 Subject: [PATCH 12/12] gps: Fix error name in accordance with lint rules --- cmd/dep/init.go | 2 +- internal/gps/manager_test.go | 16 ++++++++-------- internal/gps/solver.go | 2 +- internal/gps/source_manager.go | 20 ++++++++++---------- 4 files changed, 20 insertions(+), 20 deletions(-) diff --git a/cmd/dep/init.go b/cmd/dep/init.go index 80a1bf3e..3071ca80 100644 --- a/cmd/dep/init.go +++ b/cmd/dep/init.go @@ -249,7 +249,7 @@ func getDirectDependencies(sm gps.SourceManager, p *dep.Project) (pkgtree.Packag // in handling them and informing the user appropriately func handleAllTheFailuresOfTheWorld(err error) error { switch errors.Cause(err) { - case context.Canceled, context.DeadlineExceeded, gps.SourceManagerIsReleased: + case context.Canceled, context.DeadlineExceeded, gps.ErrSourceManagerIsReleased: return nil } diff --git a/internal/gps/manager_test.go b/internal/gps/manager_test.go index a0619b32..98e264ec 100644 --- a/internal/gps/manager_test.go +++ b/internal/gps/manager_test.go @@ -810,56 +810,56 @@ func TestErrAfterRelease(t *testing.T) { _, err := sm.SourceExists(id) if err == nil { t.Errorf("SourceExists did not error after calling Release()") - } else if err != SourceManagerIsReleased { + } 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 err != SourceManagerIsReleased { + } 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 err != SourceManagerIsReleased { + } 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 err != SourceManagerIsReleased { + } 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 err != SourceManagerIsReleased { + } 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 err != SourceManagerIsReleased { + } 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 err != SourceManagerIsReleased { + } 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 err != SourceManagerIsReleased { + } else if err != ErrSourceManagerIsReleased { t.Errorf("DeduceProjectRoot errored after Release(), but with unexpected error: %T %s", err, err.Error()) } } diff --git a/internal/gps/solver.go b/internal/gps/solver.go index 8625b1c2..78fce517 100644 --- a/internal/gps/solver.go +++ b/internal/gps/solver.go @@ -1417,5 +1417,5 @@ func pa2lp(pa atom, pkgs map[string]struct{}) LockedProject { } func contextCanceledOrSMReleased(err error) bool { - return err == context.Canceled || err == context.DeadlineExceeded || err == SourceManagerIsReleased + return err == context.Canceled || err == context.DeadlineExceeded || err == ErrSourceManagerIsReleased } diff --git a/internal/gps/source_manager.go b/internal/gps/source_manager.go index c0a2b80c..021fca8f 100644 --- a/internal/gps/source_manager.go +++ b/internal/gps/source_manager.go @@ -169,10 +169,10 @@ type SourceMgr struct { var _ SourceManager = &SourceMgr{} -// SourceManagerIsReleased is the error returned by any SourceManager method +// 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 SourceManagerIsReleased error = fmt.Errorf("this SourceManager has been released, its methods can no longer be called") +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 { @@ -405,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, SourceManagerIsReleased + return nil, nil, ErrSourceManagerIsReleased } srcg, err := sm.srcCoord.getSourceGatewayFor(context.TODO(), id) @@ -420,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{}, SourceManagerIsReleased + return pkgtree.PackageTree{}, ErrSourceManagerIsReleased } srcg, err := sm.srcCoord.getSourceGatewayFor(context.TODO(), id) @@ -445,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, SourceManagerIsReleased + return nil, ErrSourceManagerIsReleased } srcg, err := sm.srcCoord.getSourceGatewayFor(context.TODO(), id) @@ -461,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, SourceManagerIsReleased + return false, ErrSourceManagerIsReleased } srcg, err := sm.srcCoord.getSourceGatewayFor(context.TODO(), id) @@ -477,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, SourceManagerIsReleased + return false, ErrSourceManagerIsReleased } srcg, err := sm.srcCoord.getSourceGatewayFor(context.TODO(), id) @@ -495,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 SourceManagerIsReleased + return ErrSourceManagerIsReleased } srcg, err := sm.srcCoord.getSourceGatewayFor(context.TODO(), id) @@ -510,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 SourceManagerIsReleased + return ErrSourceManagerIsReleased } srcg, err := sm.srcCoord.getSourceGatewayFor(ctx, id) @@ -530,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 "", SourceManagerIsReleased + return "", ErrSourceManagerIsReleased } pd, err := sm.deduceCoord.deduceRootPath(context.TODO(), ip)