Make FirebaseApp.delete() Public by hiranya911 · Pull Request #5 · firebase/firebase-admin-java

hiranya911 · 2017-04-17T23:57:54Z

This PR fixes and exposes the delete() method in FirebaseApp class.

Introduced a new abstraction called FirebaseService
FirebaseAuth and FirebaseDatabase are wrapped as instances of FirebaseService, and attached to the FirebaseApp. Also we no longer keep static caches of auth and database instances.
This enables FirebaseApp.delete() method to gracefully tear down the "services" attached to it. Also all services attached to an app instances get cleaned/GC'd along with the app.
Updated integration test suite to use a single masterApp instances across all tests (saves about 30 seconds caused by connection start and tear down)

* Reformatted all source files * Separated unit and integration tests * Added maven checkstyle plugin * Got the basic build and unit tests working * Scrubbed the codebase for private keys, certificates etc.

…ethod

…ndency

…ration tests

…ance across all integration tests

mikelehen

Mostly looks good, but I have some questions / comments about threading and the behavior of FirebaseDatabase after a delete() call.

mikelehen · 2017-04-18T00:19:14Z

+
+import com.google.common.base.Strings;
+
+public abstract class FirebaseService<T> {


Comments on the class and methods please.

mikelehen · 2017-04-18T14:21:07Z

-      return;
+  public void delete() {
+    List<FirebaseService> servicesCopy;
+    synchronized (this) {


FYI- some say you should avoid "synchronized(this)" since it could potentially interact badly with outside code (if a developer synchronizes on your object externally). And so classes will often create an internal object to lock on instead. Some discussion here: http://stackoverflow.com/questions/442564/avoid-synchronizedthis-in-java

Makes sense. Using a per Firebase App lock object instead of "this"

mikelehen · 2017-04-18T15:49:49Z

  static void clearInstancesForTest() {
-    // TODO(arondeak): also delete, once functionality is implemented.
    synchronized (sLock) {
+      for (FirebaseApp app : ImmutableList.copyOf(instances.values())) {


Why do we lock and make an immutable copy?

We need to iterate over a copy, as delete() would try to remove from the original map while iterating. Added a comment to explain.

mikelehen · 2017-04-18T16:02:40Z

+        return;
+      }
+
+      servicesCopy = ImmutableList.copyOf(services.values());


I'm guessing you are intentionally calling destroy() outside the lock to avoid calling external code inside the lock, which could potentially result in deadlocks. But I'd prefer not guess. :-) Can you add a comment?

mikelehen · 2017-04-18T16:10:50Z

  }

-  void removeAuthStateListener(@NonNull AuthStateListener listener) {
+  synchronized void removeAuthStateListener(@NonNull AuthStateListener listener) {


FYI- This "synchronized" will likely show up in our javadoc-generated reference docs (if we have them for the admin SDK), which is kind of annoying (some methods will show up as "synchronized" and others won't, which could confuse developers). I'd avoid using synchronized methods (and per my other comment, it's probably better to use sLock than this anyway).

mikelehen · 2017-04-18T17:36:35Z

    firebaseApp.delete();
-    verify(listener, never()).onDeleted(appName, OPTIONS);
+    try {
+      try {


Can we not nest these?

mikelehen · 2017-04-18T17:37:29Z

+        // ignore
+      }
+
+      FirebaseApp firebaseApp2 = FirebaseApp.initializeApp(OPTIONS, name);


Add a comment explaining intention of this test? E.g.:

// Verify we can re-use the same name.

mikelehen · 2017-04-18T17:38:26Z

+
+  @Test
+  public void testInvokeAfterAppDelete() throws ExecutionException, InterruptedException {
+    if (!isCertCredential) {


I wonder if we should log a message or something when skipping tests so that we don't accidentally skip them without realizing it? Your call.

Added some log statements.

mikelehen · 2017-04-18T17:46:49Z

  @AfterClass
-  public static void tearDownClass() {
+  public static void tearDownClass() throws InterruptedException {
+    TestHelpers.interruptConfig(config);


I'm not 100% sure what's going on here. Would it be possible to encapsulate these in nice self-describing TestHelper methods. TestHelpers.startAppForUnitTest() / TestHelpers.stopAppForUnitTest() or something? Or is DataSnapshotTest special for some reason and needs different logic? If so, please add a comment.

We just need to use a mock/stand-in DatabaseConfig here since we are not connecting to an actual database. Added a comment to explain.

mikelehen · 2017-04-18T17:56:23Z

-            .setCredential(FirebaseCredentials.fromCertificate(getServiceAccountCertificate()))
-            .build();
-    return FirebaseApp.initializeApp(options);
+  public static synchronized FirebaseApp ensureDefaultApp() {


Can you add a comment? It looks like the intention is that all the integration tests use this method?

…ts and javadocs

hiranya911 · 2017-04-18T22:26:09Z

I've implemented all the suggested changes. Few additional changes were made to FirebaseDatabase, Repo and Query to make sure they cannot be used after application delete.

…hecks to Repo

…he contention on the database-object lock by making ensureRepo() thread safe and returning the Repo from the method.

mikelehen

Left a couple FYI comments, but I think this looks good. Thanks for the changes!

mikelehen · 2017-04-19T15:46:02Z

-    }
+      synchronized (appsLock) {
+        instances.remove(this.name);
+      }


FYI- Acquiring multiple locks is a step towards deadlock potential (if another piece of code acquires both locks but in the opposite order, you have a deadlock). I'm not too concerned in this case, but if this were a larger code base with less clear component interactions, I would avoid having multiple locks or else set very clear rules about how the locks must be used (a common strategy is to declare a locking order A > B, saying that if you want to acquire lock B you must first acquire A).

I'm fine with leaving this as-is though.

Good point. We have at least one place where we take these 2 locks in the reverse order (in clearInstancesForTest()). That shouldn't be an issue since it's only a test method, but I think it's better to implement this correct from the onset. Therefore I'm changing this back to how it was. That is I'm moving the nested synch block out. I think it makes sense for lock to guard only the instance members, and appsLock to guard the instances map. No real need for the nested synch block here.

mikelehen · 2017-04-19T16:07:34Z

-    }
-
+    checkNotNull(url,
+        "Can't pass null for argument 'url' in FirebaseDatabase.getReferenceFromUrl()");


As a minor thing, it's generally preferable not to make tangential refactorings while responding to code review feedback as it makes it harder to review and increases the potential for needing extra code review iterations. Basically, every code review iteration should make the minimal set of changes to get the change ready for submission. Anything tangential should probably go in a separate PR.

On a related note, it's generally better to have many small PRs (each with a single purpose) than single large PRs (that do a bunch of different things). It makes reviewing easier and keeps the change history cleaner.

Understood.

Hiranya Jayathilaka and others added 30 commits April 5, 2017 19:12

Initial code migration:

428eedb

* Reformatted all source files * Separated unit and integration tests * Added maven checkstyle plugin * Got the basic build and unit tests working * Scrubbed the codebase for private keys, certificates etc.

Reformatting code according to check-style plugin conventions

cfbbd6a

Code reformatting: Fixing javadoc indentation

59842b8

More reformatting: Added google check style config

fef4ecf

Code formatting continued

a606833

Code formatting continued

1d2ed82

Reformatting

8972b1b

Fixed all checkstyle violations; Build configured to fail on lint errors

e5509c3

Updated checkstyle config

0587d52

Working on re-enabling the integration tests

7f2f470

Temporarily removing IT test cases

8ee5cfc

Extending checkstyle coverage to test sourcesw

dbc6ee6

Cleaned up verbose test output

c66f35b

Adding 2 integration tests to verify the maven integration test support

f7cbdc2

Moving test only trampolines to the test/ subtree; Adding a removed m…

7715b90

…ethod

Fixing integration tests

c3591ed

Added event test

bc54329

Added the real time test

75df20f

Adding data test back

471d0a9

Fixed the object serialization test

f692bb6

Adding a factory method to MapBuilder

c368248

Added transaction integration test

a30ae95

Added transaction test

7714024

Added rules integration test

2da7955

Updated rules test

094d756

Merge branch 'hkj-initial-code-migration' into hkj-db-integration-tests

f446c79

Cleaning up the test code; Removed unnecessary utils and constants

952c6b0

Added more factory methods for MapBuilder; Removed unused legacy depe…

594bfbb

…ndency

Dropped the jackson test dependency

5dc413f

Adding release profile

738c75e

Hiranya Jayathilaka and others added 10 commits April 14, 2017 11:02

Updated titles and links

f5883d7

Fixing SCM URL

21e14d6

Removed Base64 and Preconditions

4bb3923

Removed Objects

36b334f

Implemented a mechanism to capture async errors and fail during integ…

ca5af15

…ration tests

Implementing App delete functionality

1885188

Fixing some time sensitive tests; Added test cases for app delete

e8a1d83

Implemented delete related test cases; Using a single master app inst…

83410c2

…ance across all integration tests

Merged with latest master

dc9b4ef

Updated app test

9ec7008

hiranya911 requested a review from mikelehen April 17, 2017 23:58

hiranya911 assigned mikelehen Apr 17, 2017

mikelehen suggested changes Apr 18, 2017

View reviewed changes

mikelehen assigned hiranya911 and unassigned mikelehen Apr 18, 2017

hiranya911 and others added 5 commits April 18, 2017 13:25

Improved/simplified synchronization in FirebaseApp; Added more commen…

89d501a

…ts and javadocs

Throwing exceptions after calling destroy() on FirebaseDatabase

1d5c157

Throwing errors from DatabaseReference instances after app delete

c47db63

Merge branch 'master' into hkj-delete-apps

2e93f54

Added delete checks to Repo

bfb96cb

hiranya911 assigned mikelehen and unassigned hiranya911 Apr 18, 2017

Hiranya Jayathilaka added 2 commits April 18, 2017 16:26

Removing checks from Query as they eventually call Repo; Added more c…

b50754f

…hecks to Repo

Synchronizing the FirebaseDatabase.checkNotDeleted() method; Easing t…

ed059c4

…he contention on the database-object lock by making ensureRepo() thread safe and returning the Repo from the method.

mikelehen approved these changes Apr 19, 2017

View reviewed changes

Removing the nested synch block in FirebaseApp.delete()

28e4123

hiranya911 merged commit 333a795 into master Apr 19, 2017

hiranya911 deleted the hkj-delete-apps branch April 19, 2017 17:13

samtstern mentioned this pull request Jun 4, 2019

No way to delete/unmap an App firebase/quickstart-java#9

Closed


		import com.google.common.base.Strings;

		public abstract class FirebaseService<T> {

Uh oh!

Conversation

hiranya911 commented Apr 17, 2017

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 Apr 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 commented Apr 18, 2017

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hiranya911 Apr 18, 2017 •

edited

Loading