refactor(core): update effect error handling

Previously, effect() would handle errors differently depending on the effect
type. Root effects had a try/catch that would execute them independently and
report errors to `ErrorHandler`, while component effects would "crash" CD.

This commit switches all effects to use the same error handling (errors
always reach the CD error handler).

An additional unrelated refactoring is thrown in which removes the
`pendingTask` machinery from root effects, since they make `ApplicationRef`
dirty and thus trigger the scheduler.

packages/core/src/change_detection/scheduling/zoneless_scheduling.ts

@@ -52,6 +52,8 @@ export const enum NotificationSource {
   PendingTaskRemoved,
   // An `effect()` outside of the view tree became dirty and might need to run.
   RootEffect,
+  // An `effect()` within the view tree became dirty.
+  ViewEffect,
 }
 
 /**

packages/core/src/change_detection/scheduling/zoneless_scheduling_impl.ts

@@ -155,6 +155,20 @@ export class ChangeDetectionSchedulerImpl implements ChangeDetectionScheduler {
       }
       case NotificationSource.RootEffect: {
         this.appRef.dirtyFlags |= ApplicationRefDirtyFlags.RootEffects;
+        // Root effects still force a CD, even if the scheduler is disabled. This ensures that
+        // effects always run, even when triggered from outside the zone when the scheduler is
+        // otherwise disabled.
+        force = true;
+        break;
+      }
+      case NotificationSource.ViewEffect: {
+        // This is technically a no-op, since view effects will also send a
+        // `MarkAncestorsForTraversal` notification. Still, we set this for logical consistency.
+        this.appRef.dirtyFlags |= ApplicationRefDirtyFlags.ViewTreeTraversal;
+        // View effects still force a CD, even if the scheduler is disabled. This ensures that
+        // effects always run, even when triggered from outside the zone when the scheduler is
+        // otherwise disabled.
+        force = true;
         break;
       }
       case NotificationSource.PendingTaskRemoved: {

packages/core/src/render3/reactivity/effect.ts

@@ -174,21 +174,18 @@ export function effect(
   let node: EffectNode;
 
   const viewContext = injector.get(ViewContext, null, {optional: true});
+  const notifier = injector.get(ChangeDetectionScheduler);
   if (viewContext !== null && !options?.forceRoot) {
     // This effect was created in the context of a view, and will be associated with the view.
-    node = createViewEffect(viewContext.view, effectFn);
+    node = createViewEffect(viewContext.view, notifier, effectFn);
     if (destroyRef instanceof NodeInjectorDestroyRef && destroyRef._lView === viewContext.view) {
       // The effect is being created in the same view as the `DestroyRef` references, so it will be
       // automatically destroyed without the need for an explicit `DestroyRef` registration.
       destroyRef = null;
     }
   } else {
     // This effect was created outside the context of a view, and will be scheduled independently.
-    node = createRootEffect(
-      effectFn,
-      injector.get(EffectScheduler),
-      injector.get(ChangeDetectionScheduler),
-    );
+    node = createRootEffect(effectFn, injector.get(EffectScheduler), notifier);
   }
   node.injector = injector;
 
@@ -204,6 +201,7 @@ export interface EffectNode extends ReactiveNode, SchedulableEffect {
   hasRun: boolean;
   cleanupFns: EffectCleanupFn[] | undefined;
   injector: Injector;
+  notifier: ChangeDetectionScheduler;
 
   onDestroyFn: () => void;
   fn: (cleanupFn: EffectCleanupRegisterFn) => void;
@@ -218,7 +216,6 @@ export interface ViewEffectNode extends EffectNode {
 
 export interface RootEffectNode extends EffectNode {
   scheduler: EffectScheduler;
-  notifier: ChangeDetectionScheduler;
 }
 
 /**
@@ -230,7 +227,7 @@ export const APP_EFFECT_SCHEDULER = new InjectionToken('', {
   factory: () => inject(EffectScheduler),
 });
 
-export const BASE_EFFECT_NODE: Omit<EffectNode, 'fn' | 'destroy' | 'injector'> =
+export const BASE_EFFECT_NODE: Omit<EffectNode, 'fn' | 'destroy' | 'injector' | 'notifier'> =
   /* @__PURE__ */ (() => ({
     ...REACTIVE_NODE,
     consumerIsAlwaysLive: true,
@@ -263,19 +260,25 @@ export const BASE_EFFECT_NODE: Omit<EffectNode, 'fn' | 'destroy' | 'injector'> =
       try {
         this.maybeCleanup();
         this.fn(registerCleanupFn);
-      } catch (err: unknown) {
-        // We inject the error handler lazily, to prevent circular dependencies when an effect is
-        // created inside of an ErrorHandler.
-        this.injector.get(ErrorHandler, null, {optional: true})?.handleError(err);
       } finally {
         setIsRefreshingViews(prevRefreshingViews);
         consumerAfterComputation(this, prevNode);
       }
     },
 
     maybeCleanup(this: EffectNode): void {
-      while (this.cleanupFns?.length) {
-        this.cleanupFns.pop()!();
+      if (!this.cleanupFns?.length) {
+        return;
+      }
+      try {
+        // Attempt to run the cleanup functions. Regardless of failure or success, we consider
+        // cleanup "completed" and clear the list for the next run of the effect. Note that an error
+        // from the cleanup function will still crash the current run of the effect.
+        while (this.cleanupFns.length) {
+          this.cleanupFns.pop()!();
+        }
+      } finally {
+        this.cleanupFns = [];
       }
     },
   }))();
