Tidy up TestingHooks code to be simpler and more consistent with the Web and iOS SDKs (#4975)

Author: dconeybe

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java

@@ -15,6 +15,7 @@
 package com.google.firebase.firestore;
 
 import static com.google.common.truth.Truth.assertWithMessage;
+import static com.google.firebase.firestore.remote.TestingHooksUtil.captureExistenceFilterMismatches;
 import static com.google.firebase.firestore.testutil.IntegrationTestUtil.isRunningAgainstEmulator;
 import static com.google.firebase.firestore.testutil.IntegrationTestUtil.nullList;
 import static com.google.firebase.firestore.testutil.IntegrationTestUtil.querySnapshotToIds;
@@ -37,7 +38,7 @@
 import com.google.android.gms.tasks.Task;
 import com.google.common.collect.Lists;
 import com.google.firebase.firestore.Query.Direction;
-import com.google.firebase.firestore.remote.ExistenceFilterMismatchListener;
+import com.google.firebase.firestore.remote.TestingHooksUtil.ExistenceFilterMismatchInfo;
 import com.google.firebase.firestore.testutil.EventAccumulator;
 import com.google.firebase.firestore.testutil.IntegrationTestUtil;
 import java.util.ArrayList;
@@ -47,6 +48,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.Semaphore;
+import java.util.concurrent.atomic.AtomicReference;
 import org.junit.After;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -1079,25 +1081,14 @@ public void resumingAQueryShouldUseExistenceFilterToDetectDeletes() throws Excep
 
     // Resume the query and save the resulting snapshot for verification. Use some internal testing
     // hooks to "capture" the existence filter mismatches to verify them.
-    ExistenceFilterMismatchListener existenceFilterMismatchListener =
-        new ExistenceFilterMismatchListener();
-    QuerySnapshot snapshot2;
-    ExistenceFilterMismatchListener.ExistenceFilterMismatchInfo existenceFilterMismatchInfo;
-    try {
-      existenceFilterMismatchListener.startListening();
-      snapshot2 = waitFor(collection.get());
-      // TODO(b/270731363): Remove the "if" condition below once the Firestore Emulator is fixed
-      //  to send an existence filter.
-      if (isRunningAgainstEmulator()) {
-        existenceFilterMismatchInfo = null;
-      } else {
-        existenceFilterMismatchInfo =
-            existenceFilterMismatchListener.getOrWaitForExistenceFilterMismatch(
-                /*timeoutMillis=*/ 5000);
-      }
-    } finally {
-      existenceFilterMismatchListener.stopListening();
-    }
+    AtomicReference<QuerySnapshot> snapshot2Ref = new AtomicReference<>();
+    ArrayList<ExistenceFilterMismatchInfo> existenceFilterMismatches =
+        captureExistenceFilterMismatches(
+            () -> {
+              QuerySnapshot querySnapshot = waitFor(collection.get());
+              snapshot2Ref.set(querySnapshot);
+            });
+    QuerySnapshot snapshot2 = snapshot2Ref.get();
 
     // Verify that the snapshot from the resumed query contains the expected documents; that is,
     // that it contains the 50 documents that were _not_ deleted.
@@ -1131,9 +1122,10 @@ public void resumingAQueryShouldUseExistenceFilterToDetectDeletes() throws Excep
 
     // Verify that Watch sent an existence filter with the correct counts when the query was
     // resumed.
-    assertWithMessage("Watch should have sent an existence filter")
-        .that(existenceFilterMismatchInfo)
-        .isNotNull();
+    assertWithMessage("Watch should have sent exactly 1 existence filter")
+        .that(existenceFilterMismatches)
+        .hasSize(1);
+    ExistenceFilterMismatchInfo existenceFilterMismatchInfo = existenceFilterMismatches.get(0);
     assertWithMessage("localCacheCount")
         .that(existenceFilterMismatchInfo.localCacheCount())
         .isEqualTo(100);

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/ExistenceFilterMismatchListener.java

@@ -0,0 +1,83 @@
+// Copyright 2023 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.firebase.firestore.remote;
+
+import androidx.annotation.NonNull;
+import com.google.firebase.firestore.ListenerRegistration;
+import java.util.ArrayList;
+
+/** Convenience functions for accessing the testing hooks from {@link TestingHooks}. */
+public final class TestingHooksUtil {
+
+  /** Private constructor to prevent instantiation. */
+  private TestingHooksUtil() {}
+
+  /**
+   * Captures all existence filter mismatches in the Watch 'Listen' stream that occur during the
+   * execution of the given callback.
+   *
+   * @param callback The callback to invoke; during the invocation of this callback all existence
+   *     filter mismatches will be captured.
+   * @return the captured existence filter mismatches.
+   */
+  public static ArrayList<ExistenceFilterMismatchInfo> captureExistenceFilterMismatches(
+      Runnable callback) {
+    if (callback == null) {
+      throw new NullPointerException("the given callback must not be null");
+    }
+
+    ArrayList<ExistenceFilterMismatchInfo> existenceFilterMismatches = new ArrayList<>();
+
+    ListenerRegistration listenerRegistration =
+        TestingHooks.getInstance()
+            .addExistenceFilterMismatchListener(
+                info -> {
+                  synchronized (existenceFilterMismatches) {
+                    existenceFilterMismatches.add(new ExistenceFilterMismatchInfo(info));
+                  }
+                });
+
+    try {
+      callback.run();
+    } finally {
+      listenerRegistration.remove();
+    }
+
+    // Return a *copy* of the `existenceFilterMismatches` list because, as documented in
+    // addExistenceFilterMismatchListener(), it could *still* get notifications after it is
+    // unregistered and that would be a race condition with the caller accessing the list.
+    synchronized (existenceFilterMismatches) {
+      return new ArrayList<>(existenceFilterMismatches);
+    }
+  }
+
+  /** @see TestingHooks.ExistenceFilterMismatchInfo */
+  public static final class ExistenceFilterMismatchInfo {
+
+    private final TestingHooks.ExistenceFilterMismatchInfo info;
+
+    ExistenceFilterMismatchInfo(@NonNull TestingHooks.ExistenceFilterMismatchInfo info) {
+      this.info = info;
+    }
+
+    public int localCacheCount() {
+      return info.localCacheCount();
+    }
+
+    public int existenceFilterCount() {
+      return info.existenceFilterCount();
+    }
+  }
+}

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/TestingHooksUtil.java

@@ -21,7 +21,6 @@
 import androidx.annotation.VisibleForTesting;
 import com.google.auto.value.AutoValue;
 import com.google.firebase.firestore.ListenerRegistration;
-import com.google.firebase.firestore.util.Executors;
 import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.atomic.AtomicReference;
 
@@ -50,21 +49,18 @@ static TestingHooks getInstance() {
   }
 
   /**
-   * Asynchronously notifies all registered {@link ExistenceFilterMismatchListener}` listeners
+   * Synchronously notifies all registered {@link ExistenceFilterMismatchListener}` listeners
    * registered via {@link #addExistenceFilterMismatchListener}.
    *
    * @param info Information about the existence filter mismatch to deliver to the listeners.
    */
   void notifyOnExistenceFilterMismatch(@NonNull ExistenceFilterMismatchInfo info) {
     for (AtomicReference<ExistenceFilterMismatchListener> listenerRef :
         existenceFilterMismatchListeners) {
-      Executors.BACKGROUND_EXECUTOR.execute(
-          () -> {
-            ExistenceFilterMismatchListener listener = listenerRef.get();
-            if (listener != null) {
-              listener.onExistenceFilterMismatch(info);
-            }
-          });
+      ExistenceFilterMismatchListener listener = listenerRef.get();
+      if (listener != null) {
+        listener.onExistenceFilterMismatch(info);
+      }
     }
   }
 
@@ -76,15 +72,17 @@ void notifyOnExistenceFilterMismatch(@NonNull ExistenceFilterMismatchInfo info)
    * particular ordering. If a given callback is registered multiple times then it will be notified
    * multiple times, once per registration.
    *
-   * <p>The thread on which the callback occurs is unspecified; listeners should perform their work
-   * as quickly as possible and return to avoid blocking any critical work. In particular, the
-   * listener callbacks should <em>not</em> block or perform long-running operations. Listener
-   * callbacks can occur concurrently with other callbacks on the same and other listeners.
+   * <p>The listener callbacks are performed synchronously in `NotifyOnExistenceFilterMismatch()`;
+   * therefore, listeners should perform their work as quickly as possible and return to avoid
+   * blocking any critical work. In particular, the listener callbacks should <em>not</em> block or
+   * perform long-running operations.
    *
    * @param listener the listener to register.
    * @return an object that unregisters the given listener via its {@link
    *     ListenerRegistration#remove} method; only the first unregistration request does anything;
-   *     all subsequent requests do nothing.
+   *     all subsequent requests do nothing. Note that due to inherent race conditions it is
+   *     technically possible, although unlikely, that callbacks could still occur <em>after</em>
+   *     unregistering.
    */
   ListenerRegistration addExistenceFilterMismatchListener(
       @NonNull ExistenceFilterMismatchListener listener) {