fix(core): address review feedback from Gemini and Copilot
- Move locked check inside mutex in registerService to fix TOCTOU race - Add mutex guards to enableLock and applyLock methods - Replace fmt.Errorf with errors.Join in action() for correct error aggregation (consistent with queryAll and lifecycle methods) - Add TestMessageBus_Action_Bad for error aggregation coverage Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
parent
80f23ba9b9
commit
eba9c950bb
3 changed files with 25 additions and 5 deletions
|
|
@ -2,7 +2,6 @@ package core
|
||||||
|
|
||||||
import (
|
import (
|
||||||
"errors"
|
"errors"
|
||||||
"fmt"
|
|
||||||
"sync"
|
"sync"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
@ -35,7 +34,7 @@ func (b *messageBus) action(msg Message) error {
|
||||||
var agg error
|
var agg error
|
||||||
for _, h := range handlers {
|
for _, h := range handlers {
|
||||||
if err := h(b.core, msg); err != nil {
|
if err := h(b.core, msg); err != nil {
|
||||||
agg = fmt.Errorf("%w; %v", agg, err)
|
agg = errors.Join(agg, err)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
return agg
|
return agg
|
||||||
|
|
|
||||||
|
|
@ -1,6 +1,7 @@
|
||||||
package core
|
package core
|
||||||
|
|
||||||
import (
|
import (
|
||||||
|
"errors"
|
||||||
"sync"
|
"sync"
|
||||||
"testing"
|
"testing"
|
||||||
|
|
||||||
|
|
@ -25,6 +26,22 @@ func TestMessageBus_Action_Good(t *testing.T) {
|
||||||
assert.Len(t, received, 2)
|
assert.Len(t, received, 2)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestMessageBus_Action_Bad(t *testing.T) {
|
||||||
|
c, _ := New()
|
||||||
|
|
||||||
|
err1 := errors.New("handler1 failed")
|
||||||
|
err2 := errors.New("handler2 failed")
|
||||||
|
|
||||||
|
c.bus.registerAction(func(_ *Core, msg Message) error { return err1 })
|
||||||
|
c.bus.registerAction(func(_ *Core, msg Message) error { return nil })
|
||||||
|
c.bus.registerAction(func(_ *Core, msg Message) error { return err2 })
|
||||||
|
|
||||||
|
err := c.bus.action("test")
|
||||||
|
assert.Error(t, err)
|
||||||
|
assert.ErrorIs(t, err, err1)
|
||||||
|
assert.ErrorIs(t, err, err2)
|
||||||
|
}
|
||||||
|
|
||||||
func TestMessageBus_RegisterAction_Good(t *testing.T) {
|
func TestMessageBus_RegisterAction_Good(t *testing.T) {
|
||||||
c, _ := New()
|
c, _ := New()
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -26,14 +26,14 @@ func newServiceManager() *serviceManager {
|
||||||
// registerService adds a named service to the registry.
|
// registerService adds a named service to the registry.
|
||||||
// It also appends to startables/stoppables if the service implements those interfaces.
|
// It also appends to startables/stoppables if the service implements those interfaces.
|
||||||
func (m *serviceManager) registerService(name string, svc any) error {
|
func (m *serviceManager) registerService(name string, svc any) error {
|
||||||
if m.locked {
|
|
||||||
return fmt.Errorf("core: service %q is not permitted by the serviceLock setting", name)
|
|
||||||
}
|
|
||||||
if name == "" {
|
if name == "" {
|
||||||
return fmt.Errorf("core: service name cannot be empty")
|
return fmt.Errorf("core: service name cannot be empty")
|
||||||
}
|
}
|
||||||
m.mu.Lock()
|
m.mu.Lock()
|
||||||
defer m.mu.Unlock()
|
defer m.mu.Unlock()
|
||||||
|
if m.locked {
|
||||||
|
return fmt.Errorf("core: service %q is not permitted by the serviceLock setting", name)
|
||||||
|
}
|
||||||
if _, exists := m.services[name]; exists {
|
if _, exists := m.services[name]; exists {
|
||||||
return fmt.Errorf("core: service %q already registered", name)
|
return fmt.Errorf("core: service %q already registered", name)
|
||||||
}
|
}
|
||||||
|
|
@ -62,12 +62,16 @@ func (m *serviceManager) service(name string) any {
|
||||||
|
|
||||||
// enableLock marks that the lock should be applied after initialisation.
|
// enableLock marks that the lock should be applied after initialisation.
|
||||||
func (m *serviceManager) enableLock() {
|
func (m *serviceManager) enableLock() {
|
||||||
|
m.mu.Lock()
|
||||||
|
defer m.mu.Unlock()
|
||||||
m.lockEnabled = true
|
m.lockEnabled = true
|
||||||
}
|
}
|
||||||
|
|
||||||
// applyLock activates the service lock if it was enabled.
|
// applyLock activates the service lock if it was enabled.
|
||||||
// Called once during New() after all options have been processed.
|
// Called once during New() after all options have been processed.
|
||||||
func (m *serviceManager) applyLock() {
|
func (m *serviceManager) applyLock() {
|
||||||
|
m.mu.Lock()
|
||||||
|
defer m.mu.Unlock()
|
||||||
if m.lockEnabled {
|
if m.lockEnabled {
|
||||||
m.locked = true
|
m.locked = true
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue