From 64758de9bb798bfd14dfa8fdafc039b290d30be0 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 1 Oct 2026 11:20:11 +0000 Subject: [PATCH 1/2] refactor: replace optional task interfaces with TaskInfo, drop task wrappers Optional task behavior was spread over five interfaces (TaskName, TaskSteps, TaskWithOptions, TaskWithInitError, TaskWithNotRun) discovered by type assertion, so every wrapper had to forward each of them by hand. Some layers missed one: decorating a futuretask with BuildTask(WithParent(...)) or instancetask.WithParent dropped TaskNotRun, leaving the future unresolved and its waiters blocked forever when the task's stage never ran. - Add TaskInfo (Name, Steps, Options, InitError, NotRun) returned by a single TaskWithInfo.TaskInfo() method, and GetTaskInfo. - Remove wrap.go (WrapTask, UnwrapTask, TaskWithWrapped, BaseOverloadedTask, BaseWrappedTask): use BuildTask(WithParent, WithName) to decorate and the WithHandler task option to customize step calls. - BuildTask merges its parent's TaskInfo (name fallback, options, init error, NotRun) and gains a WithNotRun option. Its computed state is stored atomically, as SetParent runs during the setup step. - instancetask.Build returns *instancetask.Task[T], which embeds the built task instead of forwarding its methods; adds instancetask.WithNotRun. - futuretask.New returns *futuretask.Task[T], built on instancetask with a WithNotRun callback instead of a hand-written forwarding wrapper. - ServiceName is removed: a Service may implement TaskWithInfo directly. - Regression test for the decorated futuretask never resolving. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01CLyyJx24FhyAuthMeMuKPx --- README.md | 20 ++++ build.go | 168 ++++++++++++++++----------- build_test.go | 110 +++++++++++++++++- default.go | 46 +++----- futuretask/task.go | 74 ++++++------ futuretask/task_test.go | 63 +++++++--- health_http/server.go | 8 +- instancetask/build.go | 227 +++++++++++++++++-------------------- instancetask/build_test.go | 41 +++++++ instancetask/provider.go | 5 +- k8sinit/telemetry.go | 2 - manager.go | 8 +- manager_test.go | 9 +- run.go | 6 +- service.go | 29 ++--- task.go | 65 ++++++----- task_helpers.go | 4 +- task_wrapper.go | 6 +- wrap.go | 174 ---------------------------- wrap_test.go | 136 ---------------------- 20 files changed, 542 insertions(+), 659 deletions(-) delete mode 100644 wrap.go delete mode 100644 wrap_test.go diff --git a/README.md b/README.md index ac646fb..cade8c5 100644 --- a/README.md +++ b/README.md @@ -62,6 +62,26 @@ type Task interface { } ``` +A task can optionally describe itself by implementing `TaskWithInfo`. All fields are optional: + +```go +type TaskWithInfo interface { + TaskInfo() TaskInfo +} + +type TaskInfo struct { + Name string // task name + Steps []Step // steps the task implements (nil = all) + Options []TaskInstanceOption // task options set by the task itself + InitError error // initialization error + NotRun func(ctx context.Context, cause error) // called if Run returns without running any step +} +``` + +To decorate an existing task, use `BuildTask` with `WithParent`: steps not set are forwarded to the parent, +and its `TaskInfo` is merged into the built task one. To customize how steps are called, use `WithHandler` when +adding the task. + ## Example ```go diff --git a/build.go b/build.go index cae6d00..6de4d87 100644 --- a/build.go +++ b/build.go @@ -1,6 +1,7 @@ package svcinit import ( + "cmp" "context" "errors" "fmt" @@ -11,10 +12,9 @@ import ( type TaskBuild interface { Task - TaskName - TaskSteps - TaskWithOptions - TaskWithInitError + TaskWithInfo + // SetParent sets the parent task, returning the new initialization error, which is also returned from + // [TaskInfo.InitError]. SetParent(parent Task) error String() string } @@ -22,6 +22,10 @@ type TaskBuild interface { type TaskBuildFunc func(ctx context.Context) error // BuildTask creates a task from callback functions. +// +// It is also the way to decorate an existing task: use [WithParent] to forward all steps not set here to it, +// and [WithName], [WithTaskOptions] and [WithNotRun] to add to its [TaskInfo]. To customize how the steps are +// called, use [WithHandler] in [Manager.AddTask]. func BuildTask(options ...TaskBuildOption) TaskBuild { return newTaskBuild(options...) } @@ -63,6 +67,8 @@ func WithTeardown(f TaskBuildFunc) TaskBuildOption { } // WithParent sets a parent task. Any step not set in the built task will be forwarded to it. +// Its [TaskInfo] is merged into the built task one: its name is used if one is not set, its options are applied +// before the built task ones, its initialization error is returned, and its [TaskInfo.NotRun] is called. func WithParent(parent Task) TaskBuildOption { return func(build *taskBuild) { if parent == nil { @@ -73,29 +79,40 @@ func WithParent(parent Task) TaskBuildOption { } } -// WithTaskOptions sets default task options for the TaskOption interface. +// WithTaskOptions sets default task options for [TaskInfo.Options]. func WithTaskOptions(options ...TaskInstanceOption) TaskBuildOption { return func(build *taskBuild) { build.options = append(build.options, options...) } } +// WithNotRun adds a callback for [TaskInfo.NotRun]. All callbacks are called in order, before the parent one. +func WithNotRun(f func(ctx context.Context, cause error)) TaskBuildOption { + return func(build *taskBuild) { + if f != nil { + build.notRun = append(build.notRun, f) + } + } +} + // internal type taskBuild struct { - stepFunc map[Step]TaskBuildFunc - parent atomic.Pointer[Task] + stepFunc map[Step]TaskBuildFunc + parent atomic.Pointer[Task] + state atomic.Pointer[taskBuildState] + options []TaskInstanceOption + notRun []func(ctx context.Context, cause error) + name string +} + +// taskBuildState is the state computed from the step callbacks and the parent. +type taskBuildState struct { steps []Step - options []TaskInstanceOption initError error - name string } -var _ Task = (*taskBuild)(nil) -var _ TaskName = (*taskBuild)(nil) -var _ TaskSteps = (*taskBuild)(nil) -var _ TaskWithOptions = (*taskBuild)(nil) -var _ TaskWithInitError = (*taskBuild)(nil) +var _ TaskBuild = (*taskBuild)(nil) func newTaskBuild(options ...TaskBuildOption) *taskBuild { ret := &taskBuild{ @@ -104,26 +121,39 @@ func newTaskBuild(options ...TaskBuildOption) *taskBuild { for _, opt := range options { opt(ret) } - ret.initError = ret.init() + ret.init() return ret } -func (t *taskBuild) TaskSteps() []Step { - return t.steps -} - -func (t *taskBuild) TaskOptions() []TaskInstanceOption { - return t.options -} - -func (t *taskBuild) TaskInitError() error { - return t.initError +func (t *taskBuild) TaskInfo() TaskInfo { + var parentInfo TaskInfo + if parent := t.loadParent(); parent != nil { + parentInfo = GetTaskInfo(parent) + } + state := t.state.Load() + ret := TaskInfo{ + Name: cmp.Or(t.name, parentInfo.Name), + Steps: slices.Clone(state.steps), + Options: slices.Concat(parentInfo.Options, t.options), + InitError: state.initError, + } + if len(t.notRun) > 0 || parentInfo.NotRun != nil { + ret.NotRun = func(ctx context.Context, cause error) { + for _, f := range t.notRun { + f(ctx, cause) + } + if parentInfo.NotRun != nil { + parentInfo.NotRun(ctx, cause) + } + } + } + return ret } func (t *taskBuild) Run(ctx context.Context, step Step) error { var parent Task - if p := t.parent.Load(); p != nil && taskHasStep(*p, step) { - parent = *p + if p := t.loadParent(); p != nil && taskHasStep(p, step) { + parent = p } if fn, ok := t.stepFunc[step]; ok { @@ -138,28 +168,34 @@ func (t *taskBuild) Run(ctx context.Context, step Step) error { return newInvalidTaskStep(step) } -func (t *taskBuild) TaskName() string { - if t.name != "" { - return t.name +func (t *taskBuild) String() string { + if tn := GetTaskName(t); tn != "" { + return tn } - if parent := t.parent.Load(); parent != nil { - return GetTaskName(*parent) + return getDefaultTaskDescription(t) +} + +func (t *taskBuild) SetParent(parent Task) error { + if parent == nil { + t.parent.Store(nil) + } else { + t.parent.Store(&parent) } - return "" + return t.init() } -func (t *taskBuild) String() string { - if tn := t.TaskName(); tn != "" { - return tn +func (t *taskBuild) loadParent() Task { + if p := t.parent.Load(); p != nil { + return *p } - return getDefaultTaskDescription(t) + return nil } // hasMissingStep returns whether any step callback is nil, or if there are no steps at all (neither callbacks // nor a parent to forward to). func (t *taskBuild) hasMissingStep() bool { if len(t.stepFunc) == 0 { - return t.parent.Load() == nil + return t.loadParent() == nil } for _, sf := range t.stepFunc { if sf == nil { @@ -169,44 +205,46 @@ func (t *taskBuild) hasMissingStep() bool { return false } -func (t *taskBuild) SetParent(parent Task) error { - if parent == nil { - t.parent.Store(nil) - } else { - t.parent.Store(&parent) - } - t.initError = t.init() - return t.initError -} - -// init computes the task steps and returns any initialization error. +// init computes the task steps and initialization error, and returns the error. func (t *taskBuild) init() error { - var nilTaskErr error + var errs []error if t.hasMissingStep() { - nilTaskErr = ErrNilTask + errs = append(errs, ErrNilTask) } - var duplicatedSteps []Step + // never nil, as it would mean "all steps". + steps := slices.AppendSeq(make([]Step, 0, len(allSteps)), maps.Keys(t.stepFunc)) - t.steps = slices.Collect(maps.Keys(t.stepFunc)) - - if parent := t.parent.Load(); parent != nil { - for _, step := range taskSteps(*parent) { - if !slices.Contains(t.steps, step) { - t.steps = append(t.steps, step) + if parent := t.loadParent(); parent != nil { + var duplicatedSteps []Step + for _, step := range taskSteps(parent) { + if !slices.Contains(steps, step) { + steps = append(steps, step) } else { duplicatedSteps = append(duplicatedSteps, step) } } + if len(duplicatedSteps) > 0 { + errs = append(errs, fmt.Errorf("%w: build task parent already has '%s' step(s)", ErrDuplicateStep, + stringerString(duplicatedSteps))) + } + if err := GetTaskInfo(parent).InitError; err != nil { + errs = append(errs, err) + } } - if len(duplicatedSteps) == 0 { - return nilTaskErr + state := &taskBuildState{ + steps: steps, + initError: buildJoinedErrors(errs), } - dupErr := fmt.Errorf("%w: build task parent already has '%s' step(s)", ErrDuplicateStep, - stringerString(duplicatedSteps)) - if nilTaskErr != nil { - return errors.Join(nilTaskErr, dupErr) + t.state.Store(state) + return state.initError +} + +// buildJoinedErrors returns nil for no errors, the error itself if only one, or [errors.Join] of all of them. +func buildJoinedErrors(errs []error) error { + if len(errs) == 1 { + return errs[0] } - return dupErr + return errors.Join(errs...) } diff --git a/build_test.go b/build_test.go index 08d9c9f..212ecec 100644 --- a/build_test.go +++ b/build_test.go @@ -2,6 +2,7 @@ package svcinit import ( "context" + "errors" "testing" "testing/synctest" "time" @@ -46,8 +47,8 @@ func TestBuildTaskParentOnly(t *testing.T) { WithParent(TimeoutTask(time.Second, WithoutTimeoutTaskError())), WithName("renamed"), ) - assert.NilError(t, task.TaskInitError()) - assert.Equal(t, "renamed", task.TaskName()) + assert.NilError(t, task.TaskInfo().InitError) + assert.Equal(t, "renamed", task.TaskInfo().Name) sinit.AddTask(StageDefault, task) @@ -61,8 +62,109 @@ func TestBuildTaskDuplicateParentStep(t *testing.T) { WithParent(TimeoutTask(time.Second)), WithStart(func(ctx context.Context) error { return nil }), ) - assert.ErrorIs(t, task.TaskInitError(), ErrDuplicateStep) + assert.ErrorIs(t, task.TaskInfo().InitError, ErrDuplicateStep) assert.NilError(t, task.SetParent(nil)) - assert.NilError(t, task.TaskInitError()) + assert.NilError(t, task.TaskInfo().InitError) +} + +func TestBuildTaskDecorate(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + var handlerSteps []Step + decoratedCh := make(chan Task, 1) + + sm, err := New( + WithTaskCallback(TaskCallbackFunc(func(ctx context.Context, task Task, stage string, step Step, + callbackStep CallbackStep, err error) { + // callbacks receive the task passed to AddTask, there is nothing to unwrap. + if GetTaskName(task) != TaskNameTimeout { + assert.Check(t, task == <-decoratedCh) + decoratedCh <- task + } + })), + ) + assert.NilError(t, err) + + inner := &testTaskInfo{ + task: TaskFunc(func(ctx context.Context, step Step) error { + // only finishes because the WithCancelContext(true) option from inner is applied. + <-ctx.Done() + return nil + }), + info: TaskInfo{ + Name: "inner", + Steps: []Step{StepStart}, + Options: []TaskInstanceOption{WithCancelContext(true)}, + }, + } + decorated := BuildTask(WithParent(inner), WithName("outer")) + decoratedCh <- decorated + + info := decorated.TaskInfo() + assert.NilError(t, info.InitError) + assert.Equal(t, "outer", info.Name) + assert.DeepEqual(t, []Step{StepStart}, info.Steps) + assert.Equal(t, 1, len(info.Options)) + + sm.AddTask(StageDefault, decorated, WithHandler(func(ctx context.Context, task Task, step Step) error { + handlerSteps = append(handlerSteps, step) + return task.Run(ctx, step) + })) + sm.AddTask(StageDefault, TimeoutTask(time.Second, WithoutTimeoutTaskError())) + + err = sm.Run(t.Context()) + assert.NilError(t, err) + + assert.DeepEqual(t, []Step{StepStart}, handlerSteps) + }) +} + +func TestBuildTaskParentInitError(t *testing.T) { + sinit, err := New() + assert.NilError(t, err) + + task := BuildTask(WithParent(BuildTask(WithStart(nil)))) + assert.ErrorIs(t, task.TaskInfo().InitError, ErrNilTask) + + sinit.AddTask(StageDefault, task) + + err = sinit.Run(t.Context()) + assert.ErrorIs(t, err, ErrNilTask) +} + +func TestBuildTaskNotRun(t *testing.T) { + var calls []string + errCause := errors.New("cause") + + task := BuildTask( + WithParent(BuildTask( + WithStart(func(ctx context.Context) error { return nil }), + WithNotRun(func(ctx context.Context, cause error) { + assert.Check(t, errors.Is(cause, errCause)) + calls = append(calls, "parent") + }), + )), + WithNotRun(func(ctx context.Context, cause error) { calls = append(calls, "task1") }), + WithNotRun(func(ctx context.Context, cause error) { calls = append(calls, "task2") }), + ) + + notRun := task.TaskInfo().NotRun + assert.Assert(t, notRun != nil) + notRun(t.Context(), errCause) + assert.DeepEqual(t, []string{"task1", "task2", "parent"}, calls) + + assert.Assert(t, BuildTask(WithStart(func(ctx context.Context) error { return nil })).TaskInfo().NotRun == nil) +} + +type testTaskInfo struct { + task Task + info TaskInfo +} + +func (t *testTaskInfo) Run(ctx context.Context, step Step) error { + return t.task.Run(ctx, step) +} + +func (t *testTaskInfo) TaskInfo() TaskInfo { + return t.info } diff --git a/default.go b/default.go index 400f174..adb30e7 100644 --- a/default.go +++ b/default.go @@ -30,9 +30,7 @@ type TaskSignalTask struct { } var _ Task = (*TaskSignalTask)(nil) -var _ TaskName = (*TaskSignalTask)(nil) -var _ TaskSteps = (*TaskSignalTask)(nil) -var _ TaskWithOptions = (*TaskSignalTask)(nil) +var _ TaskWithInfo = (*TaskSignalTask)(nil) func (t *TaskSignalTask) Signals() []os.Signal { return t.signals @@ -48,7 +46,7 @@ func (t *TaskSignalTask) Run(ctx context.Context, step Step) error { case sig := <-c: return SignalError{Signal: sig} case <-ctx.Done(): - // safe: TaskOptions() below always sets WithCancelContext(true), so ctx here is taskDoneCtx-derived, + // safe: TaskInfo() below always sets WithCancelContext(true), so ctx here is taskDoneCtx-derived, // whose CancelCauseFunc is only ever called by the Manager itself. return context.Cause(ctx) } @@ -57,20 +55,16 @@ func (t *TaskSignalTask) Run(ctx context.Context, step Step) error { return nil } -func (t *TaskSignalTask) TaskSteps() []Step { - return []Step{StepStart} -} - -func (t *TaskSignalTask) TaskOptions() []TaskInstanceOption { - return []TaskInstanceOption{ - WithCancelContext(true), +func (t *TaskSignalTask) TaskInfo() TaskInfo { + return TaskInfo{ + Name: TaskNameSignals, + Steps: []Step{StepStart}, + Options: []TaskInstanceOption{ + WithCancelContext(true), + }, } } -func (t *TaskSignalTask) TaskName() string { - return TaskNameSignals -} - func (t *TaskSignalTask) String() string { return fmt.Sprintf("Signals %v", t.signals) } @@ -82,9 +76,7 @@ type TaskTimeoutTask struct { } var _ Task = (*TaskTimeoutTask)(nil) -var _ TaskName = (*TaskTimeoutTask)(nil) -var _ TaskSteps = (*TaskTimeoutTask)(nil) -var _ TaskWithOptions = (*TaskTimeoutTask)(nil) +var _ TaskWithInfo = (*TaskTimeoutTask)(nil) func (t *TaskTimeoutTask) Timeout() time.Duration { return t.timeout @@ -110,20 +102,16 @@ func (t *TaskTimeoutTask) Run(ctx context.Context, step Step) error { return nil } -func (t *TaskTimeoutTask) TaskSteps() []Step { - return []Step{StepStart} -} - -func (t *TaskTimeoutTask) TaskOptions() []TaskInstanceOption { - return []TaskInstanceOption{ - WithCancelContext(true), +func (t *TaskTimeoutTask) TaskInfo() TaskInfo { + return TaskInfo{ + Name: TaskNameTimeout, + Steps: []Step{StepStart}, + Options: []TaskInstanceOption{ + WithCancelContext(true), + }, } } -func (t *TaskTimeoutTask) TaskName() string { - return TaskNameTimeout -} - func (t *TaskTimeoutTask) String() string { return fmt.Sprintf("Timeout %v", t.timeout) } diff --git a/futuretask/task.go b/futuretask/task.go index 3795309..3705478 100644 --- a/futuretask/task.go +++ b/futuretask/task.go @@ -3,6 +3,7 @@ package futuretask import ( "context" "fmt" + "slices" "github.com/rrgmc/svcinit/v3" "github.com/rrgmc/svcinit/v3/instancetask" @@ -11,63 +12,52 @@ import ( // New creates a task that resolves a [svcinit.Future] from the result of setupFunc's "setup" step. // setupFunc must not be nil: unlike [instancetask.Build], there would be no data to resolve the future // with. -func New[T any](setupFunc instancetask.BuildSetupFunc[T], - options ...instancetask.BuildOption[T]) svcinit.TaskFuture[T] { - dr := svcinit.NewFuture[T]() +// If the task never runs, for example because a previous stage failed, the future is resolved with +// [svcinit.ErrTaskNotRun], so waiters don't block forever. +func New[T any](setupFunc instancetask.BuildSetupFunc[T], options ...instancetask.BuildOption[T]) *Task[T] { + future := svcinit.NewFuture[T]() if setupFunc == nil { setupFunc = func(context.Context) (T, error) { var empty T return empty, svcinit.ErrNilTask } } - return &taskFuture[T]{ - BaseOverloadedTask: &svcinit.BaseOverloadedTask[svcinit.TaskWithData[T]]{Task: instancetask.Build[T](func(ctx context.Context) (T, error) { + return &Task[T]{ + instanceTask: instancetask.Build[T](func(ctx context.Context) (T, error) { data, err := setupFunc(ctx) if err != nil { - dr.ResolveError(err) + future.ResolveError(err) var empty T return empty, err } - dr.Resolve(data) + future.Resolve(data) return data, nil - }, options...)}, - future: dr, + }, append(slices.Clip(options), instancetask.WithNotRun[T](func(_ context.Context, cause error) { + select { + case <-future.Done(): + return // resolving twice panics. + default: + } + if cause != nil { + future.ResolveError(fmt.Errorf("%w: %w", svcinit.ErrTaskNotRun, cause)) + } else { + future.ResolveError(svcinit.ErrTaskNotRun) + } + }))...), + future: future, } } -// internal - -type taskFuture[T any] struct { - *svcinit.BaseOverloadedTask[svcinit.TaskWithData[T]] - future svcinit.FutureResolver[T] +// Task is a task created by [New], which is also the [svcinit.Future] resolved by its "setup" step. +type Task[T any] struct { + *instanceTask[T] + future svcinit.Future[T] } -var _ svcinit.Future[int] = (*taskFuture[int])(nil) -var _ svcinit.Task = (*taskFuture[int])(nil) -var _ svcinit.TaskSteps = (*taskFuture[int])(nil) -var _ svcinit.TaskWithOptions = (*taskFuture[int])(nil) -var _ svcinit.TaskWithNotRun = (*taskFuture[int])(nil) +var _ svcinit.TaskFuture[int] = (*Task[int])(nil) +var _ svcinit.TaskWithInfo = (*Task[int])(nil) -func (t *taskFuture[T]) Run(ctx context.Context, step svcinit.Step) error { - return t.Task.Run(ctx, step) -} - -// TaskNotRun resolves the future with [svcinit.ErrTaskNotRun], so waiters don't block forever when the -// "setup" step never runs, for example because a previous stage failed. -func (t *taskFuture[T]) TaskNotRun(_ context.Context, cause error) { - select { - case <-t.future.Done(): - return - default: - } - if cause != nil { - t.future.ResolveError(fmt.Errorf("%w: %w", svcinit.ErrTaskNotRun, cause)) - } else { - t.future.ResolveError(svcinit.ErrTaskNotRun) - } -} - -func (t *taskFuture[T]) Value(options ...svcinit.FutureValueOption) (T, error) { +func (t *Task[T]) Value(options ...svcinit.FutureValueOption) (T, error) { ret, err := t.future.Value(options...) if err != nil { return ret, fmt.Errorf("error resolving task data: %w", err) @@ -75,6 +65,10 @@ func (t *taskFuture[T]) Value(options ...svcinit.FutureValueOption) (T, error) { return ret, nil } -func (t *taskFuture[T]) Done() <-chan struct{} { +func (t *Task[T]) Done() <-chan struct{} { return t.future.Done() } + +// internal + +type instanceTask[T any] = instancetask.Task[T] diff --git a/futuretask/task_test.go b/futuretask/task_test.go index 9075914..9af643b 100644 --- a/futuretask/task_test.go +++ b/futuretask/task_test.go @@ -159,18 +159,53 @@ func TestNotRunInitError(t *testing.T) { assert.ErrorIs(t, err, svcinit.ErrTaskNotRun) } -func TestNotRunWrapped(t *testing.T) { - sinit, err := svcinit.New() - assert.NilError(t, err) - - future := New[int](func(ctx context.Context) (int, error) { - return 10, nil - }) - sinit.AddTask(svcinit.StageDefault, svcinit.WrapTask(future)) - sinit.AddInitError(errors.New("init error")) - - _ = sinit.Run(t.Context()) - - _, err = future.Value(svcinit.WithoutFutureWait()) - assert.ErrorIs(t, err, svcinit.ErrTaskNotRun) +// TestNotRunDecorated is a regression test: decorating a future task with svcinit.BuildTask used to drop its +// "not run" notification, so the future was never resolved and its waiters blocked forever. +func TestNotRunDecorated(t *testing.T) { + for _, tc := range []struct { + name string + decorate func(task svcinit.Task) svcinit.Task + }{ + { + name: "BuildTask parent", + decorate: func(task svcinit.Task) svcinit.Task { + return svcinit.BuildTask(svcinit.WithParent(task), svcinit.WithName("decorated")) + }, + }, + { + name: "nested BuildTask parent", + decorate: func(task svcinit.Task) svcinit.Task { + return svcinit.BuildTask(svcinit.WithParent(svcinit.BuildTask(svcinit.WithParent(task)))) + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + errSetup := errors.New("setup error") + + sinit, err := svcinit.New(svcinit.WithStages("s1", "s2")) + assert.NilError(t, err) + + future := New[int](func(ctx context.Context) (int, error) { + return 10, nil + }) + sinit.AddTask("s1", svcinit.BuildTask( + svcinit.WithSetup(func(ctx context.Context) error { + return errSetup + }), + svcinit.WithStart(func(ctx context.Context) error { + return nil + }), + )) + sinit.AddTask("s2", tc.decorate(future)) + + err = sinit.Run(t.Context()) + assert.ErrorIs(t, err, errSetup) + + _, err = future.Value(svcinit.WithoutFutureWait()) + assert.ErrorIs(t, err, svcinit.ErrTaskNotRun) + assert.ErrorIs(t, err, errSetup) + }) + }) + } } diff --git a/health_http/server.go b/health_http/server.go index 016acd0..f666db9 100644 --- a/health_http/server.go +++ b/health_http/server.go @@ -35,7 +35,7 @@ func NewServer(options ...ServerOption) *Server { } var _ svcinit.Task = (*Server)(nil) -var _ svcinit.TaskName = (*Server)(nil) +var _ svcinit.TaskWithInfo = (*Server)(nil) var _ svcinit.HealthHandler = (*Server)(nil) func (h *Server) ServiceStarted(ctx context.Context) { @@ -78,8 +78,8 @@ func (h *Server) Run(ctx context.Context, step svcinit.Step) (err error) { return nil } -func (h *Server) TaskName() string { - return h.taskName +func (h *Server) TaskInfo() svcinit.TaskInfo { + return svcinit.TaskInfo{Name: h.taskName} } // options @@ -105,7 +105,7 @@ func WithServerProvider(provider func(ctx context.Context, address string) (*htt } } -// WithServerTaskName sets the [svcinit.TaskName] returned for this task. The default is "health". +// WithServerTaskName sets the [svcinit.TaskInfo.Name] returned for this task. The default is "health". func WithServerTaskName(name string) ServerOption { return &optionImpl{ serverOpt: func(server *Server) { diff --git a/instancetask/build.go b/instancetask/build.go index 1f34a7f..03bf49d 100644 --- a/instancetask/build.go +++ b/instancetask/build.go @@ -14,17 +14,97 @@ type BuildSetupFunc[T any] func(ctx context.Context) (T, error) // Build creates a task from callback functions, where some data is created in the "setup" step and passed // to all other steps. -func Build[T any](setupFunc BuildSetupFunc[T], options ...BuildOption[T]) svcinit.TaskWithData[T] { - return newTaskBuild[T](setupFunc, options...) +func Build[T any](setupFunc BuildSetupFunc[T], options ...BuildOption[T]) *Task[T] { + var optns buildOptions[T] + for _, opt := range options { + opt(&optns) + } + + ret := &Task[T]{ + setupFunc: setupFunc, + parentFromSetup: optns.parentFromSetup, + } + + tbOptions := optns.tbOptions + if setupFunc != nil { + tbOptions = append(tbOptions, svcinit.WithSetup(ret.runSetup)) + } else { + tbOptions = append(tbOptions, svcinit.WithSetup(nil)) + } + for step, stepFn := range optns.stepFunc { + if stepFn == nil { + tbOptions = append(tbOptions, svcinit.WithStep(step, nil)) + continue + } + tbOptions = append(tbOptions, svcinit.WithStep(step, func(ctx context.Context) error { + data, err := ret.TaskData() + if err != nil { + return err + } + return stepFn(ctx, data) + })) + } + + ret.build = svcinit.BuildTask(tbOptions...) + ret.buildTask = ret.build + return ret +} + +// Task is a task created by [Build]. +type Task[T any] struct { + buildTask + build svcinit.TaskBuild + data atomic.Pointer[T] + setupFunc BuildSetupFunc[T] + parentFromSetup bool +} + +var _ svcinit.TaskWithData[int] = (*Task[int])(nil) +var _ svcinit.TaskWithInfo = (*Task[int])(nil) + +// buildTask is the part of [svcinit.TaskBuild] that [Task] exposes, by embedding. +type buildTask interface { + svcinit.Task + svcinit.TaskWithInfo + fmt.Stringer +} + +// TaskData returns the data returned from the "setup" step, or [svcinit.ErrNotInitialized] if it didn't run yet. +func (t *Task[T]) TaskData() (T, error) { + data := t.data.Load() + if data == nil { + var empty T + return empty, fmt.Errorf("%w: data not initialized", svcinit.ErrNotInitialized) + } + return *data, nil } -type BuildOption[T any] func(*taskBuild[T]) +func (t *Task[T]) runSetup(ctx context.Context) error { + if t.data.Load() != nil { + return svcinit.ErrAlreadyInitialized + } + data, err := t.setupFunc(ctx) + if err != nil { + return err + } + t.data.Store(&data) + if t.parentFromSetup { + tt, ok := any(data).(svcinit.Task) + if !ok { + return fmt.Errorf("%w: data returned from setup doesn't implement Task to be set as parent", svcinit.ErrInitialization) + } + return t.build.SetParent(tt) + } + return nil +} + +// options + +type BuildOption[T any] func(*buildOptions[T]) // WithName sets the task name. func WithName[T any](name string) BuildOption[T] { - return func(build *taskBuild[T]) { - build.tbOptions = append(build.tbOptions, svcinit.WithName(name)) - } + return withBuildOption[T](svcinit.WithName(name)) } // WithStart sets a callback for the "start" step. @@ -43,143 +123,48 @@ func WithTeardown[T any](f BuildFunc[T]) BuildOption[T] { } // WithParent sets a parent task. Any step not set in the built task will be forwarded to it. +// See [svcinit.WithParent]. func WithParent[T any](parent svcinit.Task) BuildOption[T] { - return func(build *taskBuild[T]) { - build.tbOptions = append(build.tbOptions, svcinit.WithParent(parent)) - } + return withBuildOption[T](svcinit.WithParent(parent)) } // WithParentFromSetup sets a parent task from the result of the "setup" task. // If this value doesn't implement Task, an initialization error will be issued. func WithParentFromSetup[T any](parentFromSetup bool) BuildOption[T] { - return func(build *taskBuild[T]) { - build.parentFromSetup = parentFromSetup + return func(o *buildOptions[T]) { + o.parentFromSetup = parentFromSetup } } -// WithTaskOptions sets default task options for the TaskOption interface. +// WithTaskOptions sets default task options for [svcinit.TaskInfo.Options]. func WithTaskOptions[T any](options ...svcinit.TaskInstanceOption) BuildOption[T] { - return func(build *taskBuild[T]) { - build.tbOptions = append(build.tbOptions, svcinit.WithTaskOptions(options...)) - } + return withBuildOption[T](svcinit.WithTaskOptions(options...)) +} + +// WithNotRun adds a callback for [svcinit.TaskInfo.NotRun]. See [svcinit.WithNotRun]. +func WithNotRun[T any](f func(ctx context.Context, cause error)) BuildOption[T] { + return withBuildOption[T](svcinit.WithNotRun(f)) } // internal -type taskBuild[T any] struct { - tb svcinit.TaskBuild - data atomic.Pointer[T] - setupFunc BuildSetupFunc[T] +type buildOptions[T any] struct { stepFunc map[svcinit.Step]BuildFunc[T] parentFromSetup bool tbOptions []svcinit.TaskBuildOption } -var _ svcinit.TaskWithData[int] = (*taskBuild[int])(nil) -var _ svcinit.TaskName = (*taskBuild[int])(nil) -var _ svcinit.TaskSteps = (*taskBuild[int])(nil) -var _ svcinit.TaskWithOptions = (*taskBuild[int])(nil) -var _ svcinit.TaskWithInitError = (*taskBuild[int])(nil) - -func newTaskBuild[T any](setupFunc BuildSetupFunc[T], options ...BuildOption[T]) svcinit.TaskWithData[T] { - ret := &taskBuild[T]{ - setupFunc: setupFunc, - stepFunc: make(map[svcinit.Step]BuildFunc[T]), - } - for _, opt := range options { - opt(ret) +func withBuildOption[T any](option svcinit.TaskBuildOption) BuildOption[T] { + return func(o *buildOptions[T]) { + o.tbOptions = append(o.tbOptions, option) } - - if ret.setupFunc != nil { - ret.tbOptions = append(ret.tbOptions, - svcinit.WithSetup(func(ctx context.Context) error { - return ret.runSetup(ctx) - }), - ) - } else { - ret.tbOptions = append(ret.tbOptions, - svcinit.WithSetup(nil), - ) - } - - for step, stepFn := range ret.stepFunc { - if stepFn != nil { - ret.tbOptions = append(ret.tbOptions, - svcinit.WithStep(step, func(ctx context.Context) error { - data := ret.data.Load() - if data == nil { - return fmt.Errorf("%w: data not initialized", svcinit.ErrNotInitialized) - } - return stepFn(ctx, *data) - })) - } else { - ret.tbOptions = append(ret.tbOptions, - svcinit.WithStep(step, nil)) - } - } - - ret.tb = svcinit.BuildTask(ret.tbOptions...) - - return ret -} - -func (t *taskBuild[T]) TaskData() (T, error) { - if data := t.data.Load(); data == nil { - var empty T - return empty, fmt.Errorf("%w: data not initialized", svcinit.ErrNotInitialized) - } else { - return *data, nil - } -} - -func (t *taskBuild[T]) TaskSteps() []svcinit.Step { - return t.tb.TaskSteps() -} - -func (t *taskBuild[T]) TaskOptions() []svcinit.TaskInstanceOption { - return t.tb.TaskOptions() -} - -func (t *taskBuild[T]) TaskInitError() error { - return t.tb.TaskInitError() -} - -func (t *taskBuild[T]) runSetup(ctx context.Context) error { - if t.data.Load() != nil { - return svcinit.ErrAlreadyInitialized - } - data, err := t.setupFunc(ctx) - if err != nil { - return err - } - t.data.Store(&data) - if t.parentFromSetup { - if tt, ok := any(data).(svcinit.Task); ok { - err := t.tb.SetParent(tt) - if err != nil { - return err - } - } else { - return fmt.Errorf("%w: data returned from setup doesn't implement Task to be set as parent", svcinit.ErrInitialization) - } - } - return nil -} - -func (t *taskBuild[T]) Run(ctx context.Context, step svcinit.Step) error { - return t.tb.Run(ctx, step) -} - -func (t *taskBuild[T]) TaskName() string { - return t.tb.TaskName() -} - -func (t *taskBuild[T]) String() string { - return t.tb.String() } func withStep[T any](step svcinit.Step, f BuildFunc[T]) BuildOption[T] { - return func(build *taskBuild[T]) { - build.stepFunc[step] = f + return func(o *buildOptions[T]) { + if o.stepFunc == nil { + o.stepFunc = make(map[svcinit.Step]BuildFunc[T]) + } + o.stepFunc[step] = f } } diff --git a/instancetask/build_test.go b/instancetask/build_test.go index 86f67ca..a144680 100644 --- a/instancetask/build_test.go +++ b/instancetask/build_test.go @@ -86,3 +86,44 @@ func TestBuildTask(t *testing.T) { assert.DeepEqual(t, []string{"start", "stop"}, items.Get(), cmpopts.SortSlices(cmp.Less[string])) }) } + +func TestBuildTaskInfoFromParent(t *testing.T) { + var calls []string + parent := svcinit.BuildTask( + svcinit.WithName("parent"), + svcinit.WithStart(func(ctx context.Context) error { return nil }), + svcinit.WithTaskOptions(svcinit.WithCancelContext(true)), + svcinit.WithNotRun(func(ctx context.Context, cause error) { calls = append(calls, "parent") }), + ) + + task := Build[int](func(ctx context.Context) (int, error) { return 1, nil }, + WithParent[int](parent), + WithNotRun[int](func(ctx context.Context, cause error) { calls = append(calls, "task") }), + ) + + info := svcinit.GetTaskInfo(task) + assert.NilError(t, info.InitError) + assert.Equal(t, "parent", info.Name) + assert.DeepEqual(t, []svcinit.Step{svcinit.StepSetup, svcinit.StepStart}, info.Steps, + cmpopts.SortSlices(cmp.Less[svcinit.Step])) + assert.Equal(t, 1, len(info.Options)) + assert.Assert(t, info.NotRun != nil) + info.NotRun(t.Context(), nil) + assert.DeepEqual(t, []string{"task", "parent"}, calls) +} + +func TestProviderInitErrorFromSetup(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + sinit, err := svcinit.New() + assert.NilError(t, err) + + sinit.AddTask(svcinit.StageDefault, Provider(func(ctx context.Context) (svcinit.Task, error) { + // a provided task with an initialization error must fail the setup step. + return svcinit.BuildTask(svcinit.WithStart(nil)), nil + })) + sinit.AddTask(svcinit.StageDefault, svcinit.TimeoutTask(time.Second)) + + err = sinit.Run(t.Context()) + assert.ErrorIs(t, err, svcinit.ErrNilTask) + }) +} diff --git a/instancetask/provider.go b/instancetask/provider.go index b26f975..14ea1ca 100644 --- a/instancetask/provider.go +++ b/instancetask/provider.go @@ -2,6 +2,7 @@ package instancetask import ( "context" + "slices" "github.com/rrgmc/svcinit/v3" ) @@ -9,6 +10,6 @@ import ( // Provider builds a task from the task returned by a callback. // Any step not set in the built task will be forwarded to it. func Provider(provider func(ctx context.Context) (svcinit.Task, error), - options ...BuildOption[svcinit.Task]) svcinit.TaskWithData[svcinit.Task] { - return Build[svcinit.Task](provider, append(options, WithParentFromSetup[svcinit.Task](true))...) + options ...BuildOption[svcinit.Task]) *Task[svcinit.Task] { + return Build[svcinit.Task](provider, append(slices.Clip(options), WithParentFromSetup[svcinit.Task](true))...) } diff --git a/k8sinit/telemetry.go b/k8sinit/telemetry.go index 071ab6a..8864109 100644 --- a/k8sinit/telemetry.go +++ b/k8sinit/telemetry.go @@ -84,5 +84,3 @@ type noopTelemetryHandler struct { func (h *noopTelemetryHandler) FlushTelemetry(context.Context) error { return nil } - -type privateBaseOverloadedTask[T svcinit.Task] = svcinit.BaseOverloadedTask[T] diff --git a/manager.go b/manager.go index 032edc7..3998fca 100644 --- a/manager.go +++ b/manager.go @@ -75,11 +75,9 @@ func (m *Manager) AddTask(stage string, task Task, options ...TaskOption) { m.addInitError(ErrNilTask) return } - if te, ok := task.(TaskWithInitError); ok { - if err := te.TaskInitError(); err != nil { - m.addInitError(err) - return - } + if err := GetTaskInfo(task).InitError; err != nil { + m.addInitError(err) + return } if !slices.Contains(m.stages, stage) { m.addInitError(newInvalidStage(stage)) diff --git a/manager_test.go b/manager_test.go index 5d9ca76..b083f86 100644 --- a/manager_test.go +++ b/manager_test.go @@ -994,7 +994,7 @@ type testTask struct { task Task } -var _ TaskSteps = (*testTask)(nil) +var _ TaskWithInfo = (*testTask)(nil) func newTestTask(taskNo int, task Task) *testTask { return &testTask{ @@ -1007,11 +1007,8 @@ func (t *testTask) TaskNo() int { return t.taskNo } -func (t *testTask) TaskSteps() []Step { - if tt, ok := t.task.(TaskSteps); ok { - return tt.TaskSteps() - } - return allSteps +func (t *testTask) TaskInfo() TaskInfo { + return GetTaskInfo(t.task) } func (t *testTask) Run(ctx context.Context, step Step) error { diff --git a/run.go b/run.go index 0701699..97414f3 100644 --- a/run.go +++ b/run.go @@ -514,17 +514,17 @@ func (m *Manager) runStageStep(ctx, taskDoneCtx context.Context, loggerStage *sl return int(taskCount.Load()) } -// notifyTasksNotRun calls [TaskWithNotRun.TaskNotRun] for all tasks of the passed stages, which must be stages +// notifyTasksNotRun calls [TaskInfo.NotRun] for all tasks of the passed stages, which must be stages // that will never run any step. func (m *Manager) notifyTasksNotRun(ctx context.Context, stages []string, cause error) { ctx = context.WithoutCancel(ctx) for _, stage := range stages { for tw := range m.tasks.stageTasks(stage) { - if tn, ok := tw.task.(TaskWithNotRun); ok { + if notRun := GetTaskInfo(tw.task).NotRun; notRun != nil { m.logger.Log(ctx, slog2.LevelTrace, "notifying task not run", "stage", stage, "task", GetTaskDescription(tw.task)) - tn.TaskNotRun(ctx, cause) + notRun(ctx, cause) } } } diff --git a/service.go b/service.go index 1645b7d..a21c675 100644 --- a/service.go +++ b/service.go @@ -2,6 +2,7 @@ package svcinit import ( "context" + "slices" ) // Service is an abstraction of Task as an interface, for convenience. @@ -21,11 +22,6 @@ type ServiceWithTeardown interface { Teardown(ctx context.Context) error } -// ServiceName allows services to have a name. -type ServiceName interface { - TaskName -} - // ServiceTask allows getting the source Service of the Task. type ServiceTask interface { Task @@ -33,6 +29,8 @@ type ServiceTask interface { } // ServiceAsTask wraps a Service into a Task. +// The Service may implement [TaskWithInfo]; its info is used for the task, except for [TaskInfo.Steps], which is +// computed from the Service interfaces it implements. func ServiceAsTask(service Service) ServiceTask { t := &serviceTask{ service: service, @@ -53,8 +51,7 @@ type serviceTask struct { } var _ ServiceTask = (*serviceTask)(nil) -var _ TaskName = (*serviceTask)(nil) -var _ TaskSteps = (*serviceTask)(nil) +var _ TaskWithInfo = (*serviceTask)(nil) func (t *serviceTask) Run(ctx context.Context, step Step) error { switch step { @@ -75,23 +72,21 @@ func (t *serviceTask) Run(ctx context.Context, step Step) error { return nil } -func (t *serviceTask) TaskSteps() []Step { - return t.steps +func (t *serviceTask) TaskInfo() TaskInfo { + var info TaskInfo + if ti, ok := t.service.(TaskWithInfo); ok { + info = ti.TaskInfo() + } + info.Steps = slices.Clone(t.steps) + return info } func (t *serviceTask) Service() Service { return t.service } -func (t *serviceTask) TaskName() string { - if ts, ok := t.service.(ServiceName); ok { - return ts.TaskName() - } - return "" -} - func (t *serviceTask) String() string { - if tn := t.TaskName(); tn != "" { + if tn := GetTaskName(t); tn != "" { return tn } return getDefaultTaskDescription(t.service) diff --git a/task.go b/task.go index 5dfa302..513ace5 100644 --- a/task.go +++ b/task.go @@ -31,27 +31,48 @@ func (t TaskFunc) String() string { // the steps out of order or multiple times. type TaskHandler func(ctx context.Context, task Task, step Step) error -// TaskSteps returns the steps that the task implements. They will be the only ones called. -type TaskSteps interface { - TaskSteps() []Step -} - -// TaskName allows tasks to have a name. -type TaskName interface { - TaskName() string +// TaskInfo describes the optional metadata and behavior of a task. All fields are optional, and the zero value +// is the default for a task. +// It is the single extension point of a task: a task decorating another one should merge the inner task's +// TaskInfo instead of forwarding a set of interfaces. [BuildTask] with [WithParent] does this. +type TaskInfo struct { + // Name is the task name. + Name string + // Steps are the steps that the task implements. They will be the only ones called. + // If nil, all steps are called. + Steps []Step + // Options are task options set by the task itself. They have priority over options set via [Manager.AddTask]. + Options []TaskInstanceOption + // InitError is a task initialization error. If not nil, [Manager.AddTask] won't add the task, and + // [Manager.Run] will return the error. + InitError error + // NotRun is called when [Manager.Run] returns without having run any of the task steps, for example because + // a setup step of a previous stage failed. cause is the error returned from Run, and may be nil. + // It can be used to release anything waiting on the task, like an unresolved [Future]. + NotRun func(ctx context.Context, cause error) +} + +// TaskWithInfo allows a task to describe its optional metadata and behavior. +type TaskWithInfo interface { + TaskInfo() TaskInfo +} + +// GetTaskInfo returns the task info, or the zero value if the task don't implement [TaskWithInfo]. +func GetTaskInfo(task Task) TaskInfo { + if ti, ok := task.(TaskWithInfo); ok { + return ti.TaskInfo() + } + return TaskInfo{} } -// DefaultTaskSteps returns the default value for [TaskSteps.TaskSteps], which is the list of all steps. +// DefaultTaskSteps returns the list of all steps, which is the default for [TaskInfo.Steps]. func DefaultTaskSteps() []Step { return slices.Clone(allSteps) } // GetTaskName gets the name of task, or blank if it don't have one. func GetTaskName(task Task) string { - if ts, ok := task.(TaskName); ok { - return ts.TaskName() - } - return "" + return GetTaskInfo(task).Name } // GetTaskDescription returns the task description, be it the String method, a task name, or its variable type. @@ -65,24 +86,6 @@ func GetTaskDescription(task Task) string { return getDefaultTaskDescription(task) } -// TaskWithOptions allows the task to set some of the task options. They have priority over options set via -// [Manager.AddTask]. -type TaskWithOptions interface { - TaskOptions() []TaskInstanceOption -} - -// TaskWithInitError allows a task to report an initialization error. The error might be nil. -type TaskWithInitError interface { - TaskInitError() error -} - -// TaskWithNotRun allows a task to be notified when [Manager.Run] returns without having run any of its steps, -// for example because a setup step of a previous stage failed. cause is the error returned from Run, and may be nil. -// It can be used to release anything waiting on the task, like an unresolved [Future]. -type TaskWithNotRun interface { - TaskNotRun(ctx context.Context, cause error) -} - // WithCancelContext sets whether to automatically cancel the task start step context when the first task finishes. // The default is false, meaning that the stop step should handle to stop the task. func WithCancelContext(cancelContext bool) TaskAndInstanceOption { diff --git a/task_helpers.go b/task_helpers.go index 41fd85d..659b135 100644 --- a/task_helpers.go +++ b/task_helpers.go @@ -9,8 +9,8 @@ import ( var allSteps = []Step{StepSetup, StepStart, StepStop, StepTeardown} // order matters func taskSteps(task Task) []Step { - if ts, ok := task.(TaskSteps); ok { - return ts.TaskSteps() + if steps := GetTaskInfo(task).Steps; steps != nil { + return steps } return allSteps } diff --git a/task_wrapper.go b/task_wrapper.go index 7b5a7b0..81dadc1 100644 --- a/task_wrapper.go +++ b/task_wrapper.go @@ -24,10 +24,8 @@ func newTaskWrapper(task Task, options ...TaskOption) *taskWrapper { for _, option := range options { option.applyTaskOpt(&ret.options) } - if to, ok := task.(TaskWithOptions); ok { - for _, option := range to.TaskOptions() { - option.applyTaskInstanceOpt(&ret.options) - } + for _, option := range GetTaskInfo(task).Options { + option.applyTaskInstanceOpt(&ret.options) } return ret } diff --git a/wrap.go b/wrap.go deleted file mode 100644 index 7e08f96..0000000 --- a/wrap.go +++ /dev/null @@ -1,174 +0,0 @@ -package svcinit - -import ( - "context" -) - -// TaskWithWrapped is a task which was wrapped from another Task. -type TaskWithWrapped interface { - Task - WrappedTask() Task -} - -// WrapTask wraps a task in a TaskWithWrapped, allowing the handler to be customized. -func WrapTask(task Task, options ...WrapTaskOption) TaskWithWrapped { - return newWrappedTask(task, options...) -} - -type WrapTaskOption func(task *wrappedTask) - -// WithWrapTaskHandler sets an optional handler for the task. -func WithWrapTaskHandler(handler TaskHandler) WrapTaskOption { - return func(task *wrappedTask) { - task.handler = handler - } -} - -// WithWrapName sets the task name. -func WithWrapName(name string) WrapTaskOption { - return func(task *wrappedTask) { - task.name = name - } -} - -// UnwrapTask unwraps TaskWithWrapped from tasks. -func UnwrapTask(task Task) Task { - if task == nil { - return nil - } - for { - if tc, ok := task.(TaskWithWrapped); ok && tc.WrappedTask() != nil { - task = tc.WrappedTask() - } else { - return task - } - } -} - -// BaseOverloadedTask wraps a task and forwards all task interface methods. -// It doesn't implement TaskWithWrapped. -type BaseOverloadedTask[T Task] struct { - Task T -} - -var _ TaskName = (*BaseOverloadedTask[Task])(nil) -var _ TaskSteps = (*BaseOverloadedTask[Task])(nil) -var _ TaskWithOptions = (*BaseOverloadedTask[Task])(nil) -var _ TaskWithInitError = (*BaseOverloadedTask[Task])(nil) -var _ TaskWithNotRun = (*BaseOverloadedTask[Task])(nil) - -func (t *BaseOverloadedTask[T]) TaskOptions() []TaskInstanceOption { - if tt, ok := any(t.Task).(TaskWithOptions); ok { - return tt.TaskOptions() - } - return nil -} - -func (t *BaseOverloadedTask[T]) TaskSteps() []Step { - if tt, ok := any(t.Task).(TaskSteps); ok { - return tt.TaskSteps() - } - return DefaultTaskSteps() -} - -func (t *BaseOverloadedTask[T]) TaskInitError() error { - if tt, ok := any(t.Task).(TaskWithInitError); ok { - return tt.TaskInitError() - } - return nil -} - -func (t *BaseOverloadedTask[T]) TaskNotRun(ctx context.Context, cause error) { - if tt, ok := any(t.Task).(TaskWithNotRun); ok { - tt.TaskNotRun(ctx, cause) - } -} - -func (t *BaseOverloadedTask[T]) TaskName() string { - return GetTaskName(t.Task) -} - -func (t *BaseOverloadedTask[T]) String() string { - if tn := t.TaskName(); tn != "" { - return tn - } - return getDefaultTaskDescription(t.Task) -} - -// BaseWrappedTask wraps a task and forwards all task interface methods. -// It implements TaskWithWrapped. -type BaseWrappedTask[T Task] struct { - *BaseOverloadedTask[T] -} - -func NewBaseWrappedTask[T Task](task T) *BaseWrappedTask[T] { - return &BaseWrappedTask[T]{ - &BaseOverloadedTask[T]{ - Task: task, - }, - } -} - -var _ TaskName = (*BaseWrappedTask[Task])(nil) -var _ TaskSteps = (*BaseWrappedTask[Task])(nil) -var _ TaskWithOptions = (*BaseWrappedTask[Task])(nil) -var _ TaskWithInitError = (*BaseWrappedTask[Task])(nil) -var _ TaskWithWrapped = (*BaseWrappedTask[Task])(nil) - -func (t *BaseWrappedTask[T]) Run(ctx context.Context, step Step) error { - return t.Task.Run(ctx, step) -} - -func (t *BaseWrappedTask[T]) WrappedTask() Task { - return t.Task -} - -// internal - -type wrappedTask struct { - *baseWrappedTaskPrivate[Task] - handler TaskHandler - name string -} - -var _ Task = (*wrappedTask)(nil) -var _ TaskName = (*wrappedTask)(nil) -var _ TaskSteps = (*wrappedTask)(nil) -var _ TaskWithOptions = (*wrappedTask)(nil) -var _ TaskWithInitError = (*wrappedTask)(nil) -var _ TaskWithWrapped = (*wrappedTask)(nil) - -func newWrappedTask(task Task, options ...WrapTaskOption) *wrappedTask { - ret := &wrappedTask{ - baseWrappedTaskPrivate: NewBaseWrappedTask[Task](checkNilTask(task)), - } - for _, option := range options { - option(ret) - } - return ret -} - -func (t *wrappedTask) Run(ctx context.Context, step Step) error { - if t.handler != nil { - return t.handler(ctx, t.Task, step) - } - return t.Task.Run(ctx, step) -} - -func (t *wrappedTask) TaskName() string { - if t.name != "" { - return t.name - } - return GetTaskName(t.Task) -} - -func (t *wrappedTask) String() string { - if tn := t.TaskName(); tn != "" { - return tn - } - return getDefaultTaskDescription(t.Task) -} - -type baseOverloadedTaskPrivate[T Task] = BaseOverloadedTask[T] - -type baseWrappedTaskPrivate[T Task] = BaseWrappedTask[T] diff --git a/wrap_test.go b/wrap_test.go deleted file mode 100644 index 4ace4f2..0000000 --- a/wrap_test.go +++ /dev/null @@ -1,136 +0,0 @@ -package svcinit - -import ( - "cmp" - "context" - "testing" - "testing/synctest" - - cmp2 "github.com/google/go-cmp/cmp" - "github.com/google/go-cmp/cmp/cmpopts" - "github.com/rrgmc/svcinit/v3/internal/testutils" - "gotest.tools/v3/assert" -) - -func TestWrapTask(t *testing.T) { - synctest.Test(t, func(t *testing.T) { - started := &testutils.TestList[string]{} - stopped := &testutils.TestList[string]{} - - sm, err := New( - WithTaskCallback(TaskCallbackFunc(func(ctx context.Context, task Task, stage string, step Step, callbackStep CallbackStep, err error) { - _, ok := getTestTaskNo(task) - assert.Check(t, !ok) - - tn, ok := getTestTaskNo(UnwrapTask(task)) - assert.Check(t, ok) - assert.Check(t, cmp2.Equal(3, tn)) - })), - ) - assert.NilError(t, err) - - task1 := newTestTask(3, BuildTask( - WithStart(func(ctx context.Context) error { - started.Add("task3") - return nil - }), - WithStop(func(ctx context.Context) error { - stopped.Add("task3") - return nil - }), - )) - - sm.AddTask(StageDefault, WrapTask(task1)) - - err = sm.Run(t.Context()) - assert.NilError(t, err) - - assert.DeepEqual(t, []string{"task3"}, started.Get(), cmpopts.SortSlices(cmp.Less[string])) - assert.DeepEqual(t, []string{"task3"}, stopped.Get(), cmpopts.SortSlices(cmp.Less[string])) - }) -} - -func TestWrapTaskImplements(t *testing.T) { - synctest.Test(t, func(t *testing.T) { - started := &testutils.TestList[string]{} - - sm, err := New( - WithTaskCallback(TaskCallbackFunc(func(ctx context.Context, task Task, stage string, step Step, callbackStep CallbackStep, err error) { - _, ok := task.(*testTaskComplete) - assert.Check(t, !ok) - - _, ok = UnwrapTask(task).(*testTaskComplete) - if !assert.Check(t, ok) { - return - } - - ts, ok := task.(TaskSteps) - if !assert.Check(t, ok) { - return - } - assert.Check(t, cmp2.Equal([]Step{StepStart}, ts.TaskSteps())) - - to, ok := task.(TaskWithOptions) - if !assert.Check(t, ok) { - return - } - assert.Check(t, 1 == len(to.TaskOptions())) - })), - ) - assert.NilError(t, err) - - task1 := &testTaskComplete{ - task: BuildTask( - WithStart(func(ctx context.Context) error { - started.Add("task3") - return nil - }), - ), - steps: []Step{ - StepStart, - }, - options: []TaskInstanceOption{ - WithCancelContext(true), - }, - } - - sm.AddTask(StageDefault, WrapTask(task1)) - - err = sm.Run(t.Context()) - assert.NilError(t, err) - - assert.DeepEqual(t, []string{"task3"}, started.Get(), cmpopts.SortSlices(cmp.Less[string])) - }) -} - -type testTaskComplete struct { - task Task - steps []Step - options []TaskInstanceOption -} - -var _ Task = (*testTaskComplete)(nil) -var _ TaskSteps = (*testTaskComplete)(nil) -var _ TaskWithOptions = (*testTaskComplete)(nil) - -func (t *testTaskComplete) Run(ctx context.Context, step Step) error { - return t.task.Run(ctx, step) -} - -func (t *testTaskComplete) TaskSteps() []Step { - return t.steps -} - -func (t *testTaskComplete) TaskOptions() []TaskInstanceOption { - return t.options -} - -func TestWrapTaskInitError(t *testing.T) { - sinit, err := New() - assert.NilError(t, err) - - sinit.AddTask(StageDefault, WrapTask(BuildTask(WithStart(nil)))) - - err = sinit.Run(t.Context()) - assert.ErrorIs(t, err, ErrNilTask) -} From 639e47a50e1bb8b015695e062daec6d1a6e596d5 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 1 Oct 2026 11:28:53 +0000 Subject: [PATCH 2/2] fix: apply task options from TaskInfo changes after setup Task instance options were applied once, when the task was added, so options of a task only known after the "setup" step were ignored, like the ones from the task returned by an instancetask.Provider callback (WithCancelContext, WithStartStepManager). The task wrapper now keeps only the Manager.AddTask options, and computes the effective options from the task's current TaskInfo when they are used. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01CLyyJx24FhyAuthMeMuKPx --- instancetask/build_test.go | 51 ++++++++++++++++++++++++++++++++++++++ run.go | 9 ++++--- task_wrapper.go | 28 +++++++++++++-------- 3 files changed, 74 insertions(+), 14 deletions(-) diff --git a/instancetask/build_test.go b/instancetask/build_test.go index a144680..8e68dad 100644 --- a/instancetask/build_test.go +++ b/instancetask/build_test.go @@ -3,6 +3,7 @@ package instancetask import ( "cmp" "context" + "sync/atomic" "testing" "testing/synctest" "time" @@ -127,3 +128,53 @@ func TestProviderInitErrorFromSetup(t *testing.T) { assert.ErrorIs(t, err, svcinit.ErrNilTask) }) } + +// TestProviderTaskOptions is a regression test: the options of the task returned by the Provider callback used to +// be ignored, as task options were computed when the task was added, before the "setup" step set the parent. +func TestProviderTaskOptions(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + var startCancelled, ssmCanCancel atomic.Bool + + sinit, err := svcinit.New( + // if the options are ignored, the start steps are never cancelled: give up waiting instead of blocking. + svcinit.WithEnforceShutdownTimeout(true), + ) + assert.NilError(t, err) + + sinit.AddTask(svcinit.StageDefault, Provider(func(ctx context.Context) (svcinit.Task, error) { + return svcinit.BuildTask( + svcinit.WithStart(func(ctx context.Context) error { + // only cancelled by the Manager because of the WithCancelContext(true) option. + <-ctx.Done() + startCancelled.Store(true) + return nil + }), + svcinit.WithTaskOptions(svcinit.WithCancelContext(true)), + ), nil + })) + + sinit.AddTask(svcinit.StageDefault, Provider(func(ctx context.Context) (svcinit.Task, error) { + return svcinit.BuildTask( + svcinit.WithStart(func(ctx context.Context) error { + <-ctx.Done() + return nil + }), + svcinit.WithStop(func(ctx context.Context) error { + // only available because of the WithStartStepManager() option. + ssm := svcinit.StartStepManagerFromContext(ctx) + ssmCanCancel.Store(ssm.CanContextCancel()) + ssm.ContextCancel(context.Canceled) + return nil + }), + svcinit.WithTaskOptions(svcinit.WithStartStepManager()), + ), nil + })) + + sinit.AddTask(svcinit.StageDefault, svcinit.TimeoutTask(time.Second, svcinit.WithoutTimeoutTaskError())) + + err = sinit.Run(t.Context()) + assert.NilError(t, err) + assert.Check(t, startCancelled.Load()) + assert.Check(t, ssmCanCancel.Load()) + }) +} diff --git a/run.go b/run.go index 97414f3..8467a9f 100644 --- a/run.go +++ b/run.go @@ -429,15 +429,16 @@ func (m *Manager) runStageStep(ctx, taskDoneCtx context.Context, loggerStage *sl defer wg.Done() taskCtx := taskDoneCtx var taskCancelOnStop context.CancelCauseFunc + taskOpts := tw.currentOptions() switch step { case StepStart: logAttrs = append(logAttrs, - slog.Bool("cancelContext", tw.options.cancelContext), + slog.Bool("cancelContext", taskOpts.cancelContext), ) - if !tw.options.cancelContext { + if !taskOpts.cancelContext { taskCtx = ctx // don't cancel context automatically using the global task done context } - if tw.options.startStepManager { + if taskOpts.startStepManager { logAttrs = append(logAttrs, slog.Bool("ssm", true)) // create cancellable context for the start step. tw.mu.Lock() @@ -449,7 +450,7 @@ func (m *Manager) runStageStep(ctx, taskDoneCtx context.Context, loggerStage *sl tw.mu.Unlock() } case StepStop: - if tw.options.startStepManager { + if taskOpts.startStepManager { logAttrs = append(logAttrs, slog.Bool("ssm", true)) startStepMan := &startStepManager{ logger: loggerTask, diff --git a/task_wrapper.go b/task_wrapper.go index 81dadc1..5125126 100644 --- a/task_wrapper.go +++ b/task_wrapper.go @@ -8,8 +8,8 @@ import ( ) type taskWrapper struct { - task Task - options taskOptions + task Task + addOptions taskOptions // options passed to [Manager.AddTask]. Use [taskWrapper.currentOptions] to read. mu sync.Mutex executeSteps []Step @@ -22,14 +22,22 @@ func newTaskWrapper(task Task, options ...TaskOption) *taskWrapper { task: task, } for _, option := range options { - option.applyTaskOpt(&ret.options) - } - for _, option := range GetTaskInfo(task).Options { - option.applyTaskInstanceOpt(&ret.options) + option.applyTaskOpt(&ret.addOptions) } return ret } +// currentOptions returns the task options: the [Manager.AddTask] ones, with the task [TaskInfo.Options] applied +// over them. It is computed on each call, as the task info may change after the setup step, for example when +// its parent is set from the setup step result like in [instancetask.Provider]. +func (t *taskWrapper) currentOptions() taskOptions { + options := t.addOptions + for _, option := range GetTaskInfo(t.task).Options { + option.applyTaskInstanceOpt(&options) + } + return options +} + // checkStartStep checks if the step can be started for this task. // Returns any logic error found. func (t *taskWrapper) checkStartStep(step Step) (bool, error) { @@ -61,8 +69,8 @@ func (t *taskWrapper) run(ctx context.Context, stage string, step Step, callback // setup is only added if no run error, so start and stop are not called in that case. t.addStepDone(step) } - if t.options.handler != nil { - err = t.options.handler(ctx, t.task, step) + if handler := t.currentOptions().handler; handler != nil { + err = handler(ctx, t.task, step) } else { err = t.task.Run(ctx, step) } @@ -78,7 +86,7 @@ func (t *taskWrapper) run(ctx context.Context, stage string, step Step, callback func (t *taskWrapper) runCallbacks(ctx context.Context, stage string, step Step, callbackStep CallbackStep, err error, callbacks []TaskCallback) { - for _, cbs := range [2][]TaskCallback{callbacks, t.options.callbacks} { + for _, cbs := range [2][]TaskCallback{callbacks, t.currentOptions().callbacks} { for _, callback := range cbs { callback.Callback(ctx, t.task, stage, step, callbackStep, err) } @@ -101,7 +109,7 @@ func (t *taskWrapper) internalCanStartStep(step Step) bool { } // if SSM is enabled, a stop step must exist for it to work. If it don't exist, create an internal one. if step == StepStop { - return t.options.startStepManager + return t.currentOptions().startStepManager } return false }