fix(core): allow onDestroy unregistration while destroying (#50237)

garrettld · AndrewKushnir · commit cc41758b595d · 2023-05-11T10:17:07.000-07:00
Ensure that all onDestroy callbacks are called by allowing unregistering of onDestroy hooks while destroying. Fixes #50221 PR Close #50237
diff --git a/packages/core/src/di/r3_injector.ts b/packages/core/src/di/r3_injector.ts
@@ -199,15 +199,18 @@ export class R3Injector extends EnvironmentInjector {
       for (const service of this._ngOnDestroyHooks) {
         service.ngOnDestroy();
       }
-      for (const hook of this._onDestroyHooks) {
+      const onDestroyHooks = this._onDestroyHooks;
+      // Reset the _onDestroyHooks array before iterating over it to prevent hooks that unregister
+      // themselves from mutating the array during iteration.
+      this._onDestroyHooks = [];
+      for (const hook of onDestroyHooks) {
         hook();
       }
     } finally {
       // Release all references.
       this.records.clear();
       this._ngOnDestroyHooks.clear();
       this.injectorDefTypes.clear();
-      this._onDestroyHooks.length = 0;
     }
   }
 
diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts
@@ -467,12 +467,14 @@ function processCleanups(tView: TView, lView: LView): void {
   }
   const destroyHooks = lView[ON_DESTROY_HOOKS];
   if (destroyHooks !== null) {
+    // Reset the ON_DESTROY_HOOKS array before iterating over it to prevent hooks that unregister
+    // themselves from mutating the array during iteration.
+    lView[ON_DESTROY_HOOKS] = null;
     for (let i = 0; i < destroyHooks.length; i++) {
       const destroyHooksFn = destroyHooks[i];
       ngDevMode && assertFunction(destroyHooksFn, 'Expecting destroy hook to be a function.');
       destroyHooksFn();
     }
-    lView[ON_DESTROY_HOOKS] = null;
   }
 }
 
diff --git a/packages/core/test/acceptance/destroy_ref_spec.ts b/packages/core/test/acceptance/destroy_ref_spec.ts
@@ -260,4 +260,31 @@ describe('DestroyRef', () => {
       destroyRef.onDestroy(() => {});
     }).toThrowError('NG0911: View has already been destroyed.');
   });
+
+  it('should allow unregistration while destroying', () => {
+    const destroyedLog: string[] = [];
+
+    @Component({
+      selector: 'test',
+      standalone: true,
+      template: ``,
+    })
+    class TestCmp {
+      constructor(destroyCtx: DestroyRef) {
+        const unregister = destroyCtx.onDestroy(() => {
+          destroyedLog.push('first');
+          unregister();
+        });
+        destroyCtx.onDestroy(() => {
+          destroyedLog.push('second');
+        });
+      }
+    }
+
+    const fixture = TestBed.createComponent(TestCmp);
+    expect(destroyedLog).toEqual([]);
+
+    fixture.componentRef.destroy();
+    expect(destroyedLog).toEqual(['first', 'second']);
+  });
 });
diff --git a/packages/core/test/acceptance/environment_injector_spec.ts b/packages/core/test/acceptance/environment_injector_spec.ts
@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {Component, createComponent, createEnvironmentInjector, ENVIRONMENT_INITIALIZER, EnvironmentInjector, inject, InjectFlags, InjectionToken, INJECTOR, Injector, NgModuleRef, ViewContainerRef} from '@angular/core';
+import {Component, createComponent, createEnvironmentInjector, DestroyRef, ENVIRONMENT_INITIALIZER, EnvironmentInjector, inject, InjectFlags, InjectionToken, INJECTOR, Injector, NgModuleRef, ViewContainerRef} from '@angular/core';
 import {R3Injector} from '@angular/core/src/di/r3_injector';
 import {RuntimeError, RuntimeErrorCode} from '@angular/core/src/errors';
 import {TestBed} from '@angular/core/testing';
@@ -27,6 +27,27 @@ describe('environment injector', () => {
     expect(destroyed).toBeTrue();
   });
 
+  it('should allow unregistration while destroying', () => {
+    const destroyedLog: string[] = [];
+
+    const parentEnvInjector = TestBed.inject(EnvironmentInjector);
+    const envInjector = createEnvironmentInjector([], parentEnvInjector);
+    const destroyRef = envInjector.get(DestroyRef);
+
+    const unregister = destroyRef.onDestroy(() => {
+      destroyedLog.push('first');
+      unregister();
+    });
+    destroyRef.onDestroy(() => {
+      destroyedLog.push('second');
+    });
+
+    expect(destroyedLog).toEqual([]);
+
+    envInjector.destroy();
+    expect(destroyedLog).toEqual(['first', 'second']);
+  });
+
   it('should see providers from a parent EnvInjector', () => {
     class Service {}
 

Original file line number	Diff line number	Diff line change
`@@ -199,15 +199,18 @@ export class R3Injector extends EnvironmentInjector {`
`199`	`199`	`for (const service of this._ngOnDestroyHooks) {`
`200`	`200`	`service.ngOnDestroy();`
`201`	`201`	`}`
`202`		`- for (const hook of this._onDestroyHooks) {`
	`202`	`+ const onDestroyHooks = this._onDestroyHooks;`
	`203`	`+ // Reset the _onDestroyHooks array before iterating over it to prevent hooks that unregister`
	`204`	`+ // themselves from mutating the array during iteration.`
	`205`	`+ this._onDestroyHooks = [];`
	`206`	`+ for (const hook of onDestroyHooks) {`
`203`	`207`	`hook();`
`204`	`208`	`}`
`205`	`209`	`} finally {`
`206`	`210`	`// Release all references.`
`207`	`211`	`this.records.clear();`
`208`	`212`	`this._ngOnDestroyHooks.clear();`
`209`	`213`	`this.injectorDefTypes.clear();`
`210`		`- this._onDestroyHooks.length = 0;`
`211`	`214`	`}`
`212`	`215`	`}`
`213`	`216`
Original file line number	Diff line number	Diff line change
`@@ -467,12 +467,14 @@ function processCleanups(tView: TView, lView: LView): void {`
`467`	`467`	`}`
`468`	`468`	`const destroyHooks = lView[ON_DESTROY_HOOKS];`
`469`	`469`	`if (destroyHooks !== null) {`
	`470`	`+ // Reset the ON_DESTROY_HOOKS array before iterating over it to prevent hooks that unregister`
	`471`	`+ // themselves from mutating the array during iteration.`
	`472`	`+ lView[ON_DESTROY_HOOKS] = null;`
`470`	`473`	`for (let i = 0; i < destroyHooks.length; i++) {`
`471`	`474`	`const destroyHooksFn = destroyHooks[i];`
`472`	`475`	`ngDevMode && assertFunction(destroyHooksFn, 'Expecting destroy hook to be a function.');`
`473`	`476`	`destroyHooksFn();`
`474`	`477`	`}`
`475`		`- lView[ON_DESTROY_HOOKS] = null;`
`476`	`478`	`}`
`477`	`479`	`}`
`478`	`480`