diff --git a/packages/app/src/api.test.ts b/packages/app/src/api.test.ts index 9a4f1fbe216..6dd12b82798 100644 --- a/packages/app/src/api.test.ts +++ b/packages/app/src/api.test.ts @@ -213,6 +213,7 @@ describe('API tests', () => { expect(app).to.not.equal(null); expect(app.automaticDataCollectionEnabled).to.be.false; await deleteApp(app); + expect((app as FirebaseServerAppImpl).isDeleted).to.be.true; }); it('creates FirebaseServerApp with automaticDataCollectionEnabled', async () => { @@ -233,6 +234,7 @@ describe('API tests', () => { expect(app).to.not.equal(null); expect(app.automaticDataCollectionEnabled).to.be.true; await deleteApp(app); + expect((app as FirebaseServerAppImpl).isDeleted).to.be.true; }); it('creates FirebaseServerApp with releaseOnDeref', async () => { @@ -251,6 +253,7 @@ describe('API tests', () => { expect(app).to.not.equal(null); expect(app.automaticDataCollectionEnabled).to.be.false; await deleteApp(app); + expect((app as FirebaseServerAppImpl).isDeleted).to.be.true; }); it('creates FirebaseServerApp with FirebaseApp', async () => { @@ -274,6 +277,7 @@ describe('API tests', () => { expect(app).to.not.equal(null); expect(app.options.apiKey).to.equal('test1'); await deleteApp(app); + expect((app as FirebaseServerAppImpl).isDeleted).to.be.true; }); }); @@ -285,8 +289,7 @@ describe('API tests', () => { const options = { apiKey: 'APIKEY' }; const serverAppSettingsOne: FirebaseServerAppSettings = { - automaticDataCollectionEnabled: false, - releaseOnDeref: options + automaticDataCollectionEnabled: true }; const serverAppSettingsTwo: FirebaseServerAppSettings = { @@ -295,12 +298,42 @@ describe('API tests', () => { const appOne = initializeServerApp(options, serverAppSettingsOne); expect(appOne).to.not.equal(null); - expect(appOne.automaticDataCollectionEnabled).to.be.false; + expect(appOne.automaticDataCollectionEnabled).to.be.true; const appTwo = initializeServerApp(options, serverAppSettingsTwo); expect(appTwo).to.not.equal(null); + expect(appTwo.automaticDataCollectionEnabled).to.be.false; expect(appTwo).to.not.equal(appOne); await deleteApp(appOne); await deleteApp(appTwo); + expect((appOne as FirebaseServerAppImpl).isDeleted).to.be.true; + expect((appTwo as FirebaseServerAppImpl).isDeleted).to.be.true; + }); + + it('create FirebaseServerApps with varying deleteOnDeref, and they still return same object ', async () => { + if (isBrowser()) { + // FirebaseServerApp isn't supported for execution in browser enviornments. + return; + } + + const options = { apiKey: 'APIKEY' }; + const serverAppSettingsOne: FirebaseServerAppSettings = { + automaticDataCollectionEnabled: false + }; + + const serverAppSettingsTwo: FirebaseServerAppSettings = { + automaticDataCollectionEnabled: false, + releaseOnDeref: options + }; + + const appOne = initializeServerApp(options, serverAppSettingsOne); + expect(appOne).to.not.equal(null); + expect(appOne.automaticDataCollectionEnabled).to.be.false; + const appTwo = initializeServerApp(options, serverAppSettingsTwo); + expect(appTwo).to.not.equal(null); + expect(appTwo.automaticDataCollectionEnabled).to.be.false; + expect(appTwo).to.equal(appOne); + await deleteApp(appOne); + await deleteApp(appTwo); }); it('create duplicate FirebaseServerApps returns the same object', async () => { @@ -322,11 +355,40 @@ describe('API tests', () => { expect(appTwo).to.not.equal(null); expect(appTwo).to.equal(appOne); await deleteApp(appOne); + await deleteApp(appTwo); + }); - // TODO: When Reference Counting works, update test. The following line should be false - // until and the app should be deleted a second time. + it('deleting FirebaseServerApps is ref counted', async () => { + if (isBrowser()) { + // FirebaseServerApp isn't supported for execution in browser enviornments. + return; + } + + const options = { apiKey: 'APIKEY' }; + const serverAppSettings: FirebaseServerAppSettings = { + automaticDataCollectionEnabled: false, + releaseOnDeref: options + }; + + const appOne = initializeServerApp(options, serverAppSettings); + expect((appOne as FirebaseServerAppImpl).refCount).to.equal(1); + + const appTwo = initializeServerApp(options, serverAppSettings); + expect(appTwo).to.equal(appOne); + expect((appOne as FirebaseServerAppImpl).refCount).to.equal(2); + expect((appTwo as FirebaseServerAppImpl).refCount).to.equal(2); + + await deleteApp(appOne); + expect((appOne as FirebaseServerAppImpl).refCount).to.equal(1); + expect((appTwo as FirebaseServerAppImpl).refCount).to.equal(1); + expect((appOne as FirebaseServerAppImpl).isDeleted).to.be.false; + expect((appTwo as FirebaseServerAppImpl).isDeleted).to.be.false; + + await deleteApp(appTwo); + expect((appOne as FirebaseServerAppImpl).refCount).to.equal(0); + expect((appTwo as FirebaseServerAppImpl).refCount).to.equal(0); expect((appOne as FirebaseServerAppImpl).isDeleted).to.be.true; - // await deleteApp(appTwo); + expect((appTwo as FirebaseServerAppImpl).isDeleted).to.be.true; }); describe('getApp', () => { diff --git a/packages/app/src/api.ts b/packages/app/src/api.ts index 246c06b144f..e4b1a6db341 100644 --- a/packages/app/src/api.ts +++ b/packages/app/src/api.ts @@ -235,6 +235,10 @@ export function initializeServerApp( throw ERROR_FACTORY.create(AppError.INVALID_SERVER_APP_ENVIRONMENT); } + if (_serverAppConfig.automaticDataCollectionEnabled === undefined) { + _serverAppConfig.automaticDataCollectionEnabled = false; + } + let appOptions: FirebaseOptions; if (_isFirebaseApp(_options)) { appOptions = _options.options; @@ -242,12 +246,18 @@ export function initializeServerApp( appOptions = _options; } - // Mangle the ap name based on a hash of the FirebaseServerAppSettings, and FirebaseOptions - // objects and the authIdToken, if provided. + // Build an app name based on a hash of the configuration options. const nameObj = { - _serverAppConfig, + ..._serverAppConfig, ...appOptions }; + + // However, Do not mangle the name based on releaseOnDeref, since it will vary between the + // construction of FirebaseServerApp instances. For example, if the object is the request headers. + if (nameObj.releaseOnDeref !== undefined) { + delete nameObj.releaseOnDeref; + } + const hashCode = (s: string): number => { return [...s].reduce( (hash, c) => (Math.imul(31, hash) + c.charCodeAt(0)) | 0, @@ -255,12 +265,7 @@ export function initializeServerApp( ); }; - const serverAppSettings: FirebaseServerAppSettings = { - automaticDataCollectionEnabled: false, - ..._serverAppConfig - }; - - if (serverAppSettings.releaseOnDeref !== undefined) { + if (_serverAppConfig.releaseOnDeref !== undefined) { if (typeof FinalizationRegistry === 'undefined') { throw ERROR_FACTORY.create( AppError.FINALIZATION_REGISTRY_NOT_SUPPORTED, @@ -272,9 +277,9 @@ export function initializeServerApp( const nameString = '' + hashCode(JSON.stringify(nameObj)); const existingApp = _serverApps.get(nameString) as FirebaseServerApp; if (existingApp) { - // TODO: - // 1: Register a new reference to finalization registry. - // 2: Incrememnt reference count. + (existingApp as FirebaseServerAppImpl).incRefCount( + _serverAppConfig.releaseOnDeref + ); return existingApp; } @@ -285,7 +290,7 @@ export function initializeServerApp( const newApp = new FirebaseServerAppImpl( appOptions, - serverAppSettings, + _serverAppConfig, nameString, container ); @@ -362,16 +367,20 @@ export function getApps(): FirebaseApp[] { * @public */ export async function deleteApp(app: FirebaseApp): Promise { - let foundApp = false; + let cleanupProviders = false; const name = app.name; if (_apps.has(name)) { - foundApp = true; + cleanupProviders = true; _apps.delete(name); } else if (_serverApps.has(name)) { - foundApp = true; - _serverApps.delete(name); + const firebaseServerApp = app as FirebaseServerAppImpl; + if (firebaseServerApp.decRefCount() <= 0) { + _serverApps.delete(name); + cleanupProviders = true; + } } - if (foundApp) { + + if (cleanupProviders) { await Promise.all( (app as FirebaseAppImpl).container .getProviders() diff --git a/packages/app/src/firebaseServerApp.ts b/packages/app/src/firebaseServerApp.ts index 0135ff8897a..a94dc7bd760 100644 --- a/packages/app/src/firebaseServerApp.ts +++ b/packages/app/src/firebaseServerApp.ts @@ -32,6 +32,7 @@ export class FirebaseServerAppImpl { private readonly _serverConfig: FirebaseServerAppSettings; private _finalizationRegistry: FinalizationRegistry; + private _refCount: number; constructor( options: FirebaseOptions | FirebaseAppImpl, @@ -65,22 +66,48 @@ export class FirebaseServerAppImpl ...serverConfig }; - this._finalizationRegistry = new FinalizationRegistry( - this.automaticCleanup - ); + this._finalizationRegistry = new FinalizationRegistry(() => { + this.automaticCleanup(); + }); - if (this._serverConfig.releaseOnDeref !== undefined) { - this._finalizationRegistry.register( - this._serverConfig.releaseOnDeref, - this - ); - this._serverConfig.releaseOnDeref = undefined; // Don't keep a strong reference to the object. + this._refCount = 0; + this.incRefCount(this._serverConfig.releaseOnDeref); + + // Do not retain a hard reference to the dref object, otherwise the FinalizationRegisry + // will never trigger. + this._serverConfig.releaseOnDeref = undefined; + serverConfig.releaseOnDeref = undefined; + } + + get refCount(): number { + return this._refCount; + } + + // Increment the reference count of this server app. If an object is provided, register it + // with the finalization registry. + incRefCount(obj: object | undefined): void { + if (this.isDeleted) { + return; + } + this._refCount++; + if (obj !== undefined) { + this._finalizationRegistry.register(obj, this); + } + } + + // Decrement the reference count. + decRefCount(): number { + if (this.isDeleted) { + return 0; } + return --this._refCount; } - private automaticCleanup(serverApp: FirebaseServerAppImpl): void { - // TODO: implement reference counting. - void deleteApp(serverApp); + // Invoked by the FinalizationRegistry callback to note that this app should go through its + // reference counts and delete itself if no reference count remain. The coordinating logic that + // handles this is in deleteApp(...). + private automaticCleanup(): void { + void deleteApp(this); } get settings(): FirebaseServerAppSettings {