From fb6efb339b401f5ea8b2f96a3560a537288ef05e Mon Sep 17 00:00:00 2001 From: dmitri Date: Mon, 27 Sep 2021 23:08:15 +0200 Subject: [PATCH 1/2] Persist the fact that a dependency might have failed and replay it each time (*onceFun).run is invoked. Fixes https://github.com/magefile/mage/issues/371. --- mage/main_test.go | 31 +++++++++++++++- mage/testdata/racy_deps/magefile.go | 33 +++++++++++++++++ mg/deps.go | 57 +++++++++++++++++++---------- 3 files changed, 100 insertions(+), 21 deletions(-) create mode 100644 mage/testdata/racy_deps/magefile.go diff --git a/mage/main_test.go b/mage/main_test.go index 2074c64a..3de8010f 100644 --- a/mage/main_test.go +++ b/mage/main_test.go @@ -1749,7 +1749,36 @@ func TestWrongDependency(t *testing.T) { } } -// / This code liberally borrowed from https://github.com/rsc/goversion/blob/master/version/exe.go +// See https://github.com/magefile/mage/issues/371 +func TestRacyDependency(t *testing.T) { + var stdout, stderr bytes.Buffer + inv := Invocation{ + Dir: "./testdata/racy_deps", + Args: []string{"test4"}, + Stderr: &stderr, + Stdout: &stdout, + } + code := Invoke(inv) + if code != 1 { + t.Fatalf("expected 1, but got %v", code) + } + expectedStderr := `Error: error test1 +error test1 +error test1 +` + actualStderr := stderr.String() + if actualStderr != expectedStderr { + t.Fatalf("expected %q, but got %q", expectedStderr, actualStderr) + } + expectedStdout := `execute test1 +` + actualStdout := stdout.String() + if actualStdout != expectedStdout { + t.Fatalf("expected %q, but got %q", expectedStdout, actualStdout) + } +} + +// This code liberally borrowed from https://github.com/rsc/goversion/blob/master/version/exe.go type ( exeType int diff --git a/mage/testdata/racy_deps/magefile.go b/mage/testdata/racy_deps/magefile.go new file mode 100644 index 00000000..c8627d14 --- /dev/null +++ b/mage/testdata/racy_deps/magefile.go @@ -0,0 +1,33 @@ +// +build mage + +package main + +import ( + "fmt" + "time" + + "github.com/magefile/mage/mg" +) + +func Test1() error { + fmt.Println("execute test1") + return fmt.Errorf("error test1") +} + +func Test2() error { + mg.Deps(Test1) + fmt.Println("execute test2") + return nil +} + +func Test3() error { + time.Sleep(time.Second) + mg.Deps(Test2) + fmt.Println("execute test3") + return nil +} +func Test4() error { + mg.Deps(Test1, Test2, Test3) + fmt.Println("execute test4") + return nil +} diff --git a/mg/deps.go b/mg/deps.go index f0c2509b..841429c5 100644 --- a/mg/deps.go +++ b/mg/deps.go @@ -2,7 +2,6 @@ package mg import ( "context" - "fmt" "log" "os" "reflect" @@ -36,7 +35,6 @@ func (o *onceMap) LoadOrStore(f Fn) *onceFun { return existing } one := &onceFun{ - once: &sync.Once{}, fn: f, displayName: displayName(f.Name()), } @@ -91,37 +89,44 @@ func CtxDeps(ctx context.Context, fns ...interface{}) { // runDeps assumes you've already called checkFns. func runDeps(ctx context.Context, fns []Fn) { - mu := &sync.Mutex{} - var errs []string - var exit int - wg := &sync.WaitGroup{} + resultCh := make(chan error, len(fns)) + var wg sync.WaitGroup + wg.Add(len(fns)) for _, f := range fns { fn := onces.LoadOrStore(f) - wg.Add(1) go func() { defer func() { if v := recover(); v != nil { - mu.Lock() - if err, ok := v.(error); ok { - exit = changeExit(exit, ExitStatus(err)) - } else { - exit = changeExit(exit, 1) + var err error + var ok bool + if err, ok = v.(error); !ok { + err = Fatal(1) + } + select { + case <-ctx.Done(): + case resultCh <- err: } - errs = append(errs, fmt.Sprint(v)) - mu.Unlock() } wg.Done() }() - if err := fn.run(ctx); err != nil { - mu.Lock() - errs = append(errs, fmt.Sprint(err)) - exit = changeExit(exit, ExitStatus(err)) - mu.Unlock() + err := fn.run(ctx) + select { + case <-ctx.Done(): + case resultCh <- err: } }() } wg.Wait() + close(resultCh) + var errs []string + var exit int + for err := range resultCh { + if err != nil { + errs = append(errs, err.Error()) + exit = changeExit(exit, ExitStatus(err)) + } + } if len(errs) > 0 { panic(Fatal(exit, strings.Join(errs, "\n"))) } @@ -191,9 +196,12 @@ func displayName(name string) string { } type onceFun struct { - once *sync.Once + once sync.Once fn Fn err error + // depPanicked references the panic a dependency had previously + // failed with if set + depPanicked interface{} displayName string } @@ -202,10 +210,19 @@ type onceFun struct { // the same error output. func (o *onceFun) run(ctx context.Context) error { o.once.Do(func() { + defer func() { + if v := recover(); v != nil { + o.depPanicked = v + panic(v) + } + }() if Verbose() { logger.Println("Running dependency:", displayName(o.fn.Name())) } o.err = o.fn.Run(ctx) }) + if o.depPanicked != nil { + panic(o.depPanicked) + } return o.err } From 58db674b32eb1e518a6687f7b41b0853d21658a6 Mon Sep 17 00:00:00 2001 From: dmitri Date: Mon, 4 Oct 2021 11:33:36 +0200 Subject: [PATCH 2/2] Add an empty line between functions in the testfile --- mage/testdata/racy_deps/magefile.go | 2 ++ mg/deps.go | 1 + 2 files changed, 3 insertions(+) diff --git a/mage/testdata/racy_deps/magefile.go b/mage/testdata/racy_deps/magefile.go index c8627d14..5958cc76 100644 --- a/mage/testdata/racy_deps/magefile.go +++ b/mage/testdata/racy_deps/magefile.go @@ -1,3 +1,4 @@ +//go:build mage // +build mage package main @@ -26,6 +27,7 @@ func Test3() error { fmt.Println("execute test3") return nil } + func Test4() error { mg.Deps(Test1, Test2, Test3) fmt.Println("execute test4") diff --git a/mg/deps.go b/mg/deps.go index 841429c5..c0b3af21 100644 --- a/mg/deps.go +++ b/mg/deps.go @@ -2,6 +2,7 @@ package mg import ( "context" + "fmt" "log" "os" "reflect"