-
Notifications
You must be signed in to change notification settings - Fork 928
[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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -235,32 +235,37 @@ 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; | ||
} 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, | ||
|
@@ -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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The removed TODO comment of |
||
_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<void> { | ||
let foundApp = false; | ||
let cleanupProviders = false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||
|
There was a problem hiding this comment.
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 tofalse
if the optional value is not defined, so that it aligns with futureFirebaseServerApp
instances that defineautomaticDataCollectionEnabled
as explicitlyfalse
.