Implement LoadBundleTask #2296

schmidt-sebastian · 2021-01-06T17:12:07Z

This implements the logic for LoadBundleTask. It's basically a super advanced copy/paste job:

It takes some API comments from the Web implementation (https://github.com/firebase/firebase-js-sdk/blob/master/packages/firebase/index.d.ts#L8374)
It relies on the Android Task API whenever possible. For progress updates, the implementation is inspired by cs/piper///depot/google3/java/com/google/android/gmscore/integ/client/tasks/src/com/google/android/gms/tasks/TaskImpl.java
Most Task API comments are copied from StorageTask (https://github.com/firebase/firebase-android-sdk/blob/master/firebase-storage/src/main/java/com/google/firebase/storage/StorageTask.java)

google-oss-bot · 2021-01-06T17:20:03Z

Coverage Report

Affected SDKs

`firebase-firestore`

SDK overall coverage changed from 46.83% (cdf06e2) to 47.11% (e486b67d) by +0.29%.

Filename	Base (`cdf06e2`)	Head (e486b67d)	Diff
GrpcCallProvider.java	68.18%	57.95%	-10.23%
LoadBundleTask.java	0.00%	82.80%	+82.80%
LoadBundleTaskProgress.java	0.00%	74.29%	+74.29%
OnProgressListener.java	?	0.00%	?

firebase-storage

SDK overall coverage changed from 85.52% (cdf06e2) to 85.83% (e486b67d) by +0.30%.

Filename Base (cdf06e2) Head (e486b67d) Diff

Test Logs

Notes

HTML coverage reports can be produced locally with ./gradlew <product>:checkCoverage.
Report files are located at <product-build-dir>/reports/jacoco/.

Head commit (e486b67d) is created by Prow via merging commits: cdf06e2 86f3518.

google-oss-bot · 2021-01-06T17:23:21Z

Binary Size Report

Affected SDKs

firebase-common

Type Base (cdf06e2) Head (e486b67d) Diff

aar 50.1 kB 53.8 kB +3.62 kB (+7.2%)

apk (release) 635 kB 636 kB +860 B (+0.1%)
firebase-firestore

Type Base (cdf06e2) Head (e486b67d) Diff

aar 1.04 MB 1.04 MB +5.03 kB (+0.5%)

apk (release) 3.16 MB 3.16 MB +776 B (+0.0%)
firebase-storage

Type Base (cdf06e2) Head (e486b67d) Diff

aar 119 kB 116 kB -3.59 kB (-3.0%)

apk (release) 961 kB 961 kB -8 B (-0.0%)

Test Logs

Notes

Head commit (e486b67d) is created by Prow via merging commits: cdf06e2 86f3518.

dconeybe · 2021-01-06T20:27:29Z

firebase-firestore/src/main/java/com/google/firebase/firestore/LoadBundleTask.java

 /* package */ class LoadBundleTask extends Task<LoadBundleTaskProgress> {
+  private final Object lock = new Object();
+
+  /** The last progress update. Null if not yet available. */


Since "Null" is a case-sensitive keyword, I suggest fixing it to the correct case. You could even just say

"The last progress update, or {@code null} if not yet available."

Done (used your sugestion)

dconeybe · 2021-01-06T20:27:54Z

firebase-firestore/src/main/java/com/google/firebase/firestore/LoadBundleTask.java

+  private final TaskCompletionSource<LoadBundleTaskProgress> completionSource;
+
+  /**
+   * A delegate task derived from `completionSource`. All APIs events that don't involve progress


nit: "APIs" -> "API".

Also, Javadoc does not do markdown processing, so `completionSource` should be {@code completionSource}.

Done and done.

dconeybe · 2021-01-06T20:30:49Z

firebase-firestore/src/main/java/com/google/firebase/firestore/LoadBundleTask.java

+
+  /** A queue of active progress listeners. */
+  @GuardedBy("lock")
+  private final Queue<ManagedListener> progressListenerQueue;


Why is Queue chosen as the container, instead of, say, ArrayList? It looks like the main benefits of a queue (optimized operations for adding to the beginning and removing from the end) are not utilized. If Queue is the data structure you want, consider renaming the variable from progressListenerQueue to just progressListeners since the fact that it's a queue is not particularly meaningful. Also, consider just making this object's type ArrayDeque since the Queue abstraction is not really used.

I chose these datastructures based on the implementation in TaskImpl (cs/piper///depot/google3/java/com/google/android/gmscore/integ/client/tasks/src/com/google/android/gms/tasks/TaskCompletionListenerQueue.java;l=20;rcl=350262826), which follows the same pattern as implemented here. There are a couple different datastructures that I could use here, but I'd rather follow an existing pattern, especially when the source is this trustworthy.

I renamed the variable to progressListeners.

dconeybe · 2021-01-06T20:32:54Z

firebase-firestore/src/main/java/com/google/firebase/firestore/LoadBundleTask.java

  }

+  /** Returns {@code true} if the Task has completed successfully; {@code false} otherwise. */


"otherwise" is somewhat ambiguous here. What does it return if the task has not completed? Please document this explicitly. This also applies to the documentation of isCanceled() below.

I do see your suggestion as an improvement, but I would like to push back for now since this is the same documentation that is used in the base class: https://developer.android.com/reference/com/google/android/play/core/tasks/Task#iscomplete

dconeybe · 2021-01-06T20:35:05Z

firebase-firestore/src/main/java/com/google/firebase/firestore/LoadBundleTask.java

+  /**
+   * Adds a listener that is called if the Task completes successfully. The listener will be called
+   * on the main application thread. If the task has already completed successfully, a call to the
+   * listener will be immediately scheduled. If multiple listeners are added, they will be called in


"immediately scheduled" is somewhat ambiguous. Does it mean that it will be called synchronously or that it will be scheduled to be called from the main thread asynchronously? Please elaborate in the comment. Please make any changes to all adaptations of this sentence in the methods that follow.

Same response as above: I would like to keep the documentation in sync with the original API.

dconeybe · 2021-01-06T20:56:37Z

firebase-firestore/src/main/java/com/google/firebase/firestore/LoadBundleTaskProgress.java

+
+    LoadBundleTaskProgress that = (LoadBundleTaskProgress) o;
+
+    if (documentsLoaded != that.documentsLoaded) return false;


Consider writing this as a big chain of logical ands.

e.g. return documentsLoaded == that.documentsLoaded && totalDocuments == that.totalDocuments && ...

This is auto-generated so I will leave as is.

dconeybe · 2021-01-06T21:06:51Z

firebase-firestore/src/main/java/com/google/firebase/firestore/LoadBundleTaskProgress.java

+  @Override
+  public int hashCode() {
+    int result = documentsLoaded;
+    result = 31 * result + totalDocuments;


Consider using Objects.hash() from the standard library instead of implementing your own hash code.

https://docs.oracle.com/javase/8/docs/api/java/util/Objects.html#hash-java.lang.Object...-

It's not available with Android 14 (requires API level 19+).

dconeybe · 2021-01-06T21:13:02Z

firebase-firestore/src/main/java/com/google/firebase/firestore/LoadBundleTaskProgress.java

@@ -27,39 +38,79 @@
  private int totalDocuments;


Should totalDocuments, bytesLoaded, and totalBytes be declared as final as well?

Yeah, good catch.

dconeybe · 2021-01-06T21:18:39Z

firebase-firestore/src/test/java/com/google/firebase/firestore/LoadBundleTaskTest.java

@@ -38,4 +58,208 @@ public void testImplementsAllTaskInterface() {
      }
    }
  }
+
+  @Test
+  public void testSuccessListener() throws InterruptedException {


Please add tests to verify that the correct thread is used to perform the callbacks.

dconeybe · 2021-01-06T21:21:01Z

firebase-firestore/src/main/java/com/google/firebase/firestore/LoadBundleTask.java

+    synchronized (lock) {
+      snapshot = result;
+      for (ManagedListener progressListener : progressListenerQueue) {
+        progressListener.maybeRun(result);


I think this code will throw ConcurrentModificationException if the callback adds or removes a progress listener. Please add a test for this and verify that this scenario is handled correctly. This comment also applies to setException() and updateProgress().

This should work because of how this will be called, since this method will be invoked from the AsyncQueue and the other methods from the Main thread. This is pretty fragile though, so I updated it and added a test.

schmidt-sebastian

Thanks for the thorough review. Feedback addressed.

schmidt-sebastian · 2021-01-07T19:46:36Z

firebase-firestore/src/main/java/com/google/firebase/firestore/LoadBundleTask.java

 /* package */ class LoadBundleTask extends Task<LoadBundleTaskProgress> {
+  private final Object lock = new Object();
+
+  /** The last progress update. Null if not yet available. */


Done (used your sugestion)

schmidt-sebastian · 2021-01-07T19:47:36Z

firebase-firestore/src/main/java/com/google/firebase/firestore/LoadBundleTask.java

+  private final TaskCompletionSource<LoadBundleTaskProgress> completionSource;
+
+  /**
+   * A delegate task derived from `completionSource`. All APIs events that don't involve progress


Done and done.

schmidt-sebastian · 2021-01-07T19:51:02Z

firebase-firestore/src/main/java/com/google/firebase/firestore/LoadBundleTask.java

+
+  /** A queue of active progress listeners. */
+  @GuardedBy("lock")
+  private final Queue<ManagedListener> progressListenerQueue;


I chose these datastructures based on the implementation in TaskImpl (cs/piper///depot/google3/java/com/google/android/gmscore/integ/client/tasks/src/com/google/android/gms/tasks/TaskCompletionListenerQueue.java;l=20;rcl=350262826), which follows the same pattern as implemented here. There are a couple different datastructures that I could use here, but I'd rather follow an existing pattern, especially when the source is this trustworthy.

I renamed the variable to progressListeners.

schmidt-sebastian · 2021-01-07T19:53:03Z

firebase-firestore/src/main/java/com/google/firebase/firestore/LoadBundleTask.java

  }

+  /** Returns {@code true} if the Task has completed successfully; {@code false} otherwise. */


I do see your suggestion as an improvement, but I would like to push back for now since this is the same documentation that is used in the base class: https://developer.android.com/reference/com/google/android/play/core/tasks/Task#iscomplete

schmidt-sebastian · 2021-01-07T19:53:43Z

firebase-firestore/src/main/java/com/google/firebase/firestore/LoadBundleTask.java

+  /**
+   * Adds a listener that is called if the Task completes successfully. The listener will be called
+   * on the main application thread. If the task has already completed successfully, a call to the
+   * listener will be immediately scheduled. If multiple listeners are added, they will be called in


Same response as above: I would like to keep the documentation in sync with the original API.

schmidt-sebastian · 2021-01-07T21:48:04Z

firebase-firestore/src/main/java/com/google/firebase/firestore/LoadBundleTaskProgress.java

+
+  /** If the task failed, returns the exception. Otherwise, returns null. */
+  @Nullable
+  public Exception getError() {


I was trying to use names in the Task API, but failed miserably :) It's also Task#getException

schmidt-sebastian · 2021-01-07T21:48:23Z

firebase-firestore/src/main/java/com/google/firebase/firestore/LoadBundleTaskProgress.java

+
+    LoadBundleTaskProgress that = (LoadBundleTaskProgress) o;
+
+    if (documentsLoaded != that.documentsLoaded) return false;


This is auto-generated so I will leave as is.

schmidt-sebastian · 2021-01-07T21:48:57Z

firebase-firestore/src/main/java/com/google/firebase/firestore/LoadBundleTaskProgress.java

+    if (totalDocuments != that.totalDocuments) return false;
+    if (bytesLoaded != that.bytesLoaded) return false;
+    if (totalBytes != that.totalBytes) return false;
+    return taskState == that.taskState;


Oops. Forgot to regenerate after adding the exception (which we also have to add to the API document)

cc @wu-hui

schmidt-sebastian · 2021-01-07T21:49:20Z

firebase-firestore/src/main/java/com/google/firebase/firestore/LoadBundleTaskProgress.java

+  @Override
+  public int hashCode() {
+    int result = documentsLoaded;
+    result = 31 * result + totalDocuments;


It's not available with Android 14 (requires API level 19+).

schmidt-sebastian · 2021-01-07T21:49:27Z

firebase-firestore/src/test/java/com/google/firebase/firestore/LoadBundleTaskTest.java

@@ -38,4 +58,208 @@ public void testImplementsAllTaskInterface() {
      }
    }
  }
+
+  @Test
+  public void testSuccessListener() throws InterruptedException {


dconeybe

LGTM, with a few optional/minor suggestions.

dconeybe · 2021-01-08T02:54:59Z

firebase-firestore/src/main/java/com/google/firebase/firestore/LoadBundleTask.java

    completionSource.setException(exception);
  }

-  void updateProgress(LoadBundleTaskProgress snapshot) {
+  void updateProgress(LoadBundleTaskProgress progressUpdate) {
+    Queue<ManagedListener> currentListeners;


How frequently is updateProgress() called? If often (e.g. more than 10 times per second) then object creation on each progress update (i.e. creating the new ArrayDeque) can thrash GC.

One pattern I've used in the past to address this is to store the listeners directly in an array. Before making a callback you grab the reference to the array, store it in a local variable, then iterate over it and notify each listener. When a listener is added or removed then you create a new array instead of modifying the array in-place. As long as adding/removing listeners is rare relative to the notifications then it is a win because no object allocation is done to make the callbacks.

I have another idea that is somewhat out of scope but related to this idea of thrashing GC. It is a very optional suggestion. Could you make LoadBundleTaskProgress a mutable object and simply specify the same instance to each progress update? That would avoid having to allocate a new object for each progress update and since all callbacks should just get the most recent data anyways it could be a win.

I thought about this more: Since I am always invoking the callback on an executor, there should only be two scenarios:

updateProgress is executed on the same thread as the callbacks. This means that all execute calls will only run after the loop has finished processing.

updateProgress is executed on a different thread from the callbacks. This means that the other thread won't be able to grab the lock to modify the progress listener.

With that, I am reasonably confident that we don't need the extra copy and can simplify this quite a bit.

(FYI - I could implement your suggestion above by changing the data structure to our internal ImmutableSet/Map implementation, but I do still prefer to keep in sync with Android's Task API.

Ahh yes that makes sense. I missed the fact that the callbacks were being enqueued on an executor. In that case, I rescind my comment about the ConcurrentModificationException.

dconeybe · 2021-01-08T02:59:42Z

firebase-firestore/src/test/java/com/google/firebase/firestore/LoadBundleTaskTest.java

+    task.addOnProgressListener(
+        p -> {
+          assertNotEquals(TEST_THREAD_NAME, Thread.currentThread().getName());
+          latch.countDown();


You may want to call latch.countDown() before assertNotEquals(); otherwise, if the assertion fails then the latch will never count down and the test will hang. Applies here and elsewhere.

Swapping these will allow the test to pass before the assertions trigger (as the scope of the test is only bound by the latches). I think what we need to do is add try/finally blocks.

dconeybe · 2021-01-08T03:00:42Z

firebase-firestore/src/test/java/com/google/firebase/firestore/LoadBundleTaskTest.java

+    LoadBundleTask task = new LoadBundleTask();
+    task.addOnProgressListener(
+        p -> {
+          assertNotEquals(TEST_THREAD_NAME, Thread.currentThread().getName());


Can you confirm that this assertion failure will actually cause a test failure? Since the exception will be thrown on some other thread I just don't know if it will cause the test to fail.

The test shows the stacktrace, but doesn't fail. I added a super ugly hack to ensure the test fails when this happens. Let me know if you can think of something better.

Are you able to use org.junit.rules.ErrorCollector? If so, I think it would be a perfect fit for reporting assertion failures from other threads.

https://junit.org/junit4/javadoc/latest/org/junit/rules/ErrorCollector.html

I saw this when I googled around, but it didn't quite strike my mind that this is exactly what I need. Thanks!

Implement LoadBundleTask

0485daf

google-cla bot added the cla: yes Override cla label Jan 6, 2021

google-oss-bot added the size/XL label Jan 6, 2021

schmidt-sebastian requested a review from dconeybe January 6, 2021 17:13

schmidt-sebastian assigned dconeybe Jan 6, 2021

schmidt-sebastian added the api: firestore label Jan 6, 2021

Fix warnings

ee41e57

schmidt-sebastian force-pushed the mrschmidt/bundle/task branch from bea234e to ee41e57 Compare January 6, 2021 17:25

dconeybe reviewed Jan 6, 2021

View reviewed changes

dconeybe assigned schmidt-sebastian and unassigned dconeybe Jan 7, 2021

Review

b5caab8

schmidt-sebastian commented Jan 7, 2021

View reviewed changes

schmidt-sebastian assigned dconeybe and unassigned schmidt-sebastian Jan 7, 2021

Format

ef989a4

dconeybe approved these changes Jan 8, 2021

View reviewed changes

dconeybe assigned schmidt-sebastian and unassigned dconeybe Jan 8, 2021

Feedback

4ea4147

schmidt-sebastian assigned dconeybe and unassigned schmidt-sebastian Jan 8, 2021

dconeybe assigned schmidt-sebastian and unassigned dconeybe Jan 8, 2021

schmidt-sebastian added 2 commits January 7, 2021 21:09

Use collector

0f06a93

Fix order

86f3518

schmidt-sebastian merged commit 76597db into master Jan 8, 2021

schmidt-sebastian added a commit that referenced this pull request Jan 9, 2021

Implement LoadBundleTask (#2296)

f1bd4de

schmidt-sebastian added a commit that referenced this pull request Jan 9, 2021

Implement LoadBundleTask (#2296)

1a1003e

andreaowu pushed a commit that referenced this pull request Jan 29, 2021

Implement LoadBundleTask (#2296)

e03bebd

firebase locked and limited conversation to collaborators Feb 8, 2021

kaibolay deleted the mrschmidt/bundle/task branch September 14, 2022 17:56

		}

		/** Returns {@code true} if the Task has completed successfully; {@code false} otherwise. */


		LoadBundleTaskProgress that = (LoadBundleTaskProgress) o;

		if (documentsLoaded != that.documentsLoaded) return false;

Implement LoadBundleTask #2296

Implement LoadBundleTask #2296

Conversation

schmidt-sebastian commented Jan 6, 2021

google-oss-bot commented Jan 6, 2021 • edited Loading

Coverage Report

Affected SDKs

firebase-firestore

firebase-storage

Test Logs

Notes

google-oss-bot commented Jan 6, 2021 • edited Loading

Binary Size Report

Affected SDKs

firebase-common

firebase-firestore

firebase-storage

Test Logs

Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dconeybe left a comment

Choose a reason for hiding this comment

dconeybe Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-oss-bot commented Jan 6, 2021 •

edited

Loading

`firebase-firestore`

`firebase-storage`

google-oss-bot commented Jan 6, 2021 •

edited

Loading

`firebase-common`

`firebase-firestore`

`firebase-storage`

dconeybe Jan 8, 2021 •

edited

Loading