Skip to content

Commit 601010e

Browse files
Valentin Funkjamesdaniels
Valentin Funk
and
jamesdaniels
committed
fix: fix memory leak on SSR (#2243)
* chore: allow passsing Scheduler to fromRef This follows a convention from RxJS where observable creators allow passing a scheduler (e.g. fromPromise, of, empty, ...) Setting the default scheduler to asyncScheduler has the same effect as the delay(0) which is why it was removed. * fix: fix memory leak in SSR * fix(universal-test): bring back universal-test app * fix: use macrotask * fix: schedule completed and error notifications in the specified scheduler And add tests to verify behavior * chore: update misleading comment * fix(typings): don't type ZoneScheduler.delegate to support old TS versions * fix(keepUnstableUntilFirst): multicast only after rescheduling BREAKING: fromRef() observables are no longer multicasted by default * fix(keepUnstableUntilFirst): schedule emissions inside of angular synchronously, invoke zone task if unsubscribed before first emission Co-authored-by: James Daniels <[email protected]>
1 parent 0d95523 commit 601010e

36 files changed

+4681
-2027
lines changed

‎karma.conf.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ module.exports = function(config) {
1919
'node_modules/zone.js/dist/jasmine-patch.js',
2020
'node_modules/zone.js/dist/async-test.js',
2121
'node_modules/zone.js/dist/fake-async-test.js',
22+
'node_modules/zone.js/dist/task-tracking.js',
2223

2324
'node_modules/rxjs/bundles/rxjs.umd.{js,map}',
2425

‎package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
"test:watch": "concurrently \"npm run build:watch\" \"npm run delayed_karma\"",
1010
"test:debug": "npm run build && karma start",
1111
"karma": "karma start",
12-
"test:universal": "npm run build && cp -R dist/packages-dist test/universal-test/node_modules/angularfire2 && cd test/universal-test && npm run prerender",
12+
"test:universal": "npm run build && cd test/universal-test && yarn && npm run prerender",
1313
"delayed_karma": "sleep 10 && karma start",
1414
"build": "rimraf dist && node tools/build.js && npm pack ./dist/packages-dist",
1515
"changelog": "conventional-changelog -p angular -i CHANGELOG.md -s -r 1",

‎src/auth/auth.ts

Lines changed: 10 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { Injectable, Inject, Optional, NgZone, PLATFORM_ID } from '@angular/core';
22
import { Observable, of, from } from 'rxjs';
33
import { switchMap } from 'rxjs/operators';
4-
import { FIREBASE_OPTIONS, FIREBASE_APP_NAME, FirebaseOptions, FirebaseAppConfig, FirebaseAuth, _firebaseAppFactory, FirebaseZoneScheduler } from '@angular/fire';
4+
import { FIREBASE_OPTIONS, FIREBASE_APP_NAME, FirebaseOptions, FirebaseAppConfig, FirebaseAuth, _firebaseAppFactory, AngularFireSchedulers, keepUnstableUntilFirstFactory } from '@angular/fire';
55
import { User, auth } from 'firebase/app';
66

77
@Injectable()
@@ -38,31 +38,22 @@ export class AngularFireAuth {
3838
@Inject(FIREBASE_OPTIONS) options:FirebaseOptions,
3939
@Optional() @Inject(FIREBASE_APP_NAME) nameOrConfig:string|FirebaseAppConfig|null|undefined,
4040
@Inject(PLATFORM_ID) platformId: Object,
41-
private zone: NgZone
41+
zone: NgZone
4242
) {
43-
const scheduler = new FirebaseZoneScheduler(zone, platformId);
43+
const keepUnstableUntilFirst = keepUnstableUntilFirstFactory(new AngularFireSchedulers(zone), platformId);
44+
4445
this.auth = zone.runOutsideAngular(() => {
4546
const app = _firebaseAppFactory(options, zone, nameOrConfig);
4647
return app.auth();
4748
});
4849

49-
this.authState = scheduler.keepUnstableUntilFirst(
50-
scheduler.runOutsideAngular(
51-
new Observable(subscriber => {
52-
const unsubscribe = this.auth.onAuthStateChanged(subscriber);
53-
return { unsubscribe };
54-
})
55-
)
56-
);
50+
this.authState = new Observable<User | null>(subscriber => {
51+
return zone.runOutsideAngular(() => this.auth.onIdTokenChanged(subscriber));
52+
}).pipe(keepUnstableUntilFirst);;
5753

58-
this.user = scheduler.keepUnstableUntilFirst(
59-
scheduler.runOutsideAngular(
60-
new Observable(subscriber => {
61-
const unsubscribe = this.auth.onIdTokenChanged(subscriber);
62-
return { unsubscribe };
63-
})
64-
)
65-
);
54+
this.user = new Observable<User | null>(subscriber => {
55+
return zone.runOutsideAngular(() => this.auth.onIdTokenChanged(subscriber));
56+
}).pipe(keepUnstableUntilFirst);
6657

6758
this.idToken = this.user.pipe(switchMap(user => {
6859
return user ? from(user.getIdToken()) : of(null)

‎src/core/angularfire2.spec.ts

Lines changed: 196 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,14 @@
11
import { TestBed, inject } from '@angular/core/testing';
2-
import { PlatformRef, NgModule, CompilerFactory } from '@angular/core';
2+
import { PlatformRef, NgModule, CompilerFactory, NgZone } from '@angular/core';
33
import { FirebaseApp, AngularFireModule } from '@angular/fire';
4-
import { Subscription } from 'rxjs';
4+
import { Subscription, Observable, Subject, of } from 'rxjs';
55
import { COMMON_CONFIG } from './test-config';
66
import { BrowserModule } from '@angular/platform-browser';
77
import { database } from 'firebase/app';
8+
import { ZoneScheduler, keepUnstableUntilFirstFactory, AngularFireSchedulers } from './angularfire2';
9+
import { ɵPLATFORM_BROWSER_ID, ɵPLATFORM_SERVER_ID } from '@angular/common';
10+
import { tap } from 'rxjs/operators';
11+
import { TestScheduler } from 'rxjs/testing';
812

913
describe('angularfire', () => {
1014
let subscription:Subscription;
@@ -40,6 +44,196 @@ describe('angularfire', () => {
4044
app.delete().then(done, done.fail);
4145
});
4246

47+
describe('ZoneScheduler', () => {
48+
it('should execute the scheduled work inside the specified zone', done => {
49+
let ngZone = Zone.current.fork({
50+
name: 'ngZone'
51+
});
52+
const rootZone = Zone.current;
53+
54+
// Mimic real behavior: Executing in Angular
55+
ngZone.run(() => {
56+
const outsideAngularScheduler = new ZoneScheduler(rootZone);
57+
outsideAngularScheduler.schedule(() => {
58+
expect(Zone.current.name).not.toEqual('ngZone');
59+
done();
60+
});
61+
});
62+
});
63+
64+
it('should execute nested scheduled work inside the specified zone', done => {
65+
const testScheduler = new TestScheduler(null!);
66+
testScheduler.run(helpers => {
67+
const outsideAngularScheduler = new ZoneScheduler(Zone.current, testScheduler);
68+
69+
let ngZone = Zone.current.fork({
70+
name: 'ngZone'
71+
});
72+
73+
let callbacksRan = 0;
74+
75+
// Mimic real behavior: Executing in Angular
76+
ngZone.run(() => {
77+
outsideAngularScheduler.schedule(() => {
78+
callbacksRan++;
79+
expect(Zone.current.name).not.toEqual('ngZone');
80+
81+
ngZone.run(() => {
82+
// Sync queueing
83+
outsideAngularScheduler.schedule(() => {
84+
callbacksRan++;
85+
expect(Zone.current.name).not.toEqual('ngZone');
86+
});
87+
88+
// Async (10ms delay) nested scheduling
89+
outsideAngularScheduler.schedule(() => {
90+
callbacksRan++;
91+
expect(Zone.current.name).not.toEqual('ngZone');
92+
}, 10);
93+
94+
// Simulate flush from inside angular-
95+
helpers.flush();
96+
done();
97+
expect(callbacksRan).toEqual(3);
98+
})
99+
});
100+
helpers.flush();
101+
});
102+
});
103+
})
104+
})
105+
106+
describe('keepUnstableUntilFirstFactory', () => {
107+
let schedulers: AngularFireSchedulers;
108+
let outsideZone: Zone;
109+
let insideZone: Zone;
110+
beforeAll(() => {
111+
outsideZone = Zone.current;
112+
insideZone = Zone.current.fork({
113+
name: 'ngZone'
114+
});
115+
const ngZone = {
116+
run: insideZone.run.bind(insideZone),
117+
runGuarded: insideZone.runGuarded.bind(insideZone),
118+
runOutsideAngular: outsideZone.runGuarded.bind(outsideZone),
119+
runTask: insideZone.run.bind(insideZone)
120+
} as NgZone;
121+
schedulers = new AngularFireSchedulers(ngZone);
122+
})
123+
124+
it('should re-schedule emissions asynchronously', done => {
125+
const keepUnstableOp = keepUnstableUntilFirstFactory(schedulers, ɵPLATFORM_SERVER_ID);
126+
127+
let ran = false;
128+
of(null).pipe(
129+
keepUnstableOp,
130+
tap(() => ran = true)
131+
).subscribe(() => {
132+
expect(ran).toEqual(true);
133+
done();
134+
}, () => fail("Should not error"));
135+
136+
expect(ran).toEqual(false);
137+
});
138+
139+
[ɵPLATFORM_SERVER_ID, ɵPLATFORM_BROWSER_ID].map(platformId =>
140+
it(`should subscribe outside angular and observe inside angular (${platformId})`, done => {
141+
const keepUnstableOp = keepUnstableUntilFirstFactory(schedulers, platformId);
142+
143+
insideZone.run(() => {
144+
new Observable(s => {
145+
expect(Zone.current).toEqual(outsideZone);
146+
s.next("test");
147+
}).pipe(
148+
keepUnstableOp,
149+
tap(() => {
150+
expect(Zone.current).toEqual(insideZone);
151+
})
152+
).subscribe(() => {
153+
expect(Zone.current).toEqual(insideZone);
154+
done();
155+
}, err => {
156+
fail(err);
157+
});
158+
});
159+
})
160+
);
161+
162+
it('should block until first emission on server platform', done => {
163+
const testScheduler = new TestScheduler(null!);
164+
testScheduler.run(helpers => {
165+
const outsideZone = Zone.current;
166+
const taskTrack = new Zone['TaskTrackingZoneSpec']();
167+
const insideZone = Zone.current.fork(taskTrack);
168+
const trackingSchedulers: AngularFireSchedulers = {
169+
ngZone: {
170+
run: insideZone.run.bind(insideZone),
171+
runGuarded: insideZone.runGuarded.bind(insideZone),
172+
runOutsideAngular: outsideZone.runGuarded.bind(outsideZone),
173+
runTask: insideZone.run.bind(insideZone)
174+
} as NgZone,
175+
outsideAngular: new ZoneScheduler(outsideZone, testScheduler),
176+
insideAngular: new ZoneScheduler(insideZone, testScheduler),
177+
};
178+
const keepUnstableOp = keepUnstableUntilFirstFactory(trackingSchedulers, ɵPLATFORM_SERVER_ID);
179+
180+
const s = new Subject();
181+
s.pipe(
182+
keepUnstableOp,
183+
).subscribe(() => { }, err => { fail(err); }, () => { });
184+
185+
// Flush to ensure all async scheduled functions are run
186+
helpers.flush();
187+
// Should now be blocked until first item arrives
188+
expect(taskTrack.macroTasks.length).toBe(1);
189+
expect(taskTrack.macroTasks[0].source).toBe('firebaseZoneBlock');
190+
191+
// Emit next item
192+
s.next(123);
193+
helpers.flush();
194+
195+
// Should not be blocked after first item
196+
expect(taskTrack.macroTasks.length).toBe(0);
197+
198+
done();
199+
});
200+
})
201+
202+
it('should not block on client platform', done => {
203+
const testScheduler = new TestScheduler(null!);
204+
testScheduler.run(helpers => {
205+
const outsideZone = Zone.current;
206+
const taskTrack = new Zone['TaskTrackingZoneSpec']();
207+
const insideZone = Zone.current.fork(taskTrack);
208+
const trackingSchedulers: AngularFireSchedulers = {
209+
ngZone: {
210+
run: insideZone.run.bind(insideZone),
211+
runGuarded: insideZone.runGuarded.bind(insideZone),
212+
runOutsideAngular: outsideZone.runGuarded.bind(outsideZone),
213+
runTask: insideZone.run.bind(insideZone)
214+
} as NgZone,
215+
outsideAngular: new ZoneScheduler(outsideZone, testScheduler),
216+
insideAngular: new ZoneScheduler(insideZone, testScheduler),
217+
};
218+
const keepUnstableOp = keepUnstableUntilFirstFactory(trackingSchedulers, ɵPLATFORM_BROWSER_ID);
219+
220+
const s = new Subject();
221+
s.pipe(
222+
keepUnstableOp,
223+
).subscribe(() => { }, err => { fail(err); }, () => { });
224+
225+
// Flush to ensure all async scheduled functions are run
226+
helpers.flush();
227+
228+
// Zone should not be blocked
229+
expect(taskTrack.macroTasks.length).toBe(0);
230+
expect(taskTrack.microTasks.length).toBe(0);
231+
232+
done();
233+
});
234+
})
235+
})
236+
43237
describe('FirebaseApp', () => {
44238
it('should provide a FirebaseApp for the FirebaseApp binding', () => {
45239
expect(typeof app.delete).toBe('function');

0 commit comments

Comments
 (0)