Skip to content

Prepare shutdown to be a public api. #589

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.

Sign up for GitHub

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
11 commits merged into from
Jul 17, 2019
Merged

Prepare shutdown to be a public api. #589

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
fa5e624
Deregister firestore when shutdown + app.delete calls shutdown
wu-hui Jul 4, 2019
9ba6ff3
synchronized scheduling + soft shutdown
wu-hui Jul 4, 2019
835ddc8
Add comments and more tests
wu-hui Jul 8, 2019
e7e3ea1
addressing comments #1
wu-hui Jul 11, 2019
317fea7
fixing format in tools
wu-hui Jul 11, 2019
34171e8
Stop leakages
wu-hui Jul 14, 2019
74d507c
better comments
wu-hui Jul 14, 2019
fe02b17
Handle app delete shutdown differently
wu-hui Jul 17, 2019
f8b33ed
No more error-prone hack
wu-hui Jul 17, 2019
d3141b2
Change how to interact with MultiDbComponent.
wu-hui Jul 17, 2019
701fcef
add missing sync keyword
wu-hui Jul 17, 2019
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
11 changes: 9 additions & 2 deletions firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AccessHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,16 @@ public static FirebaseFirestore newFirebaseFirestore(
String persistenceKey,
CredentialsProvider credentialsProvider,
AsyncQueue asyncQueue,
FirebaseApp firebaseApp) {
FirebaseApp firebaseApp,
FirestoreMultiDbComponent.DeregisterCommand deregisterCommand) {
return new FirebaseFirestore(
context, databaseId, persistenceKey, credentialsProvider, asyncQueue, firebaseApp);
context,
databaseId,
persistenceKey,
credentialsProvider,
asyncQueue,
firebaseApp,
deregisterCommand);
}

/** Makes the shutdown method accessible. */
Expand Down
73 changes: 73 additions & 0 deletions firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import static com.google.firebase.firestore.testutil.IntegrationTestUtil.testCollection;
import static com.google.firebase.firestore.testutil.IntegrationTestUtil.testCollectionWithDocs;
import static com.google.firebase.firestore.testutil.IntegrationTestUtil.testDocument;
import static com.google.firebase.firestore.testutil.IntegrationTestUtil.testFirebaseApp;
import static com.google.firebase.firestore.testutil.IntegrationTestUtil.testFirestore;
import static com.google.firebase.firestore.testutil.IntegrationTestUtil.waitFor;
import static com.google.firebase.firestore.testutil.IntegrationTestUtil.waitForException;
Expand All @@ -30,6 +31,7 @@
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNotSame;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertTrue;
Expand All @@ -38,6 +40,7 @@
import androidx.test.ext.junit.runners.AndroidJUnit4;
import com.google.android.gms.tasks.Task;
import com.google.android.gms.tasks.TaskCompletionSource;
import com.google.firebase.FirebaseApp;
import com.google.firebase.Timestamp;
import com.google.firebase.firestore.FirebaseFirestoreException.Code;
import com.google.firebase.firestore.Query.Direction;
Expand Down Expand Up @@ -1023,4 +1026,74 @@ public void testClearPersistenceWhileRunningFails() {
FirebaseFirestoreException firestoreException = (FirebaseFirestoreException) e;
assertEquals(Code.FAILED_PRECONDITION, firestoreException.getCode());
}

@Test
public void testRestartFirestoreLeadsToNewInstance() {
FirebaseApp app = testFirebaseApp();
FirebaseFirestore instance = FirebaseFirestore.getInstance(app);
FirebaseFirestore sameInstance = FirebaseFirestore.getInstance(app);

assertSame(instance, sameInstance);
waitFor(instance.document("abc/123").set(Collections.singletonMap("field", 100L)));

instance.shutdown();
FirebaseFirestore newInstance = FirebaseFirestore.getInstance(app);

// Verify new instance works.
DocumentSnapshot doc = waitFor(newInstance.document("abc/123").get());
assertEquals(doc.get("field"), 100L);
waitFor(newInstance.document("abc/123").delete());

// Verify it is different instance.
assertNotSame(instance, newInstance);
}

@Test
public void testAppDeleteLeadsToFirestoreShutdown() {
FirebaseApp app = testFirebaseApp();
FirebaseFirestore instance = FirebaseFirestore.getInstance(app);
waitFor(instance.document("abc/123").set(Collections.singletonMap("Field", 100)));

app.delete();

assertTrue(instance.getClient().isShutdown());
}

@Test
public void testNewOperationThrowsAfterFirestoreShutdown() {
FirebaseFirestore instance = testFirestore();
DocumentReference reference = instance.document("abc/123");
waitFor(reference.set(Collections.singletonMap("Field", 100)));

instance.shutdown();

final String expectedMessage = "The client has already been shutdown";
expectError(() -> waitFor(reference.get()), expectedMessage);
expectError(() -> waitFor(reference.update("Field", 1)), expectedMessage);
expectError(
() -> waitFor(reference.set(Collections.singletonMap("Field", 1))), expectedMessage);
expectError(
() -> waitFor(instance.runBatch((batch) -> batch.update(reference, "Field", 1))),
expectedMessage);
expectError(
() -> waitFor(instance.runTransaction(transaction -> transaction.get(reference))),
expectedMessage);
}

@Test
public void testShutdownCalledMultipleTimes() {
FirebaseFirestore instance = testFirestore();
DocumentReference reference = instance.document("abc/123");
waitFor(reference.set(Collections.singletonMap("Field", 100)));

instance.shutdown();

final String expectedMessage = "The client has already been shutdown";
expectError(() -> waitFor(reference.get()), expectedMessage);

// Calling a second time should go through and change nothing.
instance.shutdown();

expectError(() -> waitFor(reference.get()), expectedMessage);
}
}
8 changes: 7 additions & 1 deletion ...tore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.google.android.gms.tasks.Task;
import com.google.android.gms.tasks.TaskCompletionSource;
import com.google.android.gms.tasks.Tasks;
import com.google.firebase.FirebaseApp;
import com.google.firebase.firestore.AccessHelper;
import com.google.firebase.firestore.BuildConfig;
import com.google.firebase.firestore.CollectionReference;
Expand Down Expand Up @@ -145,6 +146,10 @@ protected String checkAuthority(String authority) {
return settings.build();
}

public static FirebaseApp testFirebaseApp() {
return FirebaseApp.initializeApp(ApplicationProvider.getApplicationContext());
}

/** Initializes a new Firestore instance that uses the default project. */
public static FirebaseFirestore testFirestore() {
return testFirestore(newTestSettings());
Expand Down Expand Up @@ -259,7 +264,8 @@ public static FirebaseFirestore testFirestore(
persistenceKey,
new EmptyCredentialsProvider(),
asyncQueue,
/*firebaseApp=*/ null);
/*firebaseApp=*/ null,
/*deregisterCommand=*/ () -> {});
waitFor(AccessHelper.clearPersistence(firestore));
firestore.setFirestoreSettings(settings);
firestoreStatus.put(firestore, true);
Expand Down
47 changes: 41 additions & 6 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ public class FirebaseFirestore {
private final AsyncQueue asyncQueue;
private final FirebaseApp firebaseApp;
private final UserDataConverter dataConverter;
// When user requests to shutdown, use this to notify `FirestoreMultiDbComponent` to deregister
// this instance.
private final FirestoreMultiDbComponent.DeregisterCommand onDeregister;
private FirebaseFirestoreSettings settings;
private volatile FirestoreClient client;

Expand Down Expand Up @@ -94,7 +97,8 @@ static FirebaseFirestore newInstance(
@NonNull Context context,
@NonNull FirebaseApp app,
@Nullable InternalAuthProvider authProvider,
@NonNull String database) {
@NonNull String database,
@NonNull FirestoreMultiDbComponent.DeregisterCommand onDeregister) {
String projectId = app.getOptions().getProjectId();
if (projectId == null) {
throw new IllegalArgumentException("FirebaseOptions.getProjectId() cannot be null");
Expand All @@ -117,7 +121,10 @@ static FirebaseFirestore newInstance(
// so there is no need to include it in the persistence key.
String persistenceKey = app.getName();

return new FirebaseFirestore(context, databaseId, persistenceKey, provider, queue, app);
FirebaseFirestore firestore =
new FirebaseFirestore(
context, databaseId, persistenceKey, provider, queue, app, onDeregister);
return firestore;
}

@VisibleForTesting
Expand All @@ -127,7 +134,8 @@ static FirebaseFirestore newInstance(
String persistenceKey,
CredentialsProvider credentialsProvider,
AsyncQueue asyncQueue,
@Nullable FirebaseApp firebaseApp) {
@Nullable FirebaseApp firebaseApp,
FirestoreMultiDbComponent.DeregisterCommand onDeregister) {
this.context = checkNotNull(context);
this.databaseId = checkNotNull(checkNotNull(databaseId));
this.dataConverter = new UserDataConverter(databaseId);
Expand All @@ -136,6 +144,7 @@ static FirebaseFirestore newInstance(
this.asyncQueue = checkNotNull(asyncQueue);
// NOTE: We allow firebaseApp to be null in tests only.
this.firebaseApp = firebaseApp;
this.onDeregister = onDeregister;

settings = new FirebaseFirestoreSettings.Builder().build();
}
Expand Down Expand Up @@ -329,13 +338,39 @@ public Task<Void> runBatch(@NonNull WriteBatch.Function batchFunction) {
return batch.commit();
}

@VisibleForTesting
Task<Void> shutdown() {
Task<Void> shutdownInternal() {
// The client must be initialized to ensure that all subsequent API usage throws an exception.
this.ensureClientConfigured();
return client.shutdown();
}

/**
* Shuts down this FirebaseFirestore instance.
*
* <p>After shutdown only the {@link #clearPersistence()} method may be used. Any other method
* will throw an {@link IllegalStateException}.
*
* <p>To restart after shutdown, simply create a new instance of FirebaseFirestore with {@link
* #getInstance()} or {@link #getInstance(FirebaseApp)}.
*
* <p>Shutdown does not cancel any pending writes and any tasks that are awaiting a response from
* the server will not be resolved. The next time you start this instance, it will resume
* attempting to send these writes to the server.
*
* <p>Note: Under normal circumstances, calling <code>shutdown()</code> is not required. This
* method is useful only when you want to force this instance to release all of its resources or
* in combination with {@link #clearPersistence} to ensure that all local state is destroyed
* between test runs.
*
* @return A <code>Task</code> that is resolved when the instance has been successfully shut down.
*/
@VisibleForTesting
// TODO(b/135755126): Make this public and remove @VisibleForTesting
Task<Void> shutdown() {
onDeregister.execute();
return shutdownInternal();
}

@VisibleForTesting
AsyncQueue getAsyncQueue() {
return asyncQueue;
Expand Down Expand Up @@ -396,7 +431,7 @@ public static void setLoggingEnabled(boolean loggingEnabled) {
@PublicApi
public Task<Void> clearPersistence() {
final TaskCompletionSource<Void> source = new TaskCompletionSource<>();
asyncQueue.enqueueAndForget(
asyncQueue.enqueueAndForgetEvenAfterShutdown(
() -> {
try {
if (client != null && !client.isShutdown()) {
Expand Down
42 changes: 36 additions & 6 deletions ...base-firestore/src/main/java/com/google/firebase/firestore/FirestoreMultiDbComponent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,19 @@
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import com.google.firebase.FirebaseApp;
import com.google.firebase.FirebaseAppLifecycleListener;
import com.google.firebase.FirebaseOptions;
import com.google.firebase.auth.internal.InternalAuthProvider;
import java.util.HashMap;
import java.util.Map;

/** Multi-resource container for Firestore. */
class FirestoreMultiDbComponent {
class FirestoreMultiDbComponent implements FirebaseAppLifecycleListener {

public interface DeregisterCommand {
Copy link
Member

Choose a reason for hiding this comment

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

java.lang.Runnable instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good idea.

Alternatively, you could just make an interface that contains remove(String databaseId) such that FirestoreMultiDbComponent could just implement it. This removes the extra capture.

In any case, the interface describing this behavior should not be nested in FirestoreMultiDbComponent because that means FirebaseFirestore still ends up having an awareness of that type. Instead nest the interface in FirebaseFirestore.

Something like this is what I mean:

class FirebaseFiresore {
  // ...
  interface InstanceRegistry {
    void remove(@NonNull String databaseId);
  }
}

FirestoreMultiDbComponent can implement this interface directly.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

void execute();
}

/**
* A static map from instance key to FirebaseFirestore instances. Instance keys are database
* names.
Expand All @@ -41,16 +48,39 @@ class FirestoreMultiDbComponent {
this.context = context;
this.app = app;
this.authProvider = authProvider;
this.app.addLifecycleEventListener(this);
Copy link
Member

Choose a reason for hiding this comment

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

Just a consideration: app.delete() is not a documented API and no other SDK integrates with it(hence it is effectively almost a noop), I'll defer to your judgement on whether you want to integrate with it

Copy link
Contributor

Choose a reason for hiding this comment

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

During the API discussion about this, the consensus emerged that shutdown must not leave a broken instance registered and that app deletion should mean shutdown of the Firestore instance.

While this hasn't been made public or documented on Android specifically, that's a localized problem. We want our ports to behave identically to the extent that we can arrange it. If this ever becomes a documented API we'll be ready. In other words: this will be the behavior on the other ports so might as well do the same here.

Copy link
Member

Choose a reason for hiding this comment

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

sgtm

}

/** Provides instances of Firestore for given database names. */
/** Provides instances of Firestore for given database IDs. */
@NonNull
synchronized FirebaseFirestore get(@NonNull String databaseName) {
FirebaseFirestore firestore = instances.get(databaseName);
synchronized FirebaseFirestore get(@NonNull String databaseId) {
FirebaseFirestore firestore = instances.get(databaseId);
if (firestore == null) {
firestore = FirebaseFirestore.newInstance(context, app, authProvider, databaseName);
instances.put(databaseName, firestore);
firestore =
FirebaseFirestore.newInstance(
context, app, authProvider, databaseId, () -> this.remove(databaseId));
instances.put(databaseId, firestore);
}
return firestore;
}

/**
* Remove the instance of a given database ID from this component, such that if {@link
* FirestoreMultiDbComponent#get(String)} is called again with the same name, a new instance of
* {@link FirebaseFirestore} is created.
*
* <p>It is a no-op if there is no instance associated with the given database name.
*/
synchronized void remove(@NonNull String databaseId) {
instances.remove(databaseId);
}

@Override
public void onDeleted(String firebaseAppName, FirebaseOptions options) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this method also synchronize its access to the map?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed. Thanks.

// Shuts down all database instances and remove them from registry map when App is deleted.
for (Map.Entry<String, FirebaseFirestore> entry : instances.entrySet()) {
entry.getValue().shutdownInternal();
instances.remove(entry.getKey());
}
}
}
23 changes: 11 additions & 12 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ public final class FirestoreClient implements RemoteStore.RemoteStoreCallback {
private RemoteStore remoteStore;
private SyncEngine syncEngine;
private EventManager eventManager;
private volatile boolean clientShutdown = false;

// LRU-related
@Nullable private LruGarbageCollector.Scheduler lruScheduler;
Expand Down Expand Up @@ -138,21 +137,21 @@ public Task<Void> enableNetwork() {
/** Shuts down this client, cancels all writes / listeners, and releases all resources. */
public Task<Void> shutdown() {
credentialsProvider.removeChangeListener();
return asyncQueue.enqueue(
return asyncQueue.enqueueAndInitiateShutdown(
() -> {
if (!this.clientShutdown) {
remoteStore.shutdown();
persistence.shutdown();
if (lruScheduler != null) {
lruScheduler.stop();
}
this.clientShutdown = true;
remoteStore.shutdown();
persistence.shutdown();
if (lruScheduler != null) {
lruScheduler.stop();
}
});
}

/** Returns true if this client has been shutdown. */
public boolean isShutdown() {
return this.clientShutdown;
// Technically, the asyncQueue is still running, but only accepting tasks related to shutdown
// or supposed to be run after shutdown. It is effectively shut down to the eyes of users.
return this.asyncQueue.isShuttingDown();
}

/** Starts listening to a query. */
Expand Down Expand Up @@ -272,8 +271,8 @@ private void initialize(Context context, User user, boolean usePersistence, long
}

private void verifyNotShutdown() {
if (this.clientShutdown) {
throw new IllegalArgumentException("The client has already been shutdown");
if (this.isShutdown()) {
throw new IllegalStateException("The client has already been shutdown");
}
}

Expand Down
Loading