fix: remove dangling variable references on model destroy (#165)

* fix: remove dangling variable references on model destroy

* test: handle subscription cleanup more gracefully

Author: aaron-steinfeld

.semaphore/semaphore.yml

@@ -122,8 +122,8 @@
     "lodash-es": "^4.17.20"
   },
   "peerDependencies": {
-    "rxjs": "^6.5.5",
-    "core-js": "^3.6.5"
+    "core-js": "^3.6.5",
+    "rxjs": "^6.5.5"
   },
   "repository": {
     "type": "git",

package-lock.json

@@ -19,6 +19,7 @@ export * from './model/editor/editor-library';
 export * from './model/events/model-changed-event';
 export * from './model/events/model-created-event';
 export * from './model/events/model-destroyed-event';
+export * from './model/events/before-model-destroyed-event';
 export * from './model/events/model-event-installer';
 export * from './model/manager/model-lifecycle-hooks';
 export * from './model/manager/model-manager';

package.json

@@ -0,0 +1,52 @@
+import { DashboardEventManager } from '../../communication/dashboard-event-manager';
+import { getTestScheduler } from '../../test/rxjs-jest-test-scheduler';
+import { BeforeModelDestroyedEvent } from './before-model-destroyed-event';
+
+describe('Before model destroyed event', () => {
+  let mockEventManager: Partial<DashboardEventManager>;
+  let beforeModelDestroyedEvent: BeforeModelDestroyedEvent;
+
+  beforeEach(() => {
+    mockEventManager = {};
+    beforeModelDestroyedEvent = new BeforeModelDestroyedEvent(mockEventManager as DashboardEventManager);
+  });
+  test('relays all publishes to manager', () => {
+    mockEventManager.publishEvent = jest.fn();
+    const first = {};
+    const second = {};
+    beforeModelDestroyedEvent.publish(first);
+    beforeModelDestroyedEvent.publish(second);
+
+    expect(mockEventManager.publishEvent).toHaveBeenCalledTimes(2);
+
+    expect(mockEventManager.publishEvent).nthCalledWith(1, beforeModelDestroyedEvent.getKey(), first);
+
+    expect(mockEventManager.publishEvent).nthCalledWith(2, beforeModelDestroyedEvent.getKey(), second);
+  });
+
+  test('gets observable from manager for this event', () => {
+    mockEventManager.getObservableForEvent = jest.fn();
+
+    beforeModelDestroyedEvent.getObservable();
+
+    expect(mockEventManager.getObservableForEvent).toHaveBeenCalledWith(beforeModelDestroyedEvent.getKey());
+  });
+
+  test('before destruction observables notifies on the provided model then completes', () => {
+    getTestScheduler().run(({ cold, expectObservable }) => {
+      const mockModels = {
+        a: {},
+        b: {}
+      };
+      beforeModelDestroyedEvent.getObservable = jest.fn().mockReturnValue(cold('a-b-', mockModels));
+
+      expectObservable(beforeModelDestroyedEvent.getBeforeDestructionObservable(mockModels.a)).toBe('(a|)', {
+        a: undefined
+      });
+
+      expectObservable(beforeModelDestroyedEvent.getBeforeDestructionObservable(mockModels.b)).toBe('--(b|)', {
+        b: undefined
+      });
+    });
+  });
+});

src/hyperdash.ts

@@ -0,0 +1,27 @@
+import { Observable } from 'rxjs';
+import { filter, mapTo, take } from 'rxjs/operators';
+import { DashboardEvent } from '../../communication/dashboard-event';
+import { DashboardEventManager } from '../../communication/dashboard-event-manager';
+
+export const beforeModelDestroyedEventKey = Symbol('Before model destroyed');
+
+/**
+ * Fired before a model is destroyed and any destroy hooks are called.
+ */
+export class BeforeModelDestroyedEvent extends DashboardEvent<object> {
+  public constructor(dashboardEventManager: DashboardEventManager) {
+    super(dashboardEventManager, beforeModelDestroyedEventKey);
+  }
+
+  /**
+   * Returns a void observable that will notify once when the provided model is
+   * destroyed, then complete.
+   */
+  public getBeforeDestructionObservable(model: object): Observable<void> {
+    return this.getObservable().pipe(
+      filter(destroyedModel => destroyedModel === model),
+      mapTo(undefined),
+      take(1)
+    );
+  }
+}

src/model/events/before-model-destroyed-event.test.ts

@@ -3,19 +3,24 @@ import { PartialObjectMock } from '../../test/partial-object-mock';
 import { Logger } from '../../util/logging/logger';
 import { ModelApiBuilder } from '../api/builder/model-api-builder';
 import { ModelApi } from '../api/model-api';
+import { BeforeModelDestroyedEvent } from '../events/before-model-destroyed-event';
 import { ModelCreatedEvent } from '../events/model-created-event';
 import { ModelDestroyedEvent } from '../events/model-destroyed-event';
 import { ModelOnDestroy, ModelOnInit } from './model-lifecycle-hooks';
 import { ModelManager } from './model-manager';
 
 describe('Model manager', () => {
-  const testClass = class TestClass {};
+  const testClass = class TestClass {
+    // Value used to disambiguate equality between instances
+    public readonly value: number = Math.random();
+  };
   let manager: ModelManager;
   let mockLogger: PartialObjectMock<Logger>;
   let mockApiBuilder: PartialObjectMock<ModelApiBuilder<ModelApi>>;
 
   let mockCreatedEvent: PartialObjectMock<ModelCreatedEvent>;
   let mockDestroyedEvent: PartialObjectMock<ModelDestroyedEvent>;
+  let mockBeforeDestroyedEvent: PartialObjectMock<BeforeModelDestroyedEvent>;
 
   beforeEach(() => {
     mockCreatedEvent = {
@@ -26,6 +31,10 @@ describe('Model manager', () => {
       publish: jest.fn()
     };
 
+    mockBeforeDestroyedEvent = {
+      publish: jest.fn()
+    };
+
     mockLogger = {
       warn: jest.fn((message: string) => ({
         throw: jest.fn(() => {
@@ -43,7 +52,8 @@ describe('Model manager', () => {
     manager = new ModelManager(
       mockLogger as Logger,
       mockCreatedEvent as ModelCreatedEvent,
-      mockDestroyedEvent as ModelDestroyedEvent
+      mockDestroyedEvent as ModelDestroyedEvent,
+      mockBeforeDestroyedEvent as BeforeModelDestroyedEvent
     );
 
     manager.registerModelApiBuilder(mockApiBuilder as ModelApiBuilder<ModelApi>);
@@ -233,8 +243,17 @@ describe('Model manager', () => {
     expect(mockCreatedEvent.publish).toHaveBeenNthCalledWith(1, root);
     expect(mockCreatedEvent.publish).toHaveBeenNthCalledWith(2, firstChild);
 
+    mockBeforeDestroyedEvent.publish = jest.fn(model => {
+      expect(manager.isTrackedModel(model)).toBe(true);
+      expect(mockDestroyedEvent.publish).not.toHaveBeenCalledWith(model);
+    });
+
     manager.destroy(root);
 
+    expect(mockBeforeDestroyedEvent.publish).toHaveBeenCalledTimes(2);
+    expect(mockBeforeDestroyedEvent.publish).toHaveBeenNthCalledWith(1, firstChild);
+    expect(mockBeforeDestroyedEvent.publish).toHaveBeenNthCalledWith(2, root);
+
     expect(mockDestroyedEvent.publish).toHaveBeenCalledTimes(2);
     expect(mockDestroyedEvent.publish).toHaveBeenNthCalledWith(1, firstChild);
     expect(mockDestroyedEvent.publish).toHaveBeenNthCalledWith(2, root);
@@ -292,7 +311,8 @@ describe('Model manager', () => {
     manager = new ModelManager(
       mockLogger as Logger,
       mockCreatedEvent as ModelCreatedEvent,
-      mockDestroyedEvent as ModelDestroyedEvent
+      mockDestroyedEvent as ModelDestroyedEvent,
+      mockBeforeDestroyedEvent as BeforeModelDestroyedEvent
     ); // Rebuild so we don't use the mock api build from other tests
 
     const modelAClass = class {};

src/model/events/before-model-destroyed-event.ts

@@ -3,6 +3,7 @@ import { Constructable } from '../../util/constructable';
 import { Logger } from '../../util/logging/logger';
 import { ModelApiBuilder } from '../api/builder/model-api-builder';
 import { ModelApi } from '../api/model-api';
+import { BeforeModelDestroyedEvent } from '../events/before-model-destroyed-event';
 import { ModelCreatedEvent } from '../events/model-created-event';
 import { ModelDestroyedEvent } from '../events/model-destroyed-event';
 import { ModelOnDestroy, ModelOnInit } from './model-lifecycle-hooks';
@@ -19,7 +20,8 @@ export class ModelManager {
   public constructor(
     private readonly logger: Logger,
     private readonly modelCreatedEvent: ModelCreatedEvent,
-    private readonly modelDestroyedEvent: ModelDestroyedEvent
+    private readonly modelDestroyedEvent: ModelDestroyedEvent,
+    private readonly beforeModelDestroyedEvent: BeforeModelDestroyedEvent
   ) {}
 
   /**
@@ -233,6 +235,8 @@ export class ModelManager {
     // Depth first, destroy children before self
     instanceData.children.forEach(child => this.destroy(child));
 
+    this.beforeModelDestroyedEvent.publish(modelInstance);
+
     if (this.modelHasDestroyHook(modelInstance)) {
       modelInstance.modelOnDestroy();
     }

src/model/manager/model-manager.test.ts

@@ -1,4 +1,6 @@
 // tslint:disable:no-invalid-template-strings
+import { EMPTY, Subject } from 'rxjs';
+import { BeforeModelDestroyedEvent } from '../../model/events/before-model-destroyed-event';
 import { ModelChangedEvent } from '../../model/events/model-changed-event';
 import { ModelManager } from '../../model/manager/model-manager';
 import { PropertyLocation } from '../../model/property/property-location';
@@ -11,13 +13,15 @@ describe('Variable manager', () => {
   let mockLogger: Partial<Logger>;
   let mockModelManager: Partial<ModelManager>;
   let mockModelChangedEvent: Partial<ModelChangedEvent>;
+  let mockBeforeModelDestroyedEvent: Partial<BeforeModelDestroyedEvent>;
   const model = {};
   const parent = {};
   const root = {};
 
   beforeEach(() => {
     mockLogger = {
-      warn: jest.fn()
+      warn: jest.fn(),
+      error: jest.fn()
     };
     mockModelChangedEvent = {
       publishChange: jest.fn()
@@ -35,10 +39,15 @@ describe('Variable manager', () => {
       })
     };
 
+    mockBeforeModelDestroyedEvent = {
+      getBeforeDestructionObservable: jest.fn()
+    };
+
     manager = new VariableManager(
       mockLogger as Logger,
       mockModelManager as ModelManager,
-      mockModelChangedEvent as ModelChangedEvent
+      mockModelChangedEvent as ModelChangedEvent,
+      mockBeforeModelDestroyedEvent as BeforeModelDestroyedEvent
     );
   });
 
@@ -106,6 +115,7 @@ describe('Variable manager reference tracking', () => {
   let mockLogger: PartialObjectMock<Logger>;
   let mockModelManager: PartialObjectMock<ModelManager>;
   let mockModelChangedEvent: PartialObjectMock<ModelChangedEvent>;
+  let mockBeforeModelDestroyedEvent: Partial<BeforeModelDestroyedEvent>;
 
   let mockParentLocation: PartialObjectMock<PropertyLocation>;
   const parent = {};
@@ -132,6 +142,10 @@ describe('Variable manager reference tracking', () => {
       )
     };
 
+    mockBeforeModelDestroyedEvent = {
+      getBeforeDestructionObservable: jest.fn().mockReturnValue(EMPTY)
+    };
+
     mockModelLocation = {
       parentModel: model,
       setProperty: jest.fn(),
@@ -147,7 +161,8 @@ describe('Variable manager reference tracking', () => {
     manager = new VariableManager(
       mockLogger as Logger,
       mockModelManager as ModelManager,
-      mockModelChangedEvent as ModelChangedEvent
+      mockModelChangedEvent as ModelChangedEvent,
+      mockBeforeModelDestroyedEvent as BeforeModelDestroyedEvent
     );
   });
 
@@ -323,4 +338,45 @@ describe('Variable manager reference tracking', () => {
         'Attempted to resolve reference at modelProp which does not contain a registered reference'
       );
   });
+
+  test('removes dangling references after a model is destroyed', () => {
+    const destroySubject = new Subject();
+    mockBeforeModelDestroyedEvent.getBeforeDestructionObservable = jest
+      .fn()
+      .mockReturnValue(destroySubject.asObservable());
+
+    manager.registerReference(mockModelLocation as PropertyLocation, '${test}');
+    manager.set('test', 'foo', parent);
+
+    destroySubject.next(model);
+    destroySubject.complete();
+
+    // Reference should no longer be tracked
+    expect(manager.isVariableReference(mockModelLocation as PropertyLocation)).toBe(false);
+
+    // Setting a value should not call model manager or set property
+    (mockModelManager.getParent as jest.Mock).mockClear();
+    manager.set('test', 'bar', parent);
+
+    expect(mockModelManager.getParent).not.toHaveBeenCalled();
+    expect(mockModelLocation.setProperty).not.toHaveBeenCalledWith('bar');
+  });
+
+  test('dangling reference removal handles case where reference previously removed', () => {
+    const destroySubject = new Subject();
+    mockBeforeModelDestroyedEvent.getBeforeDestructionObservable = jest
+      .fn()
+      .mockReturnValue(destroySubject.asObservable());
+
+    manager.registerReference(mockModelLocation as PropertyLocation, '${test}');
+    manager.set('test', 'foo', parent);
+    manager.deregisterReference(mockModelLocation as PropertyLocation);
+
+    // Reference should no longer be tracked
+    expect(manager.isVariableReference(mockModelLocation as PropertyLocation)).toBe(false);
+
+    destroySubject.next(model);
+    destroySubject.complete();
+    expect(mockLogger.error).not.toHaveBeenCalled();
+  });
 });

src/model/manager/model-manager.ts

@@ -1,4 +1,5 @@
 import { fromPairs } from 'lodash-es';
+import { BeforeModelDestroyedEvent } from '../../model/events/before-model-destroyed-event';
 import { ModelChangedEvent } from '../../model/events/model-changed-event';
 import { ModelManager } from '../../model/manager/model-manager';
 import { PropertyLocation } from '../../model/property/property-location';
@@ -22,7 +23,8 @@ export class VariableManager {
   public constructor(
     private readonly logger: Logger,
     private readonly modelManager: ModelManager,
-    private readonly modelChangedEvent: ModelChangedEvent
+    private readonly modelChangedEvent: ModelChangedEvent,
+    private readonly beforeModelDestroyedEvent: BeforeModelDestroyedEvent
   ) {}
 
   /**
@@ -76,17 +78,22 @@ export class VariableManager {
    * Throws Error if the provided location is already being tracked
    */
   public registerReference(location: PropertyLocation, variableExpression: string): unknown {
-    let reference = new VariableReference(variableExpression, location);
-
     const referenceMap = this.getOrCreateReferenceMapForModelContainingLocation(location);
 
     if (referenceMap.has(location.toString())) {
       this.logger.error(`Attempting to register reference which has already been declared at ${location.toString()}`);
-      reference = referenceMap.get(location.toString())!;
     } else {
-      referenceMap.set(location.toString(), reference);
+      const autoCleanupSubscription = this.beforeModelDestroyedEvent
+        .getBeforeDestructionObservable(location.parentModel)
+        .subscribe(() => this.deregisterReference(location));
+      referenceMap.set(
+        location.toString(),
+        new VariableReference(variableExpression, location, autoCleanupSubscription)
+      );
     }
 
+    const reference = referenceMap.get(location.toString())!;
+
     return this.updateReference(reference);
   }
 
@@ -126,6 +133,7 @@ export class VariableManager {
     this.getOrCreateReferenceMapForModelContainingLocation(location).delete(reference.location.toString());
 
     const result = reference.unresolve();
+    reference.autoCleanupSubscription.unsubscribe();
     this.updateValueReferenceTrackingFromEvaluationResult(reference, result);
 
     return result.value!;

src/variable/manager/variable-manager.test.ts

@@ -1,4 +1,5 @@
 // tslint:disable:no-invalid-template-strings
+import { Subscription } from 'rxjs';
 import { PropertyLocation } from '../../model/property/property-location';
 import { EvaluationResult, VariableEvaluator } from '../evaluator/variable-evaluator';
 import { VariableReference } from './variable-reference';
@@ -10,6 +11,7 @@ describe('Variable reference', () => {
   let mockLocation: Partial<PropertyLocation>;
   let mockEvaluationResult: Partial<EvaluationResult<unknown>>;
   let mockUnevaluationResult: Partial<EvaluationResult<unknown>>;
+  const closeSubscription: Subscription = new Subscription();
 
   beforeEach(() => {
     mockedVariableEvaluatorConstructor.mockReset();
@@ -32,14 +34,14 @@ describe('Variable reference', () => {
   });
 
   test('resolve sets the property to the evaluation result', () => {
-    const ref = new VariableReference('${test}', mockLocation as PropertyLocation);
+    const ref = new VariableReference('${test}', mockLocation as PropertyLocation, closeSubscription);
 
     expect(ref.resolve({})).toBe(mockEvaluationResult);
     expect(mockLocation.setProperty).toHaveBeenCalledWith(mockEvaluationResult.value);
   });
 
   test('unresolve returns the unevaluate result', () => {
-    const ref = new VariableReference('${test}', mockLocation as PropertyLocation);
+    const ref = new VariableReference('${test}', mockLocation as PropertyLocation, closeSubscription);
     expect(ref.unresolve()).toBe(mockUnevaluationResult);
   });
 });

src/variable/manager/variable-manager.ts

@@ -1,3 +1,4 @@
+import { Subscription } from 'rxjs';
 import { PropertyLocation } from '../../model/property/property-location';
 import { EvaluationResult, VariableEvaluator } from '../evaluator/variable-evaluator';
 import { ResolveDictionary } from '../variable-dictionary';
@@ -8,7 +9,11 @@ import { ResolveDictionary } from '../variable-dictionary';
 export class VariableReference<T = unknown> {
   private readonly evaluator: VariableEvaluator<T>;
 
-  public constructor(variableString: string, public readonly location: PropertyLocation<T>) {
+  public constructor(
+    variableString: string,
+    public readonly location: PropertyLocation<T>,
+    public readonly autoCleanupSubscription: Subscription
+  ) {
     this.evaluator = new VariableEvaluator(variableString);
   }