fix(core): Ensure that a destroyed effect never run. (#59415)

JeanMeche · thePunderWoman · commit 5c0d68804e03 · 2025-01-08T16:40:44.000Z
Prior to this change, a scheduled root effect, even if destroyed instantly, would still run at least once. This commit fixes this. fixes #59410 PR Close #59415
diff --git a/goldens/size-tracking/integration-payloads.json b/goldens/size-tracking/integration-payloads.json
@@ -20,7 +20,7 @@
   },
   "forms": {
     "uncompressed": {
-      "main": 176726,
+      "main": 181773,
       "polyfills": 33772
     }
   },
diff --git a/packages/core/src/render3/reactivity/effect.ts b/packages/core/src/render3/reactivity/effect.ts
@@ -312,6 +312,7 @@ export const ROOT_EFFECT_NODE: Omit<RootEffectNode, 'fn' | 'scheduler' | 'notifi
       consumerDestroy(this);
       this.onDestroyFn();
       this.maybeCleanup();
+      this.scheduler.remove(this);
     },
   }))();
 
diff --git a/packages/core/src/render3/reactivity/root_effect_scheduler.ts b/packages/core/src/render3/reactivity/root_effect_scheduler.ts
@@ -36,6 +36,9 @@ export abstract class EffectScheduler {
    */
   abstract flush(): void;
 
+  /** Remove a scheduled effect */
+  abstract remove(e: SchedulableEffect): void;
+
   /** @nocollapse */
   static ɵprov = /** @pureOrBreakMyCode */ /* @__PURE__ */ ɵɵdefineInjectable({
     token: EffectScheduler,
@@ -56,6 +59,17 @@ export class ZoneAwareEffectScheduler implements EffectScheduler {
     this.enqueue(handle);
   }
 
+  remove(handle: SchedulableEffect): void {
+    const zone = handle.zone as Zone | null;
+    const queue = this.queues.get(zone)!;
+    if (!queue.has(handle)) {
+      return;
+    }
+
+    queue.delete(handle);
+    this.queuedEffectCount--;
+  }
+
   private enqueue(handle: SchedulableEffect): void {
     const zone = handle.zone as Zone | null;
     if (!this.queues.has(zone)) {
diff --git a/packages/core/test/render3/reactivity_spec.ts b/packages/core/test/render3/reactivity_spec.ts
@@ -16,7 +16,6 @@ import {
   computed,
   ContentChildren,
   createComponent,
-  createEnvironmentInjector,
   destroyPlatform,
   Directive,
   effect,
@@ -37,16 +36,14 @@ import {
   ViewContainerRef,
 } from '@angular/core';
 import {SIGNAL} from '@angular/core/primitives/signals';
-import {takeUntilDestroyed, toObservable} from '@angular/core/rxjs-interop';
-import {createInjector} from '@angular/core/src/di/create_injector';
+import {toObservable} from '@angular/core/rxjs-interop';
 import {
   EffectNode,
   setUseMicrotaskEffectsByDefault,
 } from '@angular/core/src/render3/reactivity/effect';
 import {TestBed} from '@angular/core/testing';
 import {bootstrapApplication} from '@angular/platform-browser';
 import {withBody} from '@angular/private/testing';
-import {filter, firstValueFrom, map} from 'rxjs';
 
 describe('reactivity', () => {
   let prev: boolean;
@@ -487,6 +484,53 @@ describe('reactivity', () => {
         (fix.componentInstance.injector as Injector & {destroy(): void}).destroy();
         expect(destroyed).toBeTrue();
       });
+
+      it('should not run root effects after it has been destroyed', async () => {
+        let effectCounter = 0;
+        const counter = signal(1);
+        const effectRef = TestBed.runInInjectionContext(() =>
+          effect(
+            () => {
+              counter();
+              effectCounter++;
+            },
+            {injector: TestBed.inject(EnvironmentInjector)},
+          ),
+        );
+        expect(effectCounter).toBe(0);
+        effectRef.destroy();
+        TestBed.flushEffects();
+        expect(effectCounter).toBe(0);
+
+        counter.set(2);
+        TestBed.flushEffects();
+        expect(effectCounter).toBe(0);
+      });
+
+      it('should not run view effects after it has been destroyed', async () => {
+        let effectCounter = 0;
+
+        @Component({template: ''})
+        class TestCmp {
+          counter = signal(1);
+          effectRef = effect(() => {
+            this.counter();
+            effectCounter++;
+          });
+        }
+
+        const fixture = TestBed.createComponent(TestCmp);
+        fixture.componentInstance.effectRef.destroy();
+        fixture.detectChanges();
+        expect(effectCounter).toBe(0);
+
+        TestBed.flushEffects();
+        expect(effectCounter).toBe(0);
+
+        fixture.componentInstance.counter.set(2);
+        TestBed.flushEffects();
+        expect(effectCounter).toBe(0);
+      });
     });
   });
 

Original file line number	Diff line number	Diff line change
`@@ -20,7 +20,7 @@`
`20`	`20`	`},`
`21`	`21`	`"forms": {`
`22`	`22`	`"uncompressed": {`
`23`		`- "main": 176726,`
	`23`	`+ "main": 181773,`
`24`	`24`	`"polyfills": 33772`
`25`	`25`	`}`
`26`	`26`	`},`