@@ -294,12 +297,13 @@ export const ROOT_EFFECT_NODE: Omit<RootEffectNode, 'fn' | 'scheduler' | 'notifi
     },
   }))();
 
-export const VIEW_EFFECT_NODE: Omit<ViewEffectNode, 'fn' | 'view' | 'injector'> =
+export const VIEW_EFFECT_NODE: Omit<ViewEffectNode, 'fn' | 'view' | 'injector' | 'notifier'> =
   /* @__PURE__ */ (() => ({
     ...BASE_EFFECT_NODE,
     consumerMarkedDirty(this: ViewEffectNode): void {
       this.view[FLAGS] |= LViewFlags.HasChildViewsToRefresh;
       markAncestorsForTraversal(this.view);
+      this.notifier.notify(NotificationSource.ViewEffect);
     },
     destroy(this: ViewEffectNode): void {
       consumerDestroy(this);
@@ -311,11 +315,13 @@ export const VIEW_EFFECT_NODE: Omit<ViewEffectNode, 'fn' | 'view' | 'injector'>
 
 export function createViewEffect(
   view: LView,
+  notifier: ChangeDetectionScheduler,
   fn: (onCleanup: EffectCleanupRegisterFn) => void,
 ): ViewEffectNode {
   const node = Object.create(VIEW_EFFECT_NODE) as ViewEffectNode;
   node.view = view;
   node.zone = typeof Zone !== 'undefined' ? Zone.current : null;
+  node.notifier = notifier;
   node.fn = fn;
 
   view[EFFECTS] ??= new Set();

packages/core/src/render3/reactivity/microtask_effect.ts

@@ -22,17 +22,32 @@ import {type SchedulableEffect, ZoneAwareEffectScheduler} from './root_effect_sc
 import {performanceMarkFeature} from '../../util/performance';
 import {assertNotInReactiveContext} from './asserts';
 import {assertInInjectionContext} from '../../di';
+import {PendingTasksInternal} from '../../pending_tasks';
 
 export class MicrotaskEffectScheduler extends ZoneAwareEffectScheduler {
+  private readonly pendingTasks = inject(PendingTasksInternal);
+  private taskId: number | null = null;
+
   override schedule(effect: SchedulableEffect): void {
     // Check whether there are any pending effects _before_ queueing in the base class.
-    const needsScheduling = this.taskId === null;
     super.schedule(effect);
-    if (needsScheduling) {
+    if (this.taskId === null) {
+      this.taskId = this.pendingTasks.add();
       queueMicrotask(() => this.flush());
     }
   }
 
+  override flush(): void {
+    try {
+      super.flush();
+    } finally {
+      if (this.taskId !== null) {
+        this.pendingTasks.remove(this.taskId);
+        this.taskId = null;
+      }
+    }
+  }
+
   /** @nocollapse */
   static ɵprov = /** @pureOrBreakMyCode */ ɵɵdefineInjectable({
     token: MicrotaskEffectScheduler,

packages/core/src/render3/reactivity/root_effect_scheduler.ts

@@ -51,15 +51,9 @@ export abstract class EffectScheduler {
 export class ZoneAwareEffectScheduler implements EffectScheduler {
   private queuedEffectCount = 0;
   private queues = new Map<Zone | null, Set<SchedulableEffect>>();
-  private readonly pendingTasks = inject(PendingTasksInternal);
-  protected taskId: number | null = null;
 
   schedule(handle: SchedulableEffect): void {
     this.enqueue(handle);
-
-    if (this.taskId === null) {
-      this.taskId = this.pendingTasks.add();
-    }
   }
 
   private enqueue(handle: SchedulableEffect): void {
@@ -93,11 +87,6 @@ export class ZoneAwareEffectScheduler implements EffectScheduler {
         }
       }
     }
-
-    if (this.taskId !== null) {
-      this.pendingTasks.remove(this.taskId);
-      this.taskId = null;
-    }
   }
 
   private flushQueue(queue: Set<SchedulableEffect>): void {

packages/core/test/render3/reactivity_spec.ts

@@ -101,6 +101,11 @@ describe('reactivity', () => {
     });
 
     it('should propagate errors to the ErrorHandler', () => {
+      TestBed.configureTestingModule({
+        providers: [{provide: ErrorHandler, useFactory: () => new FakeErrorHandler()}],
+        rethrowApplicationErrors: false,
+      });
+
       let run = false;
 
       let lastError: any = null;
@@ -109,24 +114,26 @@ describe('reactivity', () => {
           lastError = error;
         }
       }
-
-      const injector = createEnvironmentInjector(
-        [{provide: ErrorHandler, useFactory: () => new FakeErrorHandler()}],
-        TestBed.inject(EnvironmentInjector),
-      );
+      const appRef = TestBed.inject(ApplicationRef);
       effect(
         () => {
           run = true;
           throw new Error('fail!');
         },
-        {injector},
+        {injector: appRef.injector},
       );
-      expect(() => TestBed.flushEffects()).not.toThrow();
+      appRef.tick();
       expect(run).toBeTrue();
       expect(lastError.message).toBe('fail!');
     });
 
-    it('should be usable inside an ErrorHandler', async () => {
+    // Disabled while we consider whether this actually makes sense.
+    // This test _used_ to show that `effect()` was usable inside component error handlers, partly
+    // because effect errors used to report to component error handlers. Now, effect errors are
+    // always reported to the top-level error handler, which has never been able to use `effect()`
+    // as `effect()` depends transitively on `ApplicationRef` which depends circularly on
+    // `ErrorHandler`.
+    xit('should be usable inside an ErrorHandler', async () => {
       const shouldError = signal(false);
       let lastError: any = null;
 
@@ -145,24 +152,16 @@ describe('reactivity', () => {
         }
       }
 
-      @Component({
-        standalone: true,
-        template: '',
+      TestBed.configureTestingModule({
         providers: [{provide: ErrorHandler, useClass: FakeErrorHandler}],
-      })
-      class App {
-        errorHandler = inject(ErrorHandler);
-      }
-
-      const fixture = TestBed.createComponent(App);
-      fixture.detectChanges();
+        rethrowApplicationErrors: false,
+      });
 
-      expect(fixture.componentInstance.errorHandler).toBeInstanceOf(FakeErrorHandler);
-      expect(lastError).toBe(null);
+      const appRef = TestBed.inject(ApplicationRef);
+      expect(() => appRef.tick()).not.toThrow();
 
       shouldError.set(true);
-      fixture.detectChanges();
-
+      expect(() => appRef.tick()).not.toThrow();
       expect(lastError?.message).toBe('fail!');
     });