Skip to content

[ServerApp] RefCount FirebaseServerApps #8000

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 4 commits into from
Feb 2, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 68 additions & 6 deletions packages/app/src/api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand All @@ -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 () => {
Expand All @@ -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 () => {
Expand All @@ -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;
});
});

Expand All @@ -285,8 +289,7 @@ describe('API tests', () => {

const options = { apiKey: 'APIKEY' };
const serverAppSettingsOne: FirebaseServerAppSettings = {
automaticDataCollectionEnabled: false,
releaseOnDeref: options
automaticDataCollectionEnabled: true
};

const serverAppSettingsTwo: FirebaseServerAppSettings = {
Expand All @@ -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 () => {
Expand All @@ -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', () => {
Expand Down
43 changes: 26 additions & 17 deletions packages/app/src/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -235,32 +235,37 @@ export function initializeServerApp(
throw ERROR_FACTORY.create(AppError.INVALID_SERVER_APP_ENVIRONMENT);
}

if (_serverAppConfig.automaticDataCollectionEnabled === undefined) {
Copy link
Contributor Author

@DellaBitta DellaBitta Jan 31, 2024

Choose a reason for hiding this comment

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

Previous detection of the reusable FirebaseServerApps wasn't taking the automaticDataCollectionEnabled field into account. We need to explicitly set this to false if the optional value is not defined, so that it aligns with future FirebaseServerApp instances that define automaticDataCollectionEnabled as explicitly false.

_serverAppConfig.automaticDataCollectionEnabled = false;
}

let appOptions: FirebaseOptions;
if (_isFirebaseApp(_options)) {
appOptions = _options.options;
} else {
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,
...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._serverAppConfig.releaseOnDeref !== undefined) {
delete nameObj._serverAppConfig.releaseOnDeref;
}

const hashCode = (s: string): number => {
return [...s].reduce(
(hash, c) => (Math.imul(31, hash) + c.charCodeAt(0)) | 0,
0
);
};

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,
Expand All @@ -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(
Copy link
Contributor Author

@DellaBitta DellaBitta Jan 31, 2024

Choose a reason for hiding this comment

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

The removed TODO comment of Register a new reference to finalization registry. is handled within incRefCount which, actually thinking about it, handles both of these TODOs.

_serverAppConfig.releaseOnDeref
);
return existingApp;
}

Expand All @@ -285,7 +290,7 @@ export function initializeServerApp(

const newApp = new FirebaseServerAppImpl(
appOptions,
serverAppSettings,
_serverAppConfig,
nameString,
container
);
Expand Down Expand Up @@ -362,16 +367,20 @@ export function getApps(): FirebaseApp[] {
* @public
*/
export async function deleteApp(app: FirebaseApp): Promise<void> {
let foundApp = false;
let cleanupProviders = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed this variable to make it better reflect why the code below is executed (on new line 383).

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()
Expand Down
36 changes: 28 additions & 8 deletions packages/app/src/firebaseServerApp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export class FirebaseServerAppImpl
{
private readonly _serverConfig: FirebaseServerAppSettings;
private _finalizationRegistry: FinalizationRegistry<object>;
private _refCount: number;

constructor(
options: FirebaseOptions | FirebaseAppImpl,
Expand Down Expand Up @@ -69,18 +70,37 @@ export class FirebaseServerAppImpl
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;
}

get refCount(): number {
return this._refCount;
}

incRefCount(obj: Object | undefined) {
if (this.isDeleted) {
return;
}
this._refCount++;
if (obj !== undefined) {
this._finalizationRegistry.register(obj, this);
}
}

decRefCount(): number {
if (this.isDeleted) {
return 0;
}
return --this._refCount;
}

private automaticCleanup(serverApp: FirebaseServerAppImpl): void {
// TODO: implement reference counting.
void deleteApp(serverApp);
deleteApp(serverApp);
}

get settings(): FirebaseServerAppSettings {
Expand Down