diff --git a/context.go b/context.go index c46537b..01fa89f 100644 --- a/context.go +++ b/context.go @@ -2074,23 +2074,14 @@ func parallelVisit(modules []*moduleInfo, order visitOrderer, limit int, } if len(pauseMap) > 0 { - // Probably a deadlock due to a newly added dependency cycle. - // Start from a semi-random module being paused for and perform a depth-first - // search for the module it is paused on, ignoring modules that are marked as - // done. Note this traverses from modules to the modules that would have been - // unblocked when that module finished. - var start, end *moduleInfo - for _, pauseSpecs := range pauseMap { - for _, pauseSpec := range pauseSpecs { - if start == nil || start.String() > pauseSpec.paused.String() { - start = pauseSpec.paused - end = pauseSpec.until - } - } - } + // Probably a deadlock due to a newly added dependency cycle. Start from each module in + // the order of the input modules list and perform a depth-first search for the module + // it is paused on, ignoring modules that are marked as done. Note this traverses from + // modules to the modules that would have been unblocked when that module finished, i.e + // the reverse of the visitOrderer. - var check func(group *moduleInfo) []*moduleInfo - check = func(module *moduleInfo) []*moduleInfo { + var check func(module, end *moduleInfo) []*moduleInfo + check = func(module, end *moduleInfo) []*moduleInfo { if module.waitingCount == -1 { // This module was finished, it can't be part of a loop. return nil @@ -2101,13 +2092,13 @@ func parallelVisit(modules []*moduleInfo, order visitOrderer, limit int, } for _, dep := range order.propagate(module) { - cycle := check(dep) + cycle := check(dep, end) if cycle != nil { return append([]*moduleInfo{module}, cycle...) } } for _, depPauseSpec := range pauseMap[module] { - cycle := check(depPauseSpec.paused) + cycle := check(depPauseSpec.paused, end) if cycle != nil { return append([]*moduleInfo{module}, cycle...) } @@ -2116,9 +2107,14 @@ func parallelVisit(modules []*moduleInfo, order visitOrderer, limit int, return nil } - cycle := check(start) - if cycle != nil { - return cycleError(cycle) + // Iterate over the modules list instead of pauseMap to provide deterministic ordering. + for _, module := range modules { + for _, pauseSpec := range pauseMap[module] { + cycle := check(pauseSpec.paused, pauseSpec.until) + if len(cycle) > 0 { + return cycleError(cycle) + } + } } } diff --git a/context_test.go b/context_test.go index dd5ec38..3e49a18 100644 --- a/context_test.go +++ b/context_test.go @@ -840,38 +840,31 @@ func Test_findVariant(t *testing.T) { } func Test_parallelVisit(t *testing.T) { - moduleA := &moduleInfo{ - group: &moduleGroup{ - name: "A", - }, - } - moduleB := &moduleInfo{ - group: &moduleGroup{ - name: "B", - }, - } - moduleC := &moduleInfo{ - group: &moduleGroup{ - name: "C", - }, - } - moduleD := &moduleInfo{ - group: &moduleGroup{ - name: "D", - }, - } - moduleA.group.modules = modulesOrAliases{moduleA} - moduleB.group.modules = modulesOrAliases{moduleB} - moduleC.group.modules = modulesOrAliases{moduleC} - moduleD.group.modules = modulesOrAliases{moduleD} - addDep := func(from, to *moduleInfo) { from.directDeps = append(from.directDeps, depInfo{to, nil}) from.forwardDeps = append(from.forwardDeps, to) to.reverseDeps = append(to.reverseDeps, from) } - // A depends on B, B depends on C. Nothing depends on D, and D doesn't depend on anything. + create := func(name string) *moduleInfo { + m := &moduleInfo{ + group: &moduleGroup{ + name: name, + }, + } + m.group.modules = modulesOrAliases{m} + return m + } + moduleA := create("A") + moduleB := create("B") + moduleC := create("C") + moduleD := create("D") + moduleE := create("E") + moduleF := create("F") + moduleG := create("G") + + // A depends on B, B depends on C. Nothing depends on D through G, and they don't depend on + // anything. addDep(moduleA, moduleB) addDep(moduleB, moduleC) @@ -1037,8 +1030,45 @@ func Test_parallelVisit(t *testing.T) { }) want := []string{ `encountered dependency cycle`, - `"C" depends on "D"`, `"D" depends on "C"`, + `"C" depends on "D"`, + } + for i := range want { + if len(errs) <= i { + t.Errorf("missing error %s", want[i]) + } else if !strings.Contains(errs[i].Error(), want[i]) { + t.Errorf("expected error %s, got %s", want[i], errs[i]) + } + } + if len(errs) > len(want) { + for _, err := range errs[len(want):] { + t.Errorf("unexpected error %s", err.Error()) + } + } + }) + t.Run("pause cycle with deps", func(t *testing.T) { + pauseDeps := map[*moduleInfo]*moduleInfo{ + // F and G form a pause cycle + moduleF: moduleG, + moduleG: moduleF, + // D depends on E which depends on the pause cycle, making E the first alphabetical + // entry in pauseMap, which is not part of the cycle. + moduleD: moduleE, + moduleE: moduleF, + } + errs := parallelVisit([]*moduleInfo{moduleD, moduleE, moduleF, moduleG}, bottomUpVisitorImpl{}, 4, + func(module *moduleInfo, pause chan<- pauseSpec) bool { + if dep, ok := pauseDeps[module]; ok { + unpause := make(chan struct{}) + pause <- pauseSpec{module, dep, unpause} + <-unpause + } + return false + }) + want := []string{ + `encountered dependency cycle`, + `"G" depends on "F"`, + `"F" depends on "G"`, } for i := range want { if len(errs) <= i { diff --git a/proptools/tag.go b/proptools/tag.go index d69853a..b28ec3b 100644 --- a/proptools/tag.go +++ b/proptools/tag.go @@ -23,10 +23,17 @@ import ( // HasTag returns true if a StructField has a tag in the form `name:"foo,value"`. func HasTag(field reflect.StructField, name, value string) bool { tag := field.Tag.Get(name) - for _, entry := range strings.Split(tag, ",") { - if entry == value { + for len(tag) > 0 { + idx := strings.Index(tag, ",") + + if idx < 0 { + return tag == value + } + if tag[:idx] == value { return true } + + tag = tag[idx+1:] } return false diff --git a/proptools/tag_test.go b/proptools/tag_test.go index 0041c54..bbd2324 100644 --- a/proptools/tag_test.go +++ b/proptools/tag_test.go @@ -19,16 +19,16 @@ import ( "testing" ) -func TestHasTag(t *testing.T) { - type testType struct { - NoTag string - EmptyTag string `` - OtherTag string `foo:"bar"` - MatchingTag string `name:"value"` - ExtraValues string `name:"foo,value,bar"` - ExtraTags string `foo:"bar" name:"value"` - } +type testType struct { + NoTag string + EmptyTag string `` + OtherTag string `foo:"bar"` + MatchingTag string `name:"value"` + ExtraValues string `name:"foo,value,bar"` + ExtraTags string `foo:"bar" name:"value"` +} +func TestHasTag(t *testing.T) { tests := []struct { field string want bool @@ -68,6 +68,39 @@ func TestHasTag(t *testing.T) { } } +func BenchmarkHasTag(b *testing.B) { + tests := []struct { + field string + }{ + { + field: "NoTag", + }, + { + field: "EmptyTag", + }, + { + field: "OtherTag", + }, + { + field: "MatchingTag", + }, + { + field: "ExtraValues", + }, + { + field: "ExtraTags", + }, + } + for _, test := range tests { + b.Run(test.field, func(b *testing.B) { + field, _ := reflect.TypeOf(testType{}).FieldByName(test.field) + for i := 0; i < b.N; i++ { + HasTag(field, "name", "value") + } + }) + } +} + func TestPropertyIndexesWithTag(t *testing.T) { tests := []struct { name string