Skip to content

Commit

Permalink
refactor(core): Update usage of pending tasks to make stability async
Browse files Browse the repository at this point in the history
This commit updates the public `ExperimentalPendingTasks` to remove
tasks asynchronously. This is true today for ZoneJS
already - it waits a microtask and checks the zone state again before
deciding it's stable. As we've continued to work with the pending tasks
API in various places, we've continued to encounter difficulty with the
synchronous handling of stability. This change will ensure that
synchronous code that executes after the last pending task is removed
can add another task and keep the application unstable without it
flipping first to stable (and thus causing SSR serialization too early).
  • Loading branch information
atscott committed Aug 5, 2024
1 parent 147eee4 commit dabfba1
Show file tree
Hide file tree
Showing 18 changed files with 124 additions and 107 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
DestroyRef,
ElementRef,
EnvironmentInjector,
ExperimentalPendingTasks,
inject,
Injector,
Input,
Expand All @@ -25,7 +26,6 @@ import {
Type,
ViewContainerRef,
ViewEncapsulation,
ɵPendingTasks as PendingTasks,
EventEmitter,
Output,
} from '@angular/core';
Expand Down Expand Up @@ -84,16 +84,16 @@ export class DocViewer implements OnChanges {

// tslint:disable-next-line:no-unused-variable
private animateContent = false;
private readonly pendingRenderTasks = inject(PendingTasks);
private readonly pendingTasks = inject(ExperimentalPendingTasks);

private countOfExamples = 0;

async ngOnChanges(changes: SimpleChanges): Promise<void> {
const taskId = this.pendingRenderTasks.add();
const removeTask = this.pendingTasks.add();
if ('docContent' in changes) {
await this.renderContentsAndRunClientSetup(this.docContent!);
}
this.pendingRenderTasks.remove(taskId);
removeTask();
}

async renderContentsAndRunClientSetup(content?: string): Promise<void> {
Expand Down
8 changes: 4 additions & 4 deletions packages/common/http/src/interceptor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
runInInjectionContext,
ɵConsole as Console,
ɵformatRuntimeError as formatRuntimeError,
ɵPendingTasks as PendingTasks,
ExperimentalPendingTasks,
} from '@angular/core';
import {Observable} from 'rxjs';
import {finalize} from 'rxjs/operators';
Expand Down Expand Up @@ -241,11 +241,11 @@ export function legacyInterceptorFnFactory(): HttpInterceptorFn {
);
}

const pendingTasks = inject(PendingTasks);
const pendingTasks = inject(ExperimentalPendingTasks);
const contributeToStability = inject(REQUESTS_CONTRIBUTE_TO_STABILITY);
if (contributeToStability) {
const taskId = pendingTasks.add();
return chain(req, handler).pipe(finalize(() => pendingTasks.remove(taskId)));
const removeTask = pendingTasks.add();
return chain(req, handler).pipe(finalize(() => removeTask()));
} else {
return chain(req, handler);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/application/application_ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import {ComponentFactory, ComponentRef} from '../linker/component_factory';
import {ComponentFactoryResolver} from '../linker/component_factory_resolver';
import {NgModuleRef} from '../linker/ng_module_factory';
import {ViewRef} from '../linker/view_ref';
import {PendingTasks} from '../pending_tasks';
import {PendingTasks} from './pending_tasks_internal';
import {RendererFactory2} from '../render/api';
import {AfterRenderEventManager} from '../render3/after_render_hooks';
import {ComponentFactory as R3ComponentFactory} from '../render3/component_ref';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,53 +6,10 @@
* found in the LICENSE file at https://angular.io/license
*/

import {BehaviorSubject} from 'rxjs';

import {inject} from './di/injector_compatibility';
import {ɵɵdefineInjectable} from './di/interface/defs';
import {OnDestroy} from './interface/lifecycle_hooks';

/**
* Internal implementation of the pending tasks service.
*/
export class PendingTasks implements OnDestroy {
private taskId = 0;
private pendingTasks = new Set<number>();
private get _hasPendingTasks() {
return this.hasPendingTasks.value;
}
hasPendingTasks = new BehaviorSubject<boolean>(false);

add(): number {
if (!this._hasPendingTasks) {
this.hasPendingTasks.next(true);
}
const taskId = this.taskId++;
this.pendingTasks.add(taskId);
return taskId;
}

remove(taskId: number): void {
this.pendingTasks.delete(taskId);
if (this.pendingTasks.size === 0 && this._hasPendingTasks) {
this.hasPendingTasks.next(false);
}
}

ngOnDestroy(): void {
this.pendingTasks.clear();
if (this._hasPendingTasks) {
this.hasPendingTasks.next(false);
}
}

/** @nocollapse */
static ɵprov = /** @pureOrBreakMyCode */ ɵɵdefineInjectable({
token: PendingTasks,
providedIn: 'root',
factory: () => new PendingTasks(),
});
}
import {inject} from '../di/injector_compatibility';
import {ɵɵdefineInjectable} from '../di/interface/defs';
import {PendingTasks} from './pending_tasks_internal';
import {NgZone} from '../zone/ng_zone';

/**
* Experimental service that keeps track of pending tasks contributing to the stableness of Angular
Expand Down Expand Up @@ -81,14 +38,22 @@ export class PendingTasks implements OnDestroy {
* @experimental
*/
export class ExperimentalPendingTasks {
private internalPendingTasks = inject(PendingTasks);
private readonly internalPendingTasks = inject(PendingTasks);
private readonly ngZone = inject(NgZone);

/**
* Adds a new task that should block application's stability.
* @returns A cleanup function that removes a task when called.
* @returns A cleanup function that removes a task after a microtask delay when called.
*/
add(): () => void {
const taskId = this.internalPendingTasks.add();
return () => this.internalPendingTasks.remove(taskId);
return () => {
this.ngZone.runOutsideAngular(() => {
setTimeout(() => {
this.internalPendingTasks.remove(taskId);
});
});
};
}

/** @nocollapse */
Expand Down
55 changes: 55 additions & 0 deletions packages/core/src/application/pending_tasks_internal.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/

import {BehaviorSubject} from 'rxjs';

import {inject} from '../di/injector_compatibility';
import {ɵɵdefineInjectable} from '../di/interface/defs';
import {OnDestroy} from '../interface/lifecycle_hooks';

/**
* Internal implementation of the pending tasks service.
*/
export class PendingTasks implements OnDestroy {
private taskId = 0;
private pendingTasks = new Set<number>();
private get _hasPendingTasks() {
return this.hasPendingTasks.value;
}
hasPendingTasks = new BehaviorSubject<boolean>(false);

add(): number {
if (!this._hasPendingTasks) {
this.hasPendingTasks.next(true);
}
const taskId = this.taskId++;
this.pendingTasks.add(taskId);
return taskId;
}

remove(taskId: number): void {
this.pendingTasks.delete(taskId);
if (this.pendingTasks.size === 0 && this._hasPendingTasks) {
this.hasPendingTasks.next(false);
}
}

ngOnDestroy(): void {
this.pendingTasks.clear();
if (this._hasPendingTasks) {
this.hasPendingTasks.next(false);
}
}

/** @nocollapse */
static ɵprov = /** @pureOrBreakMyCode */ ɵɵdefineInjectable({
token: PendingTasks,
providedIn: 'root',
factory: () => new PendingTasks(),
});
}
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ export class DebugNgZoneForCheckNoChanges extends NgZone {
this.onMicrotaskEmpty.emit = () => {};
this.onStable.emit = () => {
this.scheduler ||= this.injector.get(ChangeDetectionSchedulerImpl);
if (this.scheduler.pendingRenderTaskId || this.scheduler.runningTick) {
if (this.scheduler.removePendingTask || this.scheduler.runningTick) {
return;
}
this.checkApplicationViews();
Expand Down Expand Up @@ -146,7 +146,7 @@ function exhaustiveCheckNoChangesInterval(
if (applicationRef.destroyed) {
return;
}
if (scheduler.pendingRenderTaskId || scheduler.runningTick) {
if (scheduler.removePendingTask || scheduler.runningTick) {
scheduleCheckNoChanges();
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {
StaticProvider,
} from '../../di';
import {RuntimeError, RuntimeErrorCode} from '../../errors';
import {PendingTasks} from '../../pending_tasks';
import {PendingTasks} from '../../application/pending_tasks_internal';
import {performanceMarkFeature} from '../../util/performance';
import {NgZone} from '../../zone';
import {InternalNgZoneOptions} from '../../zone/ng_zone';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {inject} from '../../di/injector_compatibility';
import {EnvironmentProviders} from '../../di/interface/provider';
import {makeEnvironmentProviders} from '../../di/provider_collection';
import {RuntimeError, RuntimeErrorCode, formatRuntimeError} from '../../errors';
import {PendingTasks} from '../../pending_tasks';
import {ExperimentalPendingTasks} from '../../application/pending_tasks';
import {
scheduleCallbackWithMicrotask,
scheduleCallbackWithRafRace,
Expand Down Expand Up @@ -56,7 +56,7 @@ function trackMicrotaskNotificationForDebugging() {
@Injectable({providedIn: 'root'})
export class ChangeDetectionSchedulerImpl implements ChangeDetectionScheduler {
private readonly appRef = inject(ApplicationRef);
private readonly taskService = inject(PendingTasks);
private readonly taskService = inject(ExperimentalPendingTasks);
private readonly ngZone = inject(NgZone);
private readonly zonelessEnabled = inject(ZONELESS_ENABLED);
private readonly disableScheduling =
Expand All @@ -69,7 +69,7 @@ export class ChangeDetectionSchedulerImpl implements ChangeDetectionScheduler {
private shouldRefreshViews = false;
private useMicrotaskScheduler = false;
runningTick = false;
pendingRenderTaskId: number | null = null;
removePendingTask: (() => void) | null = null;

constructor() {
this.subscriptions.add(
Expand Down Expand Up @@ -152,7 +152,7 @@ export class ChangeDetectionSchedulerImpl implements ChangeDetectionScheduler {
const scheduleCallback = this.useMicrotaskScheduler
? scheduleCallbackWithMicrotask
: scheduleCallbackWithRafRace;
this.pendingRenderTaskId = this.taskService.add();
this.removePendingTask = this.taskService.add();
if (this.zoneIsDefined) {
Zone.root.run(() => {
this.cancelScheduledCallback = scheduleCallback(() => {
Expand All @@ -171,7 +171,7 @@ export class ChangeDetectionSchedulerImpl implements ChangeDetectionScheduler {
return false;
}
// already scheduled or running
if (this.pendingRenderTaskId !== null || this.runningTick || this.appRef._runningTick) {
if (this.removePendingTask !== null || this.runningTick || this.appRef._runningTick) {
return false;
}
// If we're inside the zone don't bother with scheduler. Zone will stabilize
Expand Down Expand Up @@ -200,7 +200,6 @@ export class ChangeDetectionSchedulerImpl implements ChangeDetectionScheduler {
return;
}

const task = this.taskService.add();
try {
this.ngZone.run(
() => {
Expand All @@ -210,9 +209,6 @@ export class ChangeDetectionSchedulerImpl implements ChangeDetectionScheduler {
undefined,
this.schedulerTickApplyArgs,
);
} catch (e: unknown) {
this.taskService.remove(task);
throw e;
} finally {
this.cleanup();
}
Expand All @@ -224,7 +220,6 @@ export class ChangeDetectionSchedulerImpl implements ChangeDetectionScheduler {
this.useMicrotaskScheduler = true;
scheduleCallbackWithMicrotask(() => {
this.useMicrotaskScheduler = false;
this.taskService.remove(task);
});
}

Expand All @@ -245,10 +240,9 @@ export class ChangeDetectionSchedulerImpl implements ChangeDetectionScheduler {
// before removing it from the pending tasks (or the tasks service should
// not synchronously emit stable, similar to how Zone stableness only
// happens if it's still stable after a microtask).
if (this.pendingRenderTaskId !== null) {
const taskId = this.pendingRenderTaskId;
this.pendingRenderTaskId = null;
this.taskService.remove(taskId);
if (this.removePendingTask !== null) {
this.removePendingTask();
this.removePendingTask = null;
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export {
NgZoneOptions,
} from './change_detection/scheduling/ng_zone_scheduling';
export {provideExperimentalZonelessChangeDetection} from './change_detection/scheduling/zoneless_scheduling_impl';
export {ExperimentalPendingTasks} from './pending_tasks';
export {ExperimentalPendingTasks} from './application/pending_tasks';
export {provideExperimentalCheckNoChangesForDebug} from './change_detection/scheduling/exhaustive_check_no_changes';
export {enableProdMode, isDevMode} from './util/is_dev_mode';
export {
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/core_private_export.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ export {
resolveComponentResources as ɵresolveComponentResources,
restoreComponentResolutionQueue as ɵrestoreComponentResolutionQueue,
} from './metadata/resource_loading';
export {PendingTasks as ɵPendingTasks} from './pending_tasks';
export {PendingTasks as ɵPendingTasks} from './application/pending_tasks_internal';
export {ALLOW_MULTIPLE_PLATFORMS as ɵALLOW_MULTIPLE_PLATFORMS} from './platform/platform';
export {ReflectionCapabilities as ɵReflectionCapabilities} from './reflection/reflection_capabilities';
export {AnimationRendererType as ɵAnimationRendererType} from './render/api';
Expand Down
10 changes: 5 additions & 5 deletions packages/core/src/defer/instructions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {internalImportProvidersFrom} from '../di/provider_collection';
import {RuntimeError, RuntimeErrorCode} from '../errors';
import {findMatchingDehydratedView} from '../hydration/views';
import {populateDehydratedViewsInLContainer} from '../linker/view_container_ref';
import {PendingTasks} from '../pending_tasks';
import {ExperimentalPendingTasks} from '../application/pending_tasks';
import {assertLContainer, assertTNodeForLView} from '../render3/assert';
import {bindingUpdated} from '../render3/bindings';
import {ChainedInjector} from '../render3/chained_injector';
Expand Down Expand Up @@ -900,8 +900,8 @@ export function triggerResourceLoading(
}

// Indicate that an application is not stable and has a pending task.
const pendingTasks = injector.get(PendingTasks);
const taskId = pendingTasks.add();
const pendingTasks = injector.get(ExperimentalPendingTasks);
const removeTask = pendingTasks.add();

// The `dependenciesFn` might be `null` when all dependencies within
// a given defer block were eagerly referenced elsewhere in a file,
Expand All @@ -910,7 +910,7 @@ export function triggerResourceLoading(
tDetails.loadingPromise = Promise.resolve().then(() => {
tDetails.loadingPromise = null;
tDetails.loadingState = DeferDependenciesLoadingState.COMPLETE;
pendingTasks.remove(taskId);
removeTask();
});
return tDetails.loadingPromise;
}
Expand Down Expand Up @@ -942,7 +942,7 @@ export function triggerResourceLoading(
// Loading is completed, we no longer need the loading Promise
// and a pending task should also be removed.
tDetails.loadingPromise = null;
pendingTasks.remove(taskId);
pendingTasks.remove(removeTask);

if (failed) {
tDetails.loadingState = DeferDependenciesLoadingState.FAILED;
Expand Down
6 changes: 5 additions & 1 deletion packages/core/src/event_emitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {OutputRef} from './authoring/output/output_ref';
import {isInInjectionContext} from './di/contextual';
import {inject} from './di/injector_compatibility';
import {DestroyRef} from './linker/destroy_ref';
import {PendingTasks} from './pending_tasks';
import {PendingTasks} from './application/pending_tasks_internal';

/**
* Use in components with the `@Output` directive to emit custom events
Expand Down Expand Up @@ -112,6 +112,10 @@ export interface EventEmitter<T> extends Subject<T>, OutputRef<T> {
class EventEmitter_ extends Subject<any> implements OutputRef<any> {
__isAsync: boolean; // tslint:disable-line
destroyRef: DestroyRef | undefined = undefined;
// TODO: We can't use the public pending tasks here because it depends on NgZone to run the timeout outside NgZone
// NgZone depends on EventEmitter so that's a ciruclar dep. NgZone should not use EventEmitter and we should use
// the public pending task API here (and/or update the internal pending task API to have a removeAsync option
// that remvoes the taskId in a setTimeout outside the Angular Zone).
private readonly pendingTasks: PendingTasks | undefined = undefined;

constructor(isAsync: boolean = false) {
Expand Down
Loading

0 comments on commit dabfba1

Please sign in to comment.