diff --git a/module/module.go b/module/module.go index 6a256821c28..4c601497fbd 100644 --- a/module/module.go +++ b/module/module.go @@ -692,6 +692,7 @@ func (m *Module) ReconfigureResource(ctx context.Context, req *pb.ReconfigureRes } delete(m.activeResourceStreams, res.Name()) + delete(m.resLoggers, res) resInfo, ok := resource.LookupRegistration(conf.API, conf.Model) if !ok { return nil, errors.Errorf("do not know how to construct %q", conf.API) @@ -700,10 +701,13 @@ func (m *Module) ReconfigureResource(ctx context.Context, req *pb.ReconfigureRes return nil, errors.Errorf("invariant: no constructor for %q", conf.API) } - newRes, err := resInfo.Constructor(ctx, deps, *conf, m.logger) + resLogger := m.logger.Sublogger(conf.ResourceName().String()) + newRes, err := resInfo.Constructor(ctx, deps, *conf, resLogger) if err != nil { return nil, err } + m.resLoggers[newRes] = resLogger + var passthroughSource rtppassthrough.Source if p, ok := newRes.(rtppassthrough.Source); ok { passthroughSource = p @@ -776,6 +780,7 @@ func (m *Module) RemoveResource(ctx context.Context, req *pb.RemoveResourceReque delete(m.streamSourceByName, res.Name()) delete(m.activeResourceStreams, res.Name()) + delete(m.resLoggers, res) return &pb.RemoveResourceResponse{}, coll.Remove(name) } @@ -1020,3 +1025,9 @@ func validateRegistered(api resource.API, model resource.Model) error { return errors.Errorf("resource with API %s and model %s not yet registered", api, model) } + +// GetResourceLoggers returns the value of resLoggers. Only to be used for internal +// testing. +func (m *Module) GetResourceLoggers() map[resource.Resource]logging.Logger { + return m.resLoggers +} diff --git a/module/module_test.go b/module/module_test.go index 955e674c4df..baf75555887 100644 --- a/module/module_test.go +++ b/module/module_test.go @@ -1013,3 +1013,71 @@ func TestFrameSystemFromDependencies(t *testing.T) { test.That(t, err, test.ShouldBeNil) test.That(t, th.constructCount, test.ShouldEqual, 1) } + +func TestResLoggersCleanup(t *testing.T) { + // Primarily a regression test for RSDK-12383. Asserts values of resLoggers is as + // expected with no resources (empty), an added resource (one entry), a reconfigured + // resource (still one entry, but a different one given a must rebuild error), and a + // removed resource (empty). + ctx := t.Context() + logger := logging.NewTestLogger(t) + + modelName := utils.RandomAlphaString(5) + model := resource.DefaultModelFamily.WithModel(modelName) + resource.RegisterComponent(motor.API, model, resource.Registration[motor.Motor, resource.NoNativeConfig]{ + Constructor: func( + ctx context.Context, deps resource.Dependencies, cfg resource.Config, logger logging.Logger, + ) (motor.Motor, error) { + injectedMotor := &inject.Motor{ + ReconfigureFunc: func(ctx context.Context, deps resource.Dependencies, cfg resource.Config) error { + return resource.NewMustRebuildError(motor.Named(cfg.Name)) + }, + } + return injectedMotor, nil + }, + }) + t.Cleanup(func() { + resource.Deregister(motor.API, model) + }) + + cfg := &v1.ComponentConfig{ + Name: "foo", + Api: motor.API.String(), + Model: model.String(), + } + resName := motor.Named("foo") + + m := setupLocalModule(t, ctx, logger) + err := m.AddModelFromRegistry(ctx, motor.API, model) + test.That(t, err, test.ShouldBeNil) + + resLoggers := m.GetResourceLoggers() + test.That(t, resLoggers, test.ShouldBeEmpty) + + _, err = m.AddResource(ctx, &pb.AddResourceRequest{Config: cfg}) + test.That(t, err, test.ShouldBeNil) + + resLoggers = m.GetResourceLoggers() + test.That(t, resLoggers, test.ShouldNotBeNil) + test.That(t, len(resLoggers), test.ShouldEqual, 1) + var resourceAfterAddition resource.Resource + for res := range resLoggers { // for loop used to grab single key of map + resourceAfterAddition = res + } + + _, err = m.ReconfigureResource(ctx, &pb.ReconfigureResourceRequest{Config: cfg}) + test.That(t, err, test.ShouldBeNil) + + resLoggers = m.GetResourceLoggers() + test.That(t, resLoggers, test.ShouldNotBeNil) + test.That(t, len(resLoggers), test.ShouldEqual, 1) + for res := range resLoggers { // for loop used to grab single key of map + test.That(t, res, test.ShouldNotEqual, resourceAfterAddition) + } + + _, err = m.RemoveResource(ctx, &pb.RemoveResourceRequest{Name: resName.String()}) + test.That(t, err, test.ShouldBeNil) + + resLoggers = m.GetResourceLoggers() + test.That(t, resLoggers, test.ShouldBeEmpty) +} diff --git a/testutils/inject/motor.go b/testutils/inject/motor.go index 350a100cd17..87ea3702ff2 100644 --- a/testutils/inject/motor.go +++ b/testutils/inject/motor.go @@ -22,6 +22,7 @@ type Motor struct { StopFunc func(ctx context.Context, extra map[string]interface{}) error IsPoweredFunc func(ctx context.Context, extra map[string]interface{}) (bool, float64, error) IsMovingFunc func(context.Context) (bool, error) + ReconfigureFunc func(ctx context.Context, deps resource.Dependencies, conf resource.Config) error CloseFunc func(ctx context.Context) error } @@ -123,6 +124,17 @@ func (m *Motor) IsMoving(ctx context.Context) (bool, error) { return m.IsMovingFunc(ctx) } +// Reconfigure calls the injected Reconfigure or the real version. +func (m *Motor) Reconfigure(ctx context.Context, deps resource.Dependencies, conf resource.Config) error { + if m.ReconfigureFunc == nil { + if m.Motor == nil { + return resource.NewMustRebuildError(conf.ResourceName()) + } + return m.Motor.Reconfigure(ctx, deps, conf) + } + return m.ReconfigureFunc(ctx, deps, conf) +} + // Close calls the injected Close or the real version. func (m *Motor) Close(ctx context.Context) error { if m.CloseFunc == nil {