Skip to content

Commit efbab16

Browse files
shoutoutuoadi325thePunderWoman
authored andcommitted
fix(core): cancel obsolete resource loads when params become unavailable
Cancel previous loads before returning for unavailable or errored resource params. These transitions otherwise leave the old AbortSignal active and keep its pending task blocking application stability. Preserve pending-task ownership when cancelled promises settle after a new request starts. Cover unavailable params, propagated statuses and errors, and late resolution or rejection in regression tests.
1 parent b906324 commit efbab16

3 files changed

Lines changed: 110 additions & 3 deletions

File tree

‎packages/core/src/resource/resource.ts‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -434,6 +434,10 @@ export class ResourceImpl<T, R> extends BaseWritableResource<T> implements Resou
434434
// we do not want the effect to depend on the state of the resource, only on the request.
435435
const {status: currentStatus, previousStatus} = untracked(this.state);
436436

437+
// A changed request invalidates the previous load even when the new params are unavailable
438+
// or errored. Cancel it before returning so its pending task cannot keep the app unstable.
439+
this.abortInProgressLoad();
440+
437441
if (extRequest.request === undefined) {
438442
// Nothing to load (and we should already be in a non-loading state).
439443
return;
@@ -442,9 +446,6 @@ export class ResourceImpl<T, R> extends BaseWritableResource<T> implements Resou
442446
return;
443447
}
444448

445-
// Cancel any previous loading attempts.
446-
this.abortInProgressLoad();
447-
448449
// Capturing _this_ load's pending task in a local variable is important here. We may attempt to
449450
// resolve it twice:
450451
//

‎packages/core/test/resource/params_status_spec.ts‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
* found in the LICENSE file at https://angular.dev/license
77
*/
88

9+
import {timeout} from '@angular/private/testing';
10+
911
import {ApplicationRef, Injector, resource, ResourceParamsStatus, signal} from '../../src/core';
1012
import {TestBed} from '../../testing';
1113

@@ -19,6 +21,42 @@ function throwStatusAndErrors<T>(source: () => T | ResourceParamsStatus | Error)
1921
}
2022

2123
describe('resource with ResourceParamsStatus', () => {
24+
for (const [paramsStatus, expectedStatus] of [
25+
[ResourceParamsStatus.IDLE, 'idle'],
26+
[ResourceParamsStatus.LOADING, 'loading'],
27+
[new Error('params error'), 'error'],
28+
] as const) {
29+
it(`should cancel a pending load and release stability when params throws ${expectedStatus}`, async () => {
30+
const params = signal<string | ResourceParamsStatus | Error>('first');
31+
const abortSignals: AbortSignal[] = [];
32+
const res = resource({
33+
params: throwStatusAndErrors(params),
34+
loader: ({params, abortSignal}) => {
35+
abortSignals.push(abortSignal);
36+
// The first load deliberately remains pending even after it is aborted.
37+
return params === 'first' ? new Promise<string>(() => {}) : Promise.resolve(params);
38+
},
39+
injector: TestBed.inject(Injector),
40+
});
41+
const appRef = TestBed.inject(ApplicationRef);
42+
43+
await timeout();
44+
expect(abortSignals[0]?.aborted).toBeFalse();
45+
46+
params.set(paramsStatus);
47+
await appRef.whenStable();
48+
49+
expect(res.status()).toBe(expectedStatus);
50+
expect(abortSignals.map((signal) => signal.aborted)).toEqual([true]);
51+
52+
params.set('second');
53+
await appRef.whenStable();
54+
55+
expect(res.snapshot()).toEqual({status: 'resolved', value: 'second'});
56+
expect(abortSignals.map((signal) => signal.aborted)).toEqual([true, false]);
57+
});
58+
}
59+
2260
it('should transition to idle when params throws ResourceParamsStatus.IDLE', async () => {
2361
const s = signal<string | ResourceParamsStatus>('foo');
2462
const res = await act(() =>

‎packages/core/test/resource/resource_spec.ts‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
* found in the LICENSE file at https://angular.dev/license
77
*/
88

9+
import {timeout} from '@angular/private/testing';
10+
911
import {
1012
ApplicationRef,
1113
ɵCACHE_ACTIVE as CACHE_ACTIVE,
@@ -462,6 +464,72 @@ describe('resource', () => {
462464
expect(aborted).toEqual([{counter: 0}]);
463465
});
464466

467+
it('should cancel a pending load and release stability when params becomes undefined', async () => {
468+
const params = signal<number | undefined>(1);
469+
let abortSignal: AbortSignal | undefined;
470+
const res = resource({
471+
params,
472+
loader: (options) => {
473+
abortSignal = options.abortSignal;
474+
// Aborting a loader does not require its promise to settle.
475+
return new Promise<number>(() => {});
476+
},
477+
injector: TestBed.inject(Injector),
478+
});
479+
const appRef = TestBed.inject(ApplicationRef);
480+
481+
await timeout();
482+
expect(abortSignal?.aborted).toBeFalse();
483+
484+
params.set(undefined);
485+
await appRef.whenStable();
486+
487+
expect(abortSignal?.aborted).toBeTrue();
488+
expect(res.snapshot()).toEqual({status: 'idle', value: undefined});
489+
});
490+
491+
for (const settlement of ['resolve', 'reject'] as const) {
492+
it(`should discard a cancelled load's late ${settlement} without releasing a newer load's pending task`, async () => {
493+
const params = signal<number | undefined>(1);
494+
const first = promiseWithResolvers<number>();
495+
const second = promiseWithResolvers<number>();
496+
const abortSignals: AbortSignal[] = [];
497+
const res = resource({
498+
params,
499+
loader: ({params, abortSignal}) => {
500+
abortSignals.push(abortSignal);
501+
return params === 1 ? first.promise : second.promise;
502+
},
503+
injector: TestBed.inject(Injector),
504+
});
505+
const appRef = TestBed.inject(ApplicationRef);
506+
507+
await timeout();
508+
params.set(undefined);
509+
await appRef.whenStable();
510+
511+
params.set(2);
512+
await timeout();
513+
514+
let stable = false;
515+
const whenStable = appRef.whenStable().then(() => (stable = true));
516+
if (settlement === 'resolve') {
517+
first.resolve(1);
518+
} else {
519+
first.reject(new Error('cancelled load'));
520+
}
521+
await timeout();
522+
523+
expect(res.snapshot()).toEqual({status: 'loading', value: undefined});
524+
expect(abortSignals.map((signal) => signal.aborted)).toEqual([true, false]);
525+
expect(stable).toBeFalse();
526+
527+
second.resolve(2);
528+
await whenStable;
529+
expect(res.snapshot()).toEqual({status: 'resolved', value: 2});
530+
});
531+
}
532+
465533
it('should cancel pending requests when the resource is destroyed via injector', async () => {
466534
const counter = signal(0);
467535
const backend = new MockEchoBackend<{counter: number}>();

0 commit comments

Comments
 (0)