From 2dd3564da1012e26df1896df0075c12e1e4744d7 Mon Sep 17 00:00:00 2001 From: Andrey Nering Date: Sat, 8 Jul 2017 13:33:55 -0300 Subject: [PATCH] changed cyclic dep detection since interpolation can be used, detection should be a execution time, and not before now, to prevent infinite execution, there's a miximum of 100 calls per task closes #37 --- cyclic.go | 34 ---------------------- cyclic_test.go | 56 ------------------------------------ errors.go | 17 +++++++++-- task.go | 14 ++++++--- task_test.go | 12 ++++++++ testdata/cyclic/Taskfile.yml | 7 +++++ 6 files changed, 44 insertions(+), 96 deletions(-) delete mode 100644 cyclic.go delete mode 100644 cyclic_test.go create mode 100644 testdata/cyclic/Taskfile.yml diff --git a/cyclic.go b/cyclic.go deleted file mode 100644 index 1fb395ae..00000000 --- a/cyclic.go +++ /dev/null @@ -1,34 +0,0 @@ -package task - -// CheckCyclicDep checks if a task tree has any cyclic dependency -func (e *Executor) CheckCyclicDep() error { - visits := make(map[string]struct{}, len(e.Tasks)) - - var checkCyclicDep func(string, *Task) error - checkCyclicDep = func(name string, t *Task) error { - if _, ok := visits[name]; ok { - return ErrCyclicDepDetected - } - visits[name] = struct{}{} - defer delete(visits, name) - - for _, d := range t.Deps { - // FIXME: ignoring by now. should return an error instead? - task, ok := e.Tasks[d.Task] - if !ok { - continue - } - if err := checkCyclicDep(d.Task, task); err != nil { - return err - } - } - return nil - } - - for k, v := range e.Tasks { - if err := checkCyclicDep(k, v); err != nil { - return err - } - } - return nil -} diff --git a/cyclic_test.go b/cyclic_test.go deleted file mode 100644 index 602378cd..00000000 --- a/cyclic_test.go +++ /dev/null @@ -1,56 +0,0 @@ -package task_test - -import ( - "testing" - - "github.com/go-task/task" - - "github.com/stretchr/testify/assert" -) - -func TestCyclicDepCheck(t *testing.T) { - isCyclic := &task.Executor{ - Tasks: task.Tasks{ - "task-a": &task.Task{ - Deps: []*task.Dep{&task.Dep{Task: "task-b"}}, - }, - "task-b": &task.Task{ - Deps: []*task.Dep{&task.Dep{Task: "task-a"}}, - }, - }, - } - - assert.Equal(t, task.ErrCyclicDepDetected, isCyclic.CheckCyclicDep(), "task should be cyclic") - - isNotCyclic := &task.Executor{ - Tasks: task.Tasks{ - "task-a": &task.Task{ - Deps: []*task.Dep{&task.Dep{Task: "task-c"}}, - }, - "task-b": &task.Task{ - Deps: []*task.Dep{&task.Dep{Task: "task-c"}}, - }, - "task-c": &task.Task{}, - }, - } - - assert.NoError(t, isNotCyclic.CheckCyclicDep()) - - inexixtentTask := &task.Executor{ - Tasks: task.Tasks{ - "task-a": &task.Task{ - Deps: []*task.Dep{&task.Dep{Task: "invalid-task"}}, - }, - }, - } - - // FIXME: by now Task should ignore non existent tasks - // in the future we should improve the detection of - // tasks called with interpolation? - // task: - // deps: - // - task: "task{{.VARIABLE}}" - // vars: - // VARIABLE: something - assert.NoError(t, inexixtentTask.CheckCyclicDep()) -} diff --git a/errors.go b/errors.go index 2325d2ed..0ef9a7b5 100644 --- a/errors.go +++ b/errors.go @@ -6,8 +6,6 @@ import ( ) var ( - // ErrCyclicDepDetected is returned when a cyclic dependency was found in the Taskfile - ErrCyclicDepDetected = errors.New("task: cyclic dependency detected") // ErrTaskfileAlreadyExists is returned on creating a Taskfile if one already exists ErrTaskfileAlreadyExists = errors.New("task: A Taskfile already exists") ) @@ -61,3 +59,18 @@ type dynamicVarError struct { func (err *dynamicVarError) Error() string { return fmt.Sprintf(`task: Command "%s" in taskvars file failed: %s`, err.cmd, err.cause) } + +// MaximumTaskCallExceededError is returned when a task is called too +// many times. In this case you probably have a cyclic dependendy or +// infinite loop +type MaximumTaskCallExceededError struct { + task string +} + +func (e *MaximumTaskCallExceededError) Error() string { + return fmt.Sprintf( + `task: maximum task call exceeded (%d) for task "%s": probably an cyclic dep or infinite loop`, + MaximumTaskCall, + e.task, + ) +} diff --git a/task.go b/task.go index b3b8702a..5a5725e1 100644 --- a/task.go +++ b/task.go @@ -9,6 +9,7 @@ import ( "path/filepath" "strings" "sync" + "sync/atomic" "github.com/go-task/task/execext" @@ -18,6 +19,9 @@ import ( const ( // TaskFilePath is the default Taskfile TaskFilePath = "Taskfile" + // MaximumTaskCall is the max number of times a task can be called. + // This exists to prevent infinite loops on cyclic dependencies + MaximumTaskCall = 100 ) // Executor executes a Taskfile @@ -57,14 +61,12 @@ type Task struct { Vars Vars Set string Env Vars + + callCount int32 } // Run runs Task func (e *Executor) Run(args ...string) error { - if err := e.CheckCyclicDep(); err != nil { - return err - } - if e.Stdin == nil { e.Stdin = os.Stdin } @@ -110,6 +112,10 @@ func (e *Executor) RunTask(ctx context.Context, name string, vars Vars) error { return &taskNotFoundError{name} } + if atomic.AddInt32(&t.callCount, 1) >= MaximumTaskCall { + return &MaximumTaskCallExceededError{task: name} + } + if err := e.runDeps(ctx, name, vars); err != nil { return err } diff --git a/task_test.go b/task_test.go index aaa8e6b5..a5277731 100644 --- a/task_test.go +++ b/task_test.go @@ -199,3 +199,15 @@ func TestParams(t *testing.T) { assert.Equal(t, f.content, string(content)) } } + +func TestCyclicDep(t *testing.T) { + const dir = "testdata/cyclic" + + e := task.Executor{ + Dir: dir, + Stdout: ioutil.Discard, + Stderr: ioutil.Discard, + } + assert.NoError(t, e.ReadTaskfile()) + assert.IsType(t, &task.MaximumTaskCallExceededError{}, e.Run("task-1")) +} diff --git a/testdata/cyclic/Taskfile.yml b/testdata/cyclic/Taskfile.yml new file mode 100644 index 00000000..65d741cb --- /dev/null +++ b/testdata/cyclic/Taskfile.yml @@ -0,0 +1,7 @@ +task-1: + deps: + - task: task-2 + +task-2: + deps: + - task: task-1