RSDK-12383 Stop leaking resource.Resources with resLoggers in module code

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
+}

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)
+}

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 {