Skip to content

fix: Cleaning up FirebaseApp state management #476

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 1 commit into from
Sep 10, 2020
Merged

Conversation

hiranya911
Copy link
Contributor

We have a mechanism in place to tear down any SDK state and managed resources upon calling FirebaseApp.delete(). However the way this is used and enforced across services is inconsistent at the moment:

  • FirebaseAuth uses some complex and repetitive logic to throw errors from all methods after delete() has been called.
  • Most other services (e.g. FirebaseMessaging) do not bother with this.

In order to simplify the service implementations and make them consistent I'm proposing the following changes:

  1. FirebaseAuth doesn't have any state that requires explicit tear down. Therefore remove the existing repetitive logic for throwing errors after delete() is called.
  2. Make the FirebaseService.destroy() method non-abstract so that child classes are not forced to provide a no-op implementation. Child classes that do allocate managed resources (e.g. FirebaseDatabase) can still override it to tear down those resources.
  3. Clarify the semantics of FirebaseApp.delete() in the API reference docs.

This results in the following concrete behaviors after FirebaseApp.delete():

  • Calling any of the FirebaseXXXXX.getInstance() methods will throw.
  • Services with managed resources (FirebaseDatabase and FirestoreClient) will get cleaned up and become unusable.
  • Some methods in services without managed resources will throw (notably all the async methods will throw since they rely on a thread pool attached to FirebaseApp).
  • Some methods in services without managed resources will continue to work (this behavior exists today in some classes as mentioned above), but this is not a valid use of the SDK and developers should refrain from doing it.

Potential future work: If necessary we can implement a mechanism to block all rpc calls after FirebaseApp.delete() has been called. This is easy enough to enforce via the existing FirebaseRequestInitializer. But methods that do not make rpc calls (e.g. FirebaseAuth.createCustomToken()) will continue to work.

Copy link
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

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

Thank you! Makes sense!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants