Skip to content

fix: fix memory leak on SSR #2243

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Jan 21, 2020
1 change: 1 addition & 0 deletions karma.conf.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ module.exports = function(config) {
'node_modules/zone.js/dist/jasmine-patch.js',
'node_modules/zone.js/dist/async-test.js',
'node_modules/zone.js/dist/fake-async-test.js',
'node_modules/zone.js/dist/task-tracking.js',

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

Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"test:watch": "concurrently \"npm run build:watch\" \"npm run delayed_karma\"",
"test:debug": "npm run build && karma start",
"karma": "karma start",
"test:universal": "npm run build && cp -R dist/packages-dist test/universal-test/node_modules/angularfire2 && cd test/universal-test && npm run prerender",
"test:universal": "npm run build && cd test/universal-test && yarn && npm run prerender",
"delayed_karma": "sleep 10 && karma start",
"build": "rimraf dist && node tools/build.js && npm pack ./dist/packages-dist",
"changelog": "conventional-changelog -p angular -i CHANGELOG.md -s -r 1",
Expand Down
29 changes: 10 additions & 19 deletions src/auth/auth.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Injectable, Inject, Optional, NgZone, PLATFORM_ID } from '@angular/core';
import { Observable, of, from } from 'rxjs';
import { switchMap } from 'rxjs/operators';
import { FIREBASE_OPTIONS, FIREBASE_APP_NAME, FirebaseOptions, FirebaseAppConfig, FirebaseAuth, _firebaseAppFactory, FirebaseZoneScheduler } from '@angular/fire';
import { FIREBASE_OPTIONS, FIREBASE_APP_NAME, FirebaseOptions, FirebaseAppConfig, FirebaseAuth, _firebaseAppFactory, AngularFireSchedulers, keepUnstableUntilFirstFactory } from '@angular/fire';
import { User, auth } from 'firebase/app';

@Injectable()
Expand Down Expand Up @@ -38,31 +38,22 @@ export class AngularFireAuth {
@Inject(FIREBASE_OPTIONS) options:FirebaseOptions,
@Optional() @Inject(FIREBASE_APP_NAME) nameOrConfig:string|FirebaseAppConfig|null|undefined,
@Inject(PLATFORM_ID) platformId: Object,
private zone: NgZone
zone: NgZone
) {
const scheduler = new FirebaseZoneScheduler(zone, platformId);
const keepUnstableUntilFirst = keepUnstableUntilFirstFactory(new AngularFireSchedulers(zone), platformId);

this.auth = zone.runOutsideAngular(() => {
const app = _firebaseAppFactory(options, zone, nameOrConfig);
return app.auth();
});

this.authState = scheduler.keepUnstableUntilFirst(
scheduler.runOutsideAngular(
new Observable(subscriber => {
const unsubscribe = this.auth.onAuthStateChanged(subscriber);
return { unsubscribe };
})
)
);
this.authState = new Observable<User | null>(subscriber => {
return zone.runOutsideAngular(() => this.auth.onIdTokenChanged(subscriber));
}).pipe(keepUnstableUntilFirst);;

this.user = scheduler.keepUnstableUntilFirst(
scheduler.runOutsideAngular(
new Observable(subscriber => {
const unsubscribe = this.auth.onIdTokenChanged(subscriber);
return { unsubscribe };
})
)
);
this.user = new Observable<User | null>(subscriber => {
return zone.runOutsideAngular(() => this.auth.onIdTokenChanged(subscriber));
}).pipe(keepUnstableUntilFirst);

this.idToken = this.user.pipe(switchMap(user => {
return user ? from(user.getIdToken()) : of(null)
Expand Down
198 changes: 196 additions & 2 deletions src/core/angularfire2.spec.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
import { TestBed, inject } from '@angular/core/testing';
import { PlatformRef, NgModule, CompilerFactory } from '@angular/core';
import { PlatformRef, NgModule, CompilerFactory, NgZone } from '@angular/core';
import { FirebaseApp, AngularFireModule } from '@angular/fire';
import { Subscription } from 'rxjs';
import { Subscription, Observable, Subject, of } from 'rxjs';
import { COMMON_CONFIG } from './test-config';
import { BrowserModule } from '@angular/platform-browser';
import { database } from 'firebase/app';
import { ZoneScheduler, keepUnstableUntilFirstFactory, AngularFireSchedulers } from './angularfire2';
import { ɵPLATFORM_BROWSER_ID, ɵPLATFORM_SERVER_ID } from '@angular/common';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with using private APIs in the tests but is there a way we can go public? Are there any changes coming up in 9 in regard to these constants?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big deal TBH, a lot more maintainable than stubbing isBrowser/Server

import { tap } from 'rxjs/operators';
import { TestScheduler } from 'rxjs/testing';

describe('angularfire', () => {
let subscription:Subscription;
Expand Down Expand Up @@ -40,6 +44,196 @@ describe('angularfire', () => {
app.delete().then(done, done.fail);
});

describe('ZoneScheduler', () => {
it('should execute the scheduled work inside the specified zone', done => {
let ngZone = Zone.current.fork({
name: 'ngZone'
});
const rootZone = Zone.current;

// Mimic real behavior: Executing in Angular
ngZone.run(() => {
const outsideAngularScheduler = new ZoneScheduler(rootZone);
outsideAngularScheduler.schedule(() => {
expect(Zone.current.name).not.toEqual('ngZone');
done();
});
});
});

it('should execute nested scheduled work inside the specified zone', done => {
const testScheduler = new TestScheduler(null!);
testScheduler.run(helpers => {
const outsideAngularScheduler = new ZoneScheduler(Zone.current, testScheduler);

let ngZone = Zone.current.fork({
name: 'ngZone'
});

let callbacksRan = 0;

// Mimic real behavior: Executing in Angular
ngZone.run(() => {
outsideAngularScheduler.schedule(() => {
callbacksRan++;
expect(Zone.current.name).not.toEqual('ngZone');

ngZone.run(() => {
// Sync queueing
outsideAngularScheduler.schedule(() => {
callbacksRan++;
expect(Zone.current.name).not.toEqual('ngZone');
});

// Async (10ms delay) nested scheduling
outsideAngularScheduler.schedule(() => {
callbacksRan++;
expect(Zone.current.name).not.toEqual('ngZone');
}, 10);

// Simulate flush from inside angular-
helpers.flush();
done();
expect(callbacksRan).toEqual(3);
})
});
helpers.flush();
});
});
})
})

describe('keepUnstableUntilFirstFactory', () => {
let schedulers: AngularFireSchedulers;
let outsideZone: Zone;
let insideZone: Zone;
beforeAll(() => {
outsideZone = Zone.current;
insideZone = Zone.current.fork({
name: 'ngZone'
});
const ngZone = {
run: insideZone.run.bind(insideZone),
runGuarded: insideZone.runGuarded.bind(insideZone),
runOutsideAngular: outsideZone.runGuarded.bind(outsideZone),
runTask: insideZone.run.bind(insideZone)
} as NgZone;
schedulers = new AngularFireSchedulers(ngZone);
})

it('should re-schedule emissions asynchronously', done => {
const keepUnstableOp = keepUnstableUntilFirstFactory(schedulers, ɵPLATFORM_SERVER_ID);

let ran = false;
of(null).pipe(
keepUnstableOp,
tap(() => ran = true)
).subscribe(() => {
expect(ran).toEqual(true);
done();
}, () => fail("Should not error"));

expect(ran).toEqual(false);
});

[ɵPLATFORM_SERVER_ID, ɵPLATFORM_BROWSER_ID].map(platformId =>
it(`should subscribe outside angular and observe inside angular (${platformId})`, done => {
const keepUnstableOp = keepUnstableUntilFirstFactory(schedulers, platformId);

insideZone.run(() => {
new Observable(s => {
expect(Zone.current).toEqual(outsideZone);
s.next("test");
}).pipe(
keepUnstableOp,
tap(() => {
expect(Zone.current).toEqual(insideZone);
})
).subscribe(() => {
expect(Zone.current).toEqual(insideZone);
done();
}, err => {
fail(err);
});
});
})
);

it('should block until first emission on server platform', done => {
const testScheduler = new TestScheduler(null!);
testScheduler.run(helpers => {
const outsideZone = Zone.current;
const taskTrack = new Zone['TaskTrackingZoneSpec']();
const insideZone = Zone.current.fork(taskTrack);
const trackingSchedulers: AngularFireSchedulers = {
ngZone: {
run: insideZone.run.bind(insideZone),
runGuarded: insideZone.runGuarded.bind(insideZone),
runOutsideAngular: outsideZone.runGuarded.bind(outsideZone),
runTask: insideZone.run.bind(insideZone)
} as NgZone,
outsideAngular: new ZoneScheduler(outsideZone, testScheduler),
insideAngular: new ZoneScheduler(insideZone, testScheduler),
};
const keepUnstableOp = keepUnstableUntilFirstFactory(trackingSchedulers, ɵPLATFORM_SERVER_ID);

const s = new Subject();
s.pipe(
keepUnstableOp,
).subscribe(() => { }, err => { fail(err); }, () => { });

// Flush to ensure all async scheduled functions are run
helpers.flush();
// Should now be blocked until first item arrives
expect(taskTrack.macroTasks.length).toBe(1);
expect(taskTrack.macroTasks[0].source).toBe('firebaseZoneBlock');

// Emit next item
s.next(123);
helpers.flush();

// Should not be blocked after first item
expect(taskTrack.macroTasks.length).toBe(0);

done();
});
})

it('should not block on client platform', done => {
const testScheduler = new TestScheduler(null!);
testScheduler.run(helpers => {
const outsideZone = Zone.current;
const taskTrack = new Zone['TaskTrackingZoneSpec']();
const insideZone = Zone.current.fork(taskTrack);
const trackingSchedulers: AngularFireSchedulers = {
ngZone: {
run: insideZone.run.bind(insideZone),
runGuarded: insideZone.runGuarded.bind(insideZone),
runOutsideAngular: outsideZone.runGuarded.bind(outsideZone),
runTask: insideZone.run.bind(insideZone)
} as NgZone,
outsideAngular: new ZoneScheduler(outsideZone, testScheduler),
insideAngular: new ZoneScheduler(insideZone, testScheduler),
};
const keepUnstableOp = keepUnstableUntilFirstFactory(trackingSchedulers, ɵPLATFORM_BROWSER_ID);

const s = new Subject();
s.pipe(
keepUnstableOp,
).subscribe(() => { }, err => { fail(err); }, () => { });

// Flush to ensure all async scheduled functions are run
helpers.flush();

// Zone should not be blocked
expect(taskTrack.macroTasks.length).toBe(0);
expect(taskTrack.microTasks.length).toBe(0);

done();
});
})
})

describe('FirebaseApp', () => {
it('should provide a FirebaseApp for the FirebaseApp binding', () => {
expect(typeof app.delete).toBe('function');
Expand Down
Loading