diff --git a/bus/bus.go b/bus/bus.go index 00267dcb..dd55b867 100644 --- a/bus/bus.go +++ b/bus/bus.go @@ -8,6 +8,26 @@ import ( "github.com/aperturerobotics/util/broadcast" ) +// ControllerCloseError reports a Controller.Close failure. +// +// AddController reports this error through its callback. ExecuteController and +// an AddController attachment failure may include it in their returned error. +// Callers can distinguish close failures from execution failures with +// errors.As. +type ControllerCloseError struct { + Err error +} + +// Error returns the controller close failure. +func (e *ControllerCloseError) Error() string { + return "controller close failed: " + e.Err.Error() +} + +// Unwrap returns the underlying close failure. +func (e *ControllerCloseError) Unwrap() error { + return e.Err +} + // Bus manages running controllers. It has an attached directive controller, // which is used to build declarative state requests between controllers. type Bus interface { @@ -20,24 +40,30 @@ type Bus interface { // controllers are added or removed from the bus. GetControllersBroadcast() *broadcast.Broadcast - // AddController adds a controller to the bus and calls Execute(). - // The controller will exit if ctx is canceled. - // Returns a release function for the controller reference. - // The controller will receive directive callbacks until removed. - // Any fatal error in the controller is written to cb. - // If the controller is released, cb will be called with nil. - // cb can be nil + // AddController attaches a controller and calls Execute asynchronously. + // The controller receives directive callbacks until it is released, + // Execute returns an error, or Execute panics. + // + // The returned release function cancels the Execute context, detaches + // directive handling, waits for Execute to return, calls Close once, calls + // cb, and then returns. Repeated release calls are idempotent. + // + // cb receives the execution error, a ControllerCloseError, both errors + // joined together, or nil when release completes without either error. + // cb can be nil. AddController(ctx context.Context, ctrl controller.Controller, cb func(exitErr error)) (func(), error) - // ExecuteController adds a controller to the bus and calls Execute(). - // The controller will exit if ctx is canceled. - // Any fatal error in the controller is returned. - // The controller will receive directive callbacks. - // If this function returns nil, call RemoveController to remove the controller. + // ExecuteController attaches a controller and calls Execute synchronously. + // A nil Execute return leaves the controller attached until + // RemoveController is called. An Execute error or panic detaches the + // controller, calls Close once, and is returned, joined with any + // ControllerCloseError. + // RemoveController may be called concurrently to cancel and finalize the + // synchronous execution. ExecuteController(context.Context, controller.Controller) error - // RemoveController removes the controller from the bus. - // The controller will no longer receive callbacks. - // Note: this might not cancel the Execute() context automatically. + // RemoveController synchronously releases one attached instance of the + // controller. It cancels that instance's Execute context, detaches + // directive handling, waits for Execute to return, and calls Close once. RemoveController(controller.Controller) } diff --git a/bus/inmem/controller.go b/bus/inmem/controller.go index 12e9fab4..7f49c2c2 100644 --- a/bus/inmem/controller.go +++ b/bus/inmem/controller.go @@ -1,11 +1,88 @@ package inmem -import "github.com/aperturerobotics/controllerbus/controller" +import ( + "context" + "errors" + "sync" + "time" -// attachedCtrl contains an attached controller + "github.com/aperturerobotics/controllerbus/bus" + "github.com/aperturerobotics/controllerbus/controller" +) + +const releaseWarningInterval = 30 * time.Second + +// attachedCtrl contains the lifecycle of one attached controller instance. type attachedCtrl struct { - // ctrl is the controller ctrl controller.Controller - // rel releases the controller - rel func() + rel func() + + cancel context.CancelFunc + executeDone chan struct{} + executeErr error + callback func(error) + + finalizeOnce sync.Once + finalErr error +} + +// newAttachedCtrl constructs an attached controller lifecycle. +func newAttachedCtrl( + ctrl controller.Controller, + rel func(), + cancel context.CancelFunc, + callback func(error), +) *attachedCtrl { + return &attachedCtrl{ + ctrl: ctrl, + rel: rel, + cancel: cancel, + executeDone: make(chan struct{}), + callback: callback, + } +} + +// finishExecution records the result before publishing execution completion. +func (c *attachedCtrl) finishExecution(err error) { + c.executeErr = err + close(c.executeDone) +} + +// finalize cancels, detaches, waits, closes, and reports exactly once. +func (c *attachedCtrl) finalize(b *Bus) error { + c.finalizeOnce.Do(func() { + c.cancel() + b.detachController(c) + + ticker := time.NewTicker(releaseWarningInterval) + defer ticker.Stop() + waitForExecute: + for { + select { + case <-c.executeDone: + break waitForExecute + case <-ticker.C: + b.le.WithField("controller", c.ctrl).Warn("waiting for controller Execute to return") + } + } + + c.finalErr = joinControllerErrors(c.executeErr, c.ctrl.Close()) + if c.callback != nil { + c.callback(c.finalErr) + } + }) + return c.finalErr +} + +// joinControllerErrors preserves the execution error and identifies a close +// failure for errors.As. +func joinControllerErrors(executeErr, closeErr error) error { + if closeErr == nil { + return executeErr + } + wrappedCloseErr := &bus.ControllerCloseError{Err: closeErr} + if executeErr == nil { + return wrappedCloseErr + } + return errors.Join(executeErr, wrappedCloseErr) } diff --git a/bus/inmem/detach_test.go b/bus/inmem/detach_test.go new file mode 100644 index 00000000..05fa8422 --- /dev/null +++ b/bus/inmem/detach_test.go @@ -0,0 +1,61 @@ +package inmem + +import "testing" + +func TestDetachControllerIsIdempotent(t *testing.T) { + releaseCalls := 0 + target := &attachedCtrl{ + rel: func() { releaseCalls++ }, + } + retained := &attachedCtrl{} + b := NewBus(nil) + b.controllers = []*attachedCtrl{target, retained} + + locked := b.bcast.Lock() + firstBroadcast := locked.WaitCh() + locked.Unlock() + + b.detachController(target) + + if got := releaseCalls; got != 1 { + t.Fatalf("release calls after first detach = %d, want 1", got) + } + if got := len(b.controllers); got != 1 { + t.Fatalf("controller count after first detach = %d, want 1", got) + } + if b.controllers[0] != retained { + t.Fatal("retained controller changed after first detach") + } + + broadcastCalls := 0 + select { + case <-firstBroadcast: + broadcastCalls++ + default: + t.Fatal("first detach did not broadcast") + } + + locked = b.bcast.Lock() + secondBroadcast := locked.WaitCh() + locked.Unlock() + + b.detachController(target) + + if got := releaseCalls; got != 1 { + t.Fatalf("release calls after second detach = %d, want 1", got) + } + if got := len(b.controllers); got != 1 { + t.Fatalf("controller count after second detach = %d, want 1", got) + } + if b.controllers[0] != retained { + t.Fatal("retained controller changed after second detach") + } + select { + case <-secondBroadcast: + broadcastCalls++ + default: + } + if broadcastCalls != 1 { + t.Fatalf("broadcast calls after two detaches = %d, want 1", broadcastCalls) + } +} diff --git a/bus/inmem/inmem.go b/bus/inmem/inmem.go index ce27b335..eab0acfd 100644 --- a/bus/inmem/inmem.go +++ b/bus/inmem/inmem.go @@ -10,6 +10,7 @@ import ( "github.com/aperturerobotics/controllerbus/directive" "github.com/aperturerobotics/util/broadcast" "github.com/pkg/errors" + "github.com/sirupsen/logrus" ) // Bus is an in-memory controller bus. @@ -17,6 +18,8 @@ type Bus struct { // Controller is the directive controller. directive.Controller + // le reports controller lifecycle diagnostics. + le *logrus.Entry // bcast is signaled when controllers are added or removed. bcast broadcast.Broadcast // mtx guards below fields @@ -27,7 +30,16 @@ type Bus struct { // NewBus constructs a new in-memory Bus with a directive controller. func NewBus(dc directive.Controller) *Bus { - return &Bus{Controller: dc} + return NewBusWithLogger(dc, nil) +} + +// NewBusWithLogger constructs a new in-memory Bus with a directive controller +// and lifecycle logger. +func NewBusWithLogger(dc directive.Controller, le *logrus.Entry) *Bus { + if le == nil { + le = logrus.NewEntry(logrus.New()) + } + return &Bus{Controller: dc, le: le} } // GetControllers returns a list of all currently active controllers. @@ -47,117 +59,137 @@ func (b *Bus) GetControllersBroadcast() *broadcast.Broadcast { return &b.bcast } -// AddController adds a controller to the bus and calls Execute(). -// Returns a release function for the controller instance. -// Any fatal error in the controller is written to the callback. -// The controller will receive directive callbacks until removed. -// cb can be nil +// AddController attaches a controller and calls Execute asynchronously. +// Its release function cancels, detaches, waits for Execute, closes once, and +// reports the lifecycle result through cb before returning. func (b *Bus) AddController(ctx context.Context, ctrl controller.Controller, cb func(exitErr error)) (func(), error) { subCtx, subCtxCancel := context.WithCancel(ctx) - relFunc := func() { + attached, err := b.attachController(ctrl, subCtxCancel, cb) + if err != nil { subCtxCancel() - b.removeController(ctrl) + return nil, joinControllerErrors(err, ctrl.Close()) } - if err := b.addController(ctrl); err != nil { - subCtxCancel() - _ = ctrl.Close() - return nil, err + + go b.executeAttached(subCtx, attached) + return func() { + attached.finalize(b) + }, nil +} + +// executeAttached runs Execute and finalizes terminal failures. +func (b *Bus) executeAttached(ctx context.Context, attached *attachedCtrl) { + err := b.executeController(ctx, attached.ctrl) + attached.finishExecution(err) + if err != nil { + attached.finalize(b) } - go func() { - var err error - defer func() { - b.handleControllerPanic(&err) - if err != nil { - subCtxCancel() - b.removeController(ctrl) - if cb != nil { - cb(err) - } - } - }() - err = ctrl.Execute(subCtx) - }() - return relFunc, nil } -// handleControllerPanic handles recover for a paniced controller. +// executeController calls Execute and converts a panic into an error. +func (b *Bus) executeController(ctx context.Context, ctrl controller.Controller) (err error) { + defer b.handleControllerPanic(&err) + return ctrl.Execute(ctx) +} + +// handleControllerPanic handles recovery for a controller panic. func (b *Bus) handleControllerPanic(outErr *error) { if rerr := recover(); rerr != nil { debug.PrintStack() e, eOk := rerr.(error) if eOk { if outErr != nil { - *outErr = errors.Wrap(e, "controller paniced") + *outErr = errors.Wrap(e, "controller panicked") } } else if outErr != nil && *outErr == nil { - *outErr = errors.New("controller paniced") + *outErr = errors.New("controller panicked") } } } -// ExecuteController adds a controller to the bus and calls Execute(). -// Any fatal error in the controller is returned. -// The controller will receive directive callbacks. -// If the controller returns nil, call RemoveController to remove the controller. -func (b *Bus) ExecuteController(ctx context.Context, c controller.Controller) (err error) { - if err := b.addController(c); err != nil { - return err +// ExecuteController attaches a controller and calls Execute synchronously. +// A nil return leaves the controller attached for RemoveController. A terminal +// failure finalizes the attachment before returning. +func (b *Bus) ExecuteController(ctx context.Context, c controller.Controller) error { + subCtx, subCtxCancel := context.WithCancel(ctx) + attached, err := b.attachController(c, subCtxCancel, nil) + if err != nil { + subCtxCancel() + return joinControllerErrors(err, c.Close()) } - defer func() { - b.handleControllerPanic(&err) - if err != nil { - b.removeController(c) - } - }() - - return c.Execute(ctx) + err = b.executeController(subCtx, c) + attached.finishExecution(err) + if err != nil { + return attached.finalize(b) + } + return nil } -// RemoveController removes the controller from the bus. +// RemoveController synchronously finalizes one attached controller instance. func (b *Bus) RemoveController(c controller.Controller) { - b.removeController(c) + attached := b.findController(c) + if attached != nil { + attached.finalize(b) + } } -// addController adds a controller to the bus -func (b *Bus) addController(c controller.Controller) error { - b.mtx.Lock() +// attachController registers and records one attached controller instance. +func (b *Bus) attachController( + c controller.Controller, + cancel context.CancelFunc, + cb func(error), +) (*attachedCtrl, error) { + // AddHandler may call HandleDirective, so it must run outside b.mtx. rel, err := b.AddHandler(c) - if err == nil { - b.controllers = append(b.controllers, &attachedCtrl{ - ctrl: c, - rel: rel, - }) + if err != nil { + return nil, err } + attached := newAttachedCtrl(c, rel, cancel, cb) + + b.mtx.Lock() + b.controllers = append(b.controllers, attached) b.mtx.Unlock() - if err == nil { - b.bcast.HoldLock(func(broadcast func(), getWaitCh func() <-chan struct{}) { - broadcast() - }) + b.bcast.HoldLock(func(broadcast func(), getWaitCh func() <-chan struct{}) { + broadcast() + }) + return attached, nil +} + +// findController returns one attached instance of c. +func (b *Bus) findController(c controller.Controller) *attachedCtrl { + b.mtx.Lock() + defer b.mtx.Unlock() + for _, attached := range b.controllers { + if attached.ctrl == c { + return attached + } } - return err + return nil } -// removeController removes a controller from the bus -func (b *Bus) removeController(c controller.Controller) { +// detachController removes an exact attached instance and then releases its +// directive handler without holding b.mtx. +func (b *Bus) detachController(attached *attachedCtrl) { var removed bool b.mtx.Lock() - for i, ci := range b.controllers { - if ci.ctrl == c { + for i, candidate := range b.controllers { + if candidate == attached { b.controllers[i] = b.controllers[len(b.controllers)-1] b.controllers[len(b.controllers)-1] = nil b.controllers = b.controllers[:len(b.controllers)-1] - ci.rel() removed = true break } } b.mtx.Unlock() - if removed { - b.bcast.HoldLock(func(broadcast func(), getWaitCh func() <-chan struct{}) { - broadcast() - }) + + if !removed { + return } + attached.rel() + b.bcast.HoldLock(func(broadcast func(), getWaitCh func() <-chan struct{}) { + broadcast() + }) } // _ is a type assertion diff --git a/bus/inmem/lifecycle_test.go b/bus/inmem/lifecycle_test.go new file mode 100644 index 00000000..bcb138ab --- /dev/null +++ b/bus/inmem/lifecycle_test.go @@ -0,0 +1,775 @@ +package inmem + +import ( + "context" + stderrors "errors" + "strings" + "sync" + "sync/atomic" + "testing" + "testing/synctest" + "time" + + "github.com/aperturerobotics/controllerbus/bus" + cbcontroller "github.com/aperturerobotics/controllerbus/controller" + "github.com/aperturerobotics/controllerbus/directive" + directivecontroller "github.com/aperturerobotics/controllerbus/directive/controller" + "github.com/sirupsen/logrus" + logrustest "github.com/sirupsen/logrus/hooks/test" +) + +type lifecycleController struct { + executeFn func(context.Context) error + closeFn func() error + handleFn func(context.Context, directive.Instance) ([]directive.Resolver, error) + closeCalls atomic.Int32 + handleCalls atomic.Int32 +} + +func (c *lifecycleController) Execute(ctx context.Context) error { + if c.executeFn == nil { + return nil + } + return c.executeFn(ctx) +} + +func (c *lifecycleController) Close() error { + c.closeCalls.Add(1) + if c.closeFn == nil { + return nil + } + return c.closeFn() +} + +func (c *lifecycleController) HandleDirective(ctx context.Context, di directive.Instance) ([]directive.Resolver, error) { + c.handleCalls.Add(1) + if c.handleFn == nil { + return nil, nil + } + return c.handleFn(ctx, di) +} + +func (c *lifecycleController) GetControllerInfo() *cbcontroller.Info { return nil } + +type trackingDirectiveController struct { + directive.Controller + addErr error + beforeAdd func(directive.Handler) + detachCalls atomic.Int32 + detached chan struct{} + detachedOnce sync.Once +} + +func (c *trackingDirectiveController) AddHandler(handler directive.Handler) (func(), error) { + if c.beforeAdd != nil { + c.beforeAdd(handler) + } + if c.addErr != nil { + return nil, c.addErr + } + + var release func() + if c.Controller != nil { + var err error + release, err = c.Controller.AddHandler(handler) + if err != nil { + return nil, err + } + } else { + release = func() {} + } + return func() { + c.detachCalls.Add(1) + c.detachedOnce.Do(func() { close(c.detached) }) + release() + }, nil +} + +func newTrackingBus() (*Bus, *trackingDirectiveController) { + logger := logrus.New() + dc := directivecontroller.NewController(context.Background(), logrus.NewEntry(logger)) + tracking := &trackingDirectiveController{ + Controller: dc, + detached: make(chan struct{}), + } + return NewBus(tracking), tracking +} + +func TestAddControllerReleaseWaitsForExecute(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + b, tracking := newTrackingBus() + executionStarted := make(chan struct{}) + allowExecuteReturn := make(chan struct{}) + releaseReturned := make(chan struct{}) + var executeReturned atomic.Bool + var closeBeforeExecute atomic.Bool + var callbackCalls atomic.Int32 + var callbackErr error + + ctrl := &lifecycleController{ + executeFn: func(ctx context.Context) error { + close(executionStarted) + <-ctx.Done() + <-allowExecuteReturn + executeReturned.Store(true) + return nil + }, + closeFn: func() error { + if !executeReturned.Load() { + closeBeforeExecute.Store(true) + } + // A Close implementation may call back into the bus. + _ = b.GetControllers() + return nil + }, + } + release, err := b.AddController(t.Context(), ctrl, func(err error) { + callbackErr = err + callbackCalls.Add(1) + }) + if err != nil { + t.Fatalf("AddController failed: %v", err) + } + <-executionStarted + + go func() { + release() + close(releaseReturned) + }() + <-tracking.detached + synctest.Wait() + + releasedEarly := false + select { + case <-releaseReturned: + releasedEarly = true + default: + } + closeCallsBeforeReturn := ctrl.closeCalls.Load() + controllersAfterDetach := len(b.GetControllers()) + + close(allowExecuteReturn) + synctest.Wait() + if releasedEarly { + t.Fatal("release returned before Execute returned") + } + if closeCallsBeforeReturn != 0 { + t.Fatalf("Close called before Execute returned: calls = %d", closeCallsBeforeReturn) + } + if controllersAfterDetach != 0 { + t.Fatalf("detached controller count = %d, want 0", controllersAfterDetach) + } + if !executeReturned.Load() { + t.Fatal("Execute return marker was not set") + } + if closeBeforeExecute.Load() { + t.Fatal("Close observed Execute before its return marker") + } + select { + case <-releaseReturned: + default: + t.Fatal("release did not return after Execute returned") + } + if got := ctrl.closeCalls.Load(); got != 1 { + t.Fatalf("Close calls = %d, want 1", got) + } + if got := tracking.detachCalls.Load(); got != 1 { + t.Fatalf("handler detach calls = %d, want 1", got) + } + if got := callbackCalls.Load(); got != 1 { + t.Fatalf("callback calls = %d, want 1", got) + } + if callbackErr != nil { + t.Fatalf("callback error = %v, want nil", callbackErr) + } + }) +} + +func TestAddControllerReleaseRacesWithExecuteError(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + b, tracking := newTrackingBus() + executeErr := stderrors.New("terminal execution failure") + closeErr := stderrors.New("terminal close failure") + executionStarted := make(chan struct{}) + raceGate := make(chan struct{}) + releaseReady := make(chan struct{}) + releaseReturned := make(chan struct{}) + callbackResults := make(chan error, 2) + var callbackCalls atomic.Int32 + + ctrl := &lifecycleController{ + executeFn: func(context.Context) error { + close(executionStarted) + <-raceGate + return executeErr + }, + closeFn: func() error { return closeErr }, + } + release, err := b.AddController(t.Context(), ctrl, func(err error) { + callbackCalls.Add(1) + callbackResults <- err + }) + if err != nil { + t.Fatalf("AddController failed: %v", err) + } + <-executionStarted + go func() { + close(releaseReady) + <-raceGate + release() + close(releaseReturned) + }() + <-releaseReady + + close(raceGate) + synctest.Wait() + select { + case <-releaseReturned: + default: + t.Fatal("racing release did not return") + } + if got := ctrl.closeCalls.Load(); got != 1 { + t.Fatalf("Close calls after racing finalizers = %d, want 1", got) + } + if got := tracking.detachCalls.Load(); got != 1 { + t.Fatalf("handler detach calls after racing finalizers = %d, want 1", got) + } + if got := callbackCalls.Load(); got != 1 { + t.Fatalf("callback calls after racing finalizers = %d, want 1", got) + } + select { + case got := <-callbackResults: + if !stderrors.Is(got, executeErr) { + t.Fatalf("callback error = %v, want execution error", got) + } + var typedCloseErr *bus.ControllerCloseError + if !stderrors.As(got, &typedCloseErr) || !stderrors.Is(typedCloseErr, closeErr) { + t.Fatalf("callback error = %v, want typed close error", got) + } + default: + t.Fatal("callback did not receive execution error") + } + + release() + if got := ctrl.closeCalls.Load(); got != 1 { + t.Fatalf("Close calls after repeated release = %d, want 1", got) + } + }) +} + +func TestAddControllerReleaseWaitsForContextIgnoringExecute(t *testing.T) { + const observerTimeout = 75 * time.Millisecond + const completionTimeout = time.Second + + b, tracking := newTrackingBus() + executionStarted := make(chan struct{}) + allowExecuteReturn := make(chan struct{}) + releaseReturned := make(chan struct{}) + var executeReturned atomic.Bool + var closeBeforeExecute atomic.Bool + + ctrl := &lifecycleController{ + executeFn: func(context.Context) error { + close(executionStarted) + <-allowExecuteReturn + executeReturned.Store(true) + return nil + }, + closeFn: func() error { + if !executeReturned.Load() { + closeBeforeExecute.Store(true) + } + return nil + }, + } + release, err := b.AddController(t.Context(), ctrl, nil) + if err != nil { + t.Fatalf("AddController failed: %v", err) + } + <-executionStarted + go func() { + release() + close(releaseReturned) + }() + + select { + case <-tracking.detached: + case <-time.After(completionTimeout): + t.Fatal("handler was not detached after release request") + } + select { + case <-releaseReturned: + t.Fatal("release returned while Execute ignored cancellation") + case <-time.After(observerTimeout): + } + if got := ctrl.closeCalls.Load(); got != 0 { + t.Fatalf("Close calls while Execute was blocked = %d, want 0", got) + } + + close(allowExecuteReturn) + select { + case <-releaseReturned: + case <-time.After(completionTimeout): + t.Fatal("release did not return after Execute gate opened") + } + if !executeReturned.Load() { + t.Fatal("Execute return marker was not set") + } + if closeBeforeExecute.Load() { + t.Fatal("Close ran before context-ignoring Execute returned") + } + if got := ctrl.closeCalls.Load(); got != 1 { + t.Fatalf("Close calls = %d, want 1", got) + } +} + +func TestAddControllerReleaseWarnsWhileExecuteIsStuck(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + logger, hook := logrustest.NewNullLogger() + dc := directivecontroller.NewController(t.Context(), logrus.NewEntry(logger)) + tracking := &trackingDirectiveController{ + Controller: dc, + detached: make(chan struct{}), + } + b := NewBusWithLogger(tracking, logrus.NewEntry(logger)) + executionStarted := make(chan struct{}) + allowExecuteReturn := make(chan struct{}) + releaseReturned := make(chan struct{}) + ctrl := &lifecycleController{executeFn: func(context.Context) error { + close(executionStarted) + <-allowExecuteReturn + return nil + }} + + release, err := b.AddController(t.Context(), ctrl, nil) + if err != nil { + t.Fatalf("AddController failed: %v", err) + } + <-executionStarted + go func() { + release() + close(releaseReturned) + }() + <-tracking.detached + + time.Sleep(releaseWarningInterval) + synctest.Wait() + if got := len(hook.AllEntries()); got != 1 { + t.Fatalf("warning count after first interval = %d, want 1", got) + } + time.Sleep(releaseWarningInterval) + synctest.Wait() + entries := hook.AllEntries() + if got := len(entries); got != 2 { + t.Fatalf("warning count after second interval = %d, want 2", got) + } + for i, entry := range entries { + if entry.Level != logrus.WarnLevel { + t.Fatalf("entry %d level = %s, want warning", i, entry.Level) + } + if entry.Message != "waiting for controller Execute to return" { + t.Fatalf("entry %d message = %q", i, entry.Message) + } + if entry.Data["controller"] != ctrl { + t.Fatalf("entry %d controller = %v, want attached controller", i, entry.Data["controller"]) + } + } + select { + case <-releaseReturned: + t.Fatal("release returned while Execute was stuck") + default: + } + + close(allowExecuteReturn) + synctest.Wait() + select { + case <-releaseReturned: + default: + t.Fatal("release did not return after Execute returned") + } + }) +} + +func TestAddControllerLifecycleOutcomes(t *testing.T) { + t.Run("add failure closes and reports close error", func(t *testing.T) { + addErr := stderrors.New("handler registration failed") + closeErr := stderrors.New("failed attachment close failed") + tracking := &trackingDirectiveController{addErr: addErr, detached: make(chan struct{})} + b := NewBus(tracking) + ctrl := &lifecycleController{closeFn: func() error { return closeErr }} + + release, err := b.AddController(t.Context(), ctrl, nil) + if release != nil { + t.Fatal("release function is non-nil after add failure") + } + if !stderrors.Is(err, addErr) { + t.Fatalf("AddController error = %v, want add error", err) + } + var typedCloseErr *bus.ControllerCloseError + if !stderrors.As(err, &typedCloseErr) || !stderrors.Is(typedCloseErr, closeErr) { + t.Fatalf("AddController error = %v, want typed close error", err) + } + if got := ctrl.closeCalls.Load(); got != 1 { + t.Fatalf("Close calls after add failure = %d, want 1", got) + } + }) + + t.Run("nil return remains attached until release", func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + b, tracking := newTrackingBus() + executed := make(chan struct{}) + callbackResults := make(chan error, 1) + ctrl := &lifecycleController{executeFn: func(context.Context) error { + close(executed) + return nil + }} + release, err := b.AddController(t.Context(), ctrl, func(err error) { callbackResults <- err }) + if err != nil { + t.Fatalf("AddController failed: %v", err) + } + <-executed + synctest.Wait() + if got := len(b.GetControllers()); got != 1 { + t.Fatalf("controller count after nil Execute = %d, want 1", got) + } + release() + if got := ctrl.closeCalls.Load(); got != 1 { + t.Fatalf("Close calls = %d, want 1", got) + } + if got := tracking.detachCalls.Load(); got != 1 { + t.Fatalf("handler detach calls = %d, want 1", got) + } + if got := <-callbackResults; got != nil { + t.Fatalf("callback error = %v, want nil", got) + } + }) + }) + + t.Run("execution error finalizes and reaches callback", func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + b, tracking := newTrackingBus() + executeErr := stderrors.New("execute failed") + callbackResults := make(chan error, 1) + ctrl := &lifecycleController{executeFn: func(context.Context) error { return executeErr }} + _, err := b.AddController(t.Context(), ctrl, func(err error) { callbackResults <- err }) + if err != nil { + t.Fatalf("AddController failed: %v", err) + } + synctest.Wait() + if got := <-callbackResults; !stderrors.Is(got, executeErr) { + t.Fatalf("callback error = %v, want execution error", got) + } + if got := ctrl.closeCalls.Load(); got != 1 { + t.Fatalf("Close calls = %d, want 1", got) + } + if got := tracking.detachCalls.Load(); got != 1 { + t.Fatalf("handler detach calls = %d, want 1", got) + } + }) + }) + + t.Run("panic finalizes and reaches callback", func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + b, tracking := newTrackingBus() + callbackResults := make(chan error, 1) + ctrl := &lifecycleController{executeFn: func(context.Context) error { panic("execute panic") }} + _, err := b.AddController(t.Context(), ctrl, func(err error) { callbackResults <- err }) + if err != nil { + t.Fatalf("AddController failed: %v", err) + } + synctest.Wait() + got := <-callbackResults + if got == nil || !strings.Contains(got.Error(), "controller panicked") { + t.Fatalf("callback error = %v, want recovered panic", got) + } + if got := ctrl.closeCalls.Load(); got != 1 { + t.Fatalf("Close calls = %d, want 1", got) + } + if got := tracking.detachCalls.Load(); got != 1 { + t.Fatalf("handler detach calls = %d, want 1", got) + } + }) + }) + + t.Run("close error is typed in callback", func(t *testing.T) { + b, _ := newTrackingBus() + closeErr := stderrors.New("close failed") + executed := make(chan struct{}) + callbackResults := make(chan error, 1) + ctrl := &lifecycleController{ + executeFn: func(context.Context) error { close(executed); return nil }, + closeFn: func() error { return closeErr }, + } + release, err := b.AddController(t.Context(), ctrl, func(err error) { callbackResults <- err }) + if err != nil { + t.Fatalf("AddController failed: %v", err) + } + <-executed + release() + got := <-callbackResults + var typedCloseErr *bus.ControllerCloseError + if !stderrors.As(got, &typedCloseErr) || !stderrors.Is(typedCloseErr, closeErr) { + t.Fatalf("callback error = %v, want typed close error", got) + } + }) + + t.Run("repeated release finalizes once", func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + b, tracking := newTrackingBus() + executed := make(chan struct{}) + var callbackCalls atomic.Int32 + ctrl := &lifecycleController{executeFn: func(context.Context) error { close(executed); return nil }} + release, err := b.AddController(t.Context(), ctrl, func(error) { callbackCalls.Add(1) }) + if err != nil { + t.Fatalf("AddController failed: %v", err) + } + <-executed + synctest.Wait() + go release() + go release() + synctest.Wait() + if got := ctrl.closeCalls.Load(); got != 1 { + t.Fatalf("Close calls after repeated release = %d, want 1", got) + } + if got := tracking.detachCalls.Load(); got != 1 { + t.Fatalf("handler detach calls after repeated release = %d, want 1", got) + } + if got := callbackCalls.Load(); got != 1 { + t.Fatalf("callback calls after repeated release = %d, want 1", got) + } + }) + }) +} + +func TestExecuteControllerLifecycle(t *testing.T) { + t.Run("nil return requires removal", func(t *testing.T) { + b, tracking := newTrackingBus() + ctrl := &lifecycleController{} + if err := b.ExecuteController(t.Context(), ctrl); err != nil { + t.Fatalf("ExecuteController failed: %v", err) + } + if got := len(b.GetControllers()); got != 1 { + t.Fatalf("controller count after nil Execute = %d, want 1", got) + } + if got := ctrl.closeCalls.Load(); got != 0 { + t.Fatalf("Close calls before removal = %d, want 0", got) + } + b.RemoveController(ctrl) + if got := ctrl.closeCalls.Load(); got != 1 { + t.Fatalf("Close calls after removal = %d, want 1", got) + } + if got := tracking.detachCalls.Load(); got != 1 { + t.Fatalf("handler detach calls = %d, want 1", got) + } + }) + + t.Run("execution and close errors are returned", func(t *testing.T) { + b, tracking := newTrackingBus() + executeErr := stderrors.New("sync execute failed") + closeErr := stderrors.New("sync close failed") + ctrl := &lifecycleController{ + executeFn: func(context.Context) error { return executeErr }, + closeFn: func() error { return closeErr }, + } + err := b.ExecuteController(t.Context(), ctrl) + if !stderrors.Is(err, executeErr) { + t.Fatalf("ExecuteController error = %v, want execution error", err) + } + var typedCloseErr *bus.ControllerCloseError + if !stderrors.As(err, &typedCloseErr) || !stderrors.Is(typedCloseErr, closeErr) { + t.Fatalf("ExecuteController error = %v, want typed close error", err) + } + if got := ctrl.closeCalls.Load(); got != 1 { + t.Fatalf("Close calls = %d, want 1", got) + } + if got := tracking.detachCalls.Load(); got != 1 { + t.Fatalf("handler detach calls = %d, want 1", got) + } + }) + + t.Run("concurrent removal cancels and waits", func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + b, tracking := newTrackingBus() + executionStarted := make(chan struct{}) + cancellationObserved := make(chan struct{}) + allowExecuteReturn := make(chan struct{}) + executeReturned := make(chan error, 1) + removeReturned := make(chan struct{}) + ctrl := &lifecycleController{executeFn: func(ctx context.Context) error { + close(executionStarted) + <-ctx.Done() + close(cancellationObserved) + <-allowExecuteReturn + return nil + }} + + go func() { + executeReturned <- b.ExecuteController(t.Context(), ctrl) + }() + <-executionStarted + go func() { + b.RemoveController(ctrl) + close(removeReturned) + }() + <-cancellationObserved + <-tracking.detached + synctest.Wait() + + executeReturnedEarly := false + select { + case <-executeReturned: + executeReturnedEarly = true + default: + } + removeReturnedEarly := false + select { + case <-removeReturned: + removeReturnedEarly = true + default: + } + closeCallsBeforeReturn := ctrl.closeCalls.Load() + + close(allowExecuteReturn) + synctest.Wait() + if executeReturnedEarly { + t.Fatal("ExecuteController returned before controller Execute returned") + } + if removeReturnedEarly { + t.Fatal("RemoveController returned before controller Execute returned") + } + if closeCallsBeforeReturn != 0 { + t.Fatalf("Close calls before Execute returned = %d, want 0", closeCallsBeforeReturn) + } + select { + case err := <-executeReturned: + if err != nil { + t.Fatalf("ExecuteController failed: %v", err) + } + default: + t.Fatal("ExecuteController did not return after controller Execute returned") + } + select { + case <-removeReturned: + default: + t.Fatal("RemoveController did not return after controller Execute returned") + } + if got := ctrl.closeCalls.Load(); got != 1 { + t.Fatalf("Close calls = %d, want 1", got) + } + if got := tracking.detachCalls.Load(); got != 1 { + t.Fatalf("handler detach calls = %d, want 1", got) + } + }) + }) +} + +func TestRemoveControllerWaitsForExecute(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + b, tracking := newTrackingBus() + executionStarted := make(chan struct{}) + cancellationObserved := make(chan struct{}) + allowExecuteReturn := make(chan struct{}) + removeReturned := make(chan struct{}) + var executeReturned atomic.Bool + ctrl := &lifecycleController{ + executeFn: func(ctx context.Context) error { + close(executionStarted) + <-ctx.Done() + close(cancellationObserved) + <-allowExecuteReturn + executeReturned.Store(true) + return nil + }, + closeFn: func() error { + if !executeReturned.Load() { + t.Error("Close called before Execute returned") + } + return nil + }, + } + release, err := b.AddController(t.Context(), ctrl, nil) + if err != nil { + t.Fatalf("AddController failed: %v", err) + } + <-executionStarted + go func() { + b.RemoveController(ctrl) + close(removeReturned) + }() + <-cancellationObserved + <-tracking.detached + synctest.Wait() + removeReturnedEarly := false + select { + case <-removeReturned: + removeReturnedEarly = true + default: + } + closeCallsBeforeReturn := ctrl.closeCalls.Load() + + close(allowExecuteReturn) + synctest.Wait() + if removeReturnedEarly { + t.Fatal("RemoveController returned before Execute returned") + } + if closeCallsBeforeReturn != 0 { + t.Fatalf("Close calls before Execute returned = %d, want 0", closeCallsBeforeReturn) + } + select { + case <-removeReturned: + default: + t.Fatal("RemoveController did not return after Execute returned") + } + if got := ctrl.closeCalls.Load(); got != 1 { + t.Fatalf("Close calls = %d, want 1", got) + } + release() + if got := ctrl.closeCalls.Load(); got != 1 { + t.Fatalf("Close calls after release following removal = %d, want 1", got) + } + }) +} + +func TestAddControllerCallsControllerOutsideBusMutex(t *testing.T) { + const timeout = time.Second + + tracking := &trackingDirectiveController{detached: make(chan struct{})} + b := NewBus(tracking) + handlerReturned := make(chan struct{}) + tracking.beforeAdd = func(handler directive.Handler) { + _, _ = handler.HandleDirective(t.Context(), nil) + close(handlerReturned) + } + ctrl := &lifecycleController{handleFn: func(context.Context, directive.Instance) ([]directive.Resolver, error) { + _ = b.GetControllers() + return nil, nil + }} + type addResult struct { + release func() + err error + } + result := make(chan addResult, 1) + go func() { + release, err := b.AddController(t.Context(), ctrl, nil) + result <- addResult{release: release, err: err} + }() + + var got addResult + select { + case got = <-result: + case <-time.After(timeout): + t.Fatal("AddController held the bus mutex while calling HandleDirective") + } + if got.err != nil { + t.Fatalf("AddController failed: %v", got.err) + } + select { + case <-handlerReturned: + default: + t.Fatal("HandleDirective was not called during registration") + } + got.release() + if calls := ctrl.handleCalls.Load(); calls != 1 { + t.Fatalf("HandleDirective calls = %d, want 1", calls) + } +} diff --git a/core/core.go b/core/core.go index a0b5773e..3ce1d7d4 100644 --- a/core/core.go +++ b/core/core.go @@ -51,7 +51,7 @@ func NewCoreBus( opts ...Option, ) (bus.Bus, *static.Resolver, error) { dc := cdc.NewController(ctx, le) - b := inmem.NewBus(dc) + b := inmem.NewBusWithLogger(dc, le) // Process options conf := &CoreBusConfig{}