diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md index 091ad59233b..4a2757b8788 100644 --- a/firebase-firestore/CHANGELOG.md +++ b/firebase-firestore/CHANGELOG.md @@ -1,5 +1,5 @@ # Unreleased - +* [changed] Fail snapshot listeners when Firestore terminates. [#6136](//github.com/firebase/firebase-android-sdk/pull/6136) # 25.1.0 * [feature] Add support for the VectorValue type. [#6154](//github.com/firebase/firebase-android-sdk/pull/6154) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java index 958e5b58c15..0cbb52eb60f 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java @@ -1263,6 +1263,8 @@ public void testCanStopListeningAfterTerminate() { waitFor(instance.terminate()); + assertEquals(eventAccumulator.awaitError().getCode(), Code.ABORTED); + // This should proceed without error. registration.remove(); // Multiple calls should proceed as an effectively no-op. diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ListenerRegistrationTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ListenerRegistrationTest.java index 7ff920888ad..2b8852a2309 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ListenerRegistrationTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ListenerRegistrationTest.java @@ -165,12 +165,13 @@ private void activityScopedListenerStopsListeningWhenActivityStops(Activity acti DocumentReference documentReference = collectionReference.document(); Semaphore events = new Semaphore(0); - collectionReference.addSnapshotListener( - activity, - (value, error) -> { - assertNull(error); - events.release(); - }); + ListenerRegistration listener = + collectionReference.addSnapshotListener( + activity, + (value, error) -> { + assertNull(error); + events.release(); + }); // Initial events waitFor(events, 1); @@ -188,6 +189,8 @@ private void activityScopedListenerStopsListeningWhenActivityStops(Activity acti // No listeners, therefore, there should be no events. waitFor(documentReference.set(map("foo", "new-bar"))); assertEquals(0, events.availablePermits()); + + listener.remove(); } /** @param activity Must be a TestActivity or a TestFragmentActivity */ diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java index 29ca658515e..f15c200ce5e 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java @@ -147,7 +147,7 @@ public void testListenUnlistenRelistenSequenceOfMirrorQueries() { // Unlisten then re-listen limit query. limitRegistration.remove(); - limit.addSnapshotListener(limitAccumulator.listener()); + limitRegistration = limit.addSnapshotListener(limitAccumulator.listener()); // Verify `limit` query still works. data = querySnapshotToValues(limitAccumulator.await()); @@ -165,13 +165,16 @@ public void testListenUnlistenRelistenSequenceOfMirrorQueries() { // Unlisten to limitToLast, update a doc, then relisten to limitToLast limitToLastRegistration.remove(); waitFor(collection.document("a").update(map("k", "a", "sort", -2))); - limitToLast.addSnapshotListener(limitToLastAccumulator.listener()); + limitToLastRegistration = limitToLast.addSnapshotListener(limitToLastAccumulator.listener()); // Verify both query get expected result. data = querySnapshotToValues(limitAccumulator.await()); assertEquals(asList(map("k", "a", "sort", -2L), map("k", "e", "sort", -1L)), data); data = querySnapshotToValues(limitToLastAccumulator.await()); assertEquals(asList(map("k", "e", "sort", -1L), map("k", "a", "sort", -2L)), data); + + limitRegistration.remove(); + limitToLastRegistration.remove(); } @Test @@ -507,19 +510,22 @@ public void watchSurvivesNetworkDisconnect() { Semaphore receivedDocument = new Semaphore(0); - collectionReference.addSnapshotListener( - MetadataChanges.INCLUDE, - (snapshot, error) -> { - if (!snapshot.isEmpty() && !snapshot.getMetadata().isFromCache()) { - receivedDocument.release(); - } - }); + ListenerRegistration listener = + collectionReference.addSnapshotListener( + MetadataChanges.INCLUDE, + (snapshot, error) -> { + if (!snapshot.isEmpty() && !snapshot.getMetadata().isFromCache()) { + receivedDocument.release(); + } + }); waitFor(firestore.disableNetwork()); collectionReference.add(map("foo", FieldValue.serverTimestamp())); waitFor(firestore.enableNetwork()); waitFor(receivedDocument); + + listener.remove(); } @Test diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/SourceTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/SourceTest.java index fd6ed1652e1..571238885e3 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/SourceTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/SourceTest.java @@ -289,15 +289,17 @@ public void getDocumentWhileOfflineWithDifferentGetOptions() { // Create an initial listener for this query (to attempt to disrupt the gets below) and wait for // the listener to deliver its initial snapshot before continuing. TaskCompletionSource source = new TaskCompletionSource<>(); - docRef.addSnapshotListener( - (docSnap, error) -> { - if (error != null) { - source.setException(error); - } else { - source.setResult(null); - } - }); + ListenerRegistration listener = + docRef.addSnapshotListener( + (docSnap, error) -> { + if (error != null) { + source.setException(error); + } else { + source.setResult(null); + } + }); waitFor(source.getTask()); + listener.remove(); Task docTask = docRef.get(Source.CACHE); waitFor(docTask); @@ -339,15 +341,17 @@ public void getCollectionWhileOfflineWithDifferentGetOptions() { // Create an initial listener for this query (to attempt to disrupt the gets below) and wait for // the listener to deliver its initial snapshot before continuing. TaskCompletionSource source = new TaskCompletionSource<>(); - colRef.addSnapshotListener( - (qrySnap, error) -> { - if (error != null) { - source.setException(error); - } else { - source.setResult(null); - } - }); + ListenerRegistration listener = + colRef.addSnapshotListener( + (qrySnap, error) -> { + if (error != null) { + source.setException(error); + } else { + source.setResult(null); + } + }); waitFor(source.getTask()); + listener.remove(); Task qrySnapTask = colRef.get(Source.CACHE); waitFor(qrySnapTask); diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/EventAccumulator.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/EventAccumulator.java index 347a95cea26..40cb6e5eed4 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/EventAccumulator.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/EventAccumulator.java @@ -19,6 +19,7 @@ import com.google.firebase.firestore.DocumentSnapshot; import com.google.firebase.firestore.EventListener; +import com.google.firebase.firestore.FirebaseFirestoreException; import com.google.firebase.firestore.QuerySnapshot; import com.google.firebase.firestore.util.Logger; import java.util.ArrayList; @@ -30,28 +31,45 @@ public class EventAccumulator { private static final int MAX_EVENTS = 10; - private final BlockingQueue events; + private final BlockingQueue events; private boolean rejectAdditionalEvents; public EventAccumulator() { - events = new ArrayBlockingQueue(MAX_EVENTS); + events = new ArrayBlockingQueue<>(MAX_EVENTS); } public EventListener listener() { return (value, error) -> { - hardAssert(error == null, "Unexpected error: %s", error); hardAssert( !rejectAdditionalEvents, "Received event after `assertNoAdditionalEvents()` was called"); - Logger.debug("EventAccumulator", "Received new event: " + value); - events.offer(value); + if (error == null) { + Logger.debug("EventAccumulator", "Received new event: " + value); + events.add(value); + } else { + Logger.debug("EventAccumulator", "Received error: " + error); + events.add(error); + } }; } + public FirebaseFirestoreException awaitError() { + try { + return (FirebaseFirestoreException) events.take(); + } catch (Exception e) { + Logger.debug("EventAccumulator", e.toString()); + throw fail("Failed to receive error"); + } + } + public List await(int numEvents) { try { List result = new ArrayList<>(numEvents); for (int i = 0; i < numEvents; ++i) { - result.add(events.take()); + Object event = events.take(); + if (event instanceof FirebaseFirestoreException) { + fail("Unexpected error: %s", event); + } + result.add((T) event); } return result; } catch (InterruptedException e) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java index c1218829b8a..88617cb2052 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java @@ -581,8 +581,8 @@ public Task runBatch(@NonNull WriteBatch.Function batchFunction) { *

To restart after termination, simply create a new instance of {@code FirebaseFirestore} with * {@link #getInstance()} or {@link #getInstance(FirebaseApp)}. * - *

{@code terminate()} 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 + *

{@code terminate()} does not cancel any pending writes but any write 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. * *

Note: Under normal circumstances, calling {@code terminate()} is not required. This method diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/EventManager.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/EventManager.java index afb8c66278a..f8819a86781 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/EventManager.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/EventManager.java @@ -17,6 +17,7 @@ import static com.google.firebase.firestore.util.Assert.hardAssert; import com.google.firebase.firestore.EventListener; +import com.google.firebase.firestore.FirebaseFirestoreException; import com.google.firebase.firestore.ListenSource; import com.google.firebase.firestore.core.SyncEngine.SyncEngineCallback; import com.google.firebase.firestore.util.Util; @@ -265,4 +266,14 @@ public void handleOnlineStateChange(OnlineState onlineState) { raiseSnapshotsInSyncEvent(); } } + + public void abortAllTargets() { + FirebaseFirestoreException error = Util.exceptionFromStatus(Status.ABORTED); + for (QueryListenersInfo info : queries.values()) { + for (QueryListener listener : info.listeners) { + listener.onError(error); + } + } + queries.clear(); + } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java index 6e2d9b87b84..7409af54bd9 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java @@ -148,6 +148,7 @@ public Task enableNetwork() { public Task terminate() { authProvider.removeChangeListener(); appCheckProvider.removeChangeListener(); + asyncQueue.enqueueAndForgetEvenAfterShutdown(() -> eventManager.abortAllTargets()); return asyncQueue.enqueueAndInitiateShutdown( () -> { remoteStore.shutdown();