Bug 1350442 - Remove redundant storeDone from the RepositorySession class r=rnewman draft
authorGrigory Kruglov <gkruglov@mozilla.com>
Fri, 31 Mar 2017 19:00:13 -0400
changeset 555781 41767ad502bd5ad8a0a487235bfdca8cf0d0c927
parent 555780 9cef0ba0c76e0ef11a32585f1c73508f87337a02
child 622705 2a3cb9f8b14c485df10ee6839f61e5ea244fce41
push id52346
push userbmo:gkruglov@mozilla.com
push dateTue, 04 Apr 2017 21:15:43 +0000
reviewersrnewman
bugs1350442
milestone55.0a1
Bug 1350442 - Remove redundant storeDone from the RepositorySession class r=rnewman Confusion between storeDone() and storeDone(long end) resulted in certain sessions (bookmarks and form history) not overriding the current method. As a result, their final "flush the queue" methods weren't being called by the buffering middleware. This patch removes the storeDone(long end) method, making such confusion a non-issue. Given that a lot of sessions tend to build up buffers which they need to then flush after a storeDone() call, passing in a timestamp into that method doesn't make sense. Instead, let's supply a default implementation in RepositorySession which calls onStoreCompleted(endTimestamp) with current time, and allow sessions to override this method and own the onStoreCompleted(endTimestamp) call. MozReview-Commit-ID: 84o7aAL8RPC
mobile/android/services/src/main/java/org/mozilla/gecko/sync/middleware/BufferingMiddlewareRepositorySession.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/middleware/MiddlewareRepositorySession.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/RepositorySession.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserBookmarksRepositorySession.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserHistoryRepositorySession.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/FormHistoryRepositorySession.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/BatchingUploader.java
mobile/android/tests/background/junit3/src/org/mozilla/gecko/background/testhelpers/WBORepository.java
mobile/android/tests/background/junit4/src/org/mozilla/android/sync/test/SynchronizerHelpers.java
mobile/android/tests/background/junit4/src/org/mozilla/gecko/background/testhelpers/WBORepository.java
mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/middleware/BufferingMiddlewareRepositorySessionTest.java
mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/uploaders/BatchingUploaderTest.java
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/middleware/BufferingMiddlewareRepositorySession.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/middleware/BufferingMiddlewareRepositorySession.java
@@ -87,62 +87,57 @@ import java.util.concurrent.Executors;
      * Incoming records are mapped to existing ones via GUIDs.
      */
     @Override
     public void storeIncomplete() {
         bufferStorage.flush();
     }
 
     @Override
-    public void storeDone() {
-        storeDone(System.currentTimeMillis());
-    }
-
-    @Override
     public void storeFlush() {
         bufferStorage.flush();
     }
 
     @Override
-    public void storeDone(final long end) {
+    public void storeDone() {
         bufferStorage.flush();
 
         // Determine if we have enough time to merge the buffer data.
         // If we don't have enough time now, we keep our buffer and try again later.
         if (!mayProceedToMergeBuffer()) {
             super.abort();
             storeDelegate.deferredStoreDelegate(storeWorkQueue).onStoreFailed(new SyncDeadlineReachedException());
             return;
         }
 
-        doMergeBuffer(end);
+        doMergeBuffer();
     }
 
     @VisibleForTesting
-    /* package-local */ void doMergeBuffer(long end) {
+    /* package-local */ void doMergeBuffer() {
         final Collection<Record> bufferData = bufferStorage.all();
 
         // Trivial case of an empty buffer.
         if (bufferData.isEmpty()) {
-            super.storeDone(end);
+            super.storeDone();
             return;
         }
 
         // Let session handle actual storing of records as it pleases.
         // See Bug 1332094 which is concerned with allowing merge to proceed transactionally.
         try {
             for (Record record : bufferData) {
                 this.inner.store(record);
             }
         } catch (NoStoreDelegateException e) {
             // At this point we should have a store delegate set on the session, so this won't happen.
         }
 
         // Let session know that there are no more records to store.
-        super.storeDone(end);
+        super.storeDone();
     }
 
     /**
      * Session abnormally aborted. This doesn't mean our so-far buffered data is invalid.
      * Clean up after ourselves, if there's anything to clean up.
      */
     @Override
     public void abort() {
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/middleware/MiddlewareRepositorySession.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/middleware/MiddlewareRepositorySession.java
@@ -159,21 +159,16 @@ public abstract class MiddlewareReposito
   }
 
   @Override
   public void storeDone() {
     inner.storeDone();
   }
 
   @Override
-  public void storeDone(long storeEnd) {
-    inner.storeDone(storeEnd);
-  }
-
-  @Override
   public boolean shouldSkip() {
     return inner.shouldSkip();
   }
 
   @Override
   public boolean dataAvailable() {
     return inner.dataAvailable();
   }
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/RepositorySession.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/RepositorySession.java
@@ -127,20 +127,21 @@ public abstract class RepositorySession 
   public abstract void store(Record record) throws NoStoreDelegateException;
 
   public void storeIncomplete() {}
 
   public void storeDone() {
     // Our default behavior will be to assume that the Runnable is
     // executed as soon as all the stores synchronously finish, so
     // our end timestamp can just be… now.
-    storeDone(now());
-  }
-
-  public void storeDone(final long end) {
+    // Sessions may override this behavior if the above assumption is incorrect.
+    // For example, a session may choose to build up buffers which will need to be flushed in
+    // storeDone, and so it will need to call onStoreComplete with the end timestamp after those
+    // operations complete.
+    final long end = now();
     Logger.debug(LOG_TAG, "Scheduling onStoreCompleted for after storing is done: " + end);
     Runnable command = new Runnable() {
       @Override
       public void run() {
         storeDelegate.onStoreCompleted(end);
       }
     };
     storeWorkQueue.execute(command);
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserBookmarksRepositorySession.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserBookmarksRepositorySession.java
@@ -971,17 +971,17 @@ public class AndroidBrowserBookmarksRepo
   @Override
   public void storeDone() {
     Runnable command = new Runnable() {
       @Override
       public void run() {
         try {
           finishUp();
         } finally {
-          AndroidBrowserBookmarksRepositorySession.super.storeDone();
+          storeDelegate.deferredStoreDelegate(storeWorkQueue).onStoreCompleted(now());
         }
       }
     };
     storeWorkQueue.execute(command);
   }
 
   @Override
   protected String buildRecordString(Record record) {
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserHistoryRepositorySession.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserHistoryRepositorySession.java
@@ -181,21 +181,16 @@ public class AndroidBrowserHistoryReposi
       } catch (NullCursorException e) {
         throw e;
       }
       trackRecord(succeeded);
       storeDelegate.onRecordStoreSucceeded(succeeded.guid); // At this point, we are really inserted.
     }
   }
 
-  @Override
-  public void storeDone() {
-    storeDone(System.currentTimeMillis());
-  }
-
   /**
    * We need to flush our internal buffer of records in case of any interruptions of record flow
    * from our "source". Downloader might be maintaining a "high-water-mark" based on the records
    * it tried to store, so it's pertinent that all of the records that were queued for storage
    * are eventually persisted.
    */
   @Override
   public void storeIncomplete() {
@@ -209,24 +204,24 @@ public class AndroidBrowserHistoryReposi
             Logger.warn(LOG_TAG, "Error flushing records to database.", e);
           }
         }
       }
     });
   }
 
   @Override
-  public void storeDone(final long end) {
+  public void storeDone() {
     storeWorkQueue.execute(new Runnable() {
       @Override
       public void run() {
         synchronized (recordsBufferMonitor) {
           try {
             flushNewRecords();
           } catch (Exception e) {
             Logger.warn(LOG_TAG, "Error flushing records to database.", e);
           }
         }
-        AndroidBrowserHistoryRepositorySession.super.storeDone(end);
+        storeDelegate.deferredStoreDelegate(storeWorkQueue).onStoreCompleted(now());
       }
     });
   }
 }
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/FormHistoryRepositorySession.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/FormHistoryRepositorySession.java
@@ -481,20 +481,20 @@ public class FormHistoryRepositorySessio
     Runnable command = new Runnable() {
       @Override
       public void run() {
         Logger.debug(LOG_TAG, "Checking for residual form history items to insert.");
         try {
           synchronized (recordsBufferMonitor) {
             flushInsertQueue();
           }
-          storeDone(now());
+          storeDelegate.deferredStoreDelegate(storeWorkQueue).onStoreCompleted(now());
         } catch (Exception e) {
           // XXX TODO
-          storeDelegate.onRecordStoreFailed(e, null);
+          storeDelegate.deferredStoreDelegate(storeWorkQueue).onRecordStoreFailed(e, null);
         }
       }
     };
     storeWorkQueue.execute(command);
   }
 
   /**
    * Called when a regular record with locally unknown GUID has been fetched
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/BatchingUploader.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/BatchingUploader.java
@@ -202,17 +202,17 @@ public class BatchingUploader {
             @Override
             public void run() {
                 flush(true, true);
             }
         });
     }
 
     /* package-local */ void finished(AtomicLong lastModifiedTimestamp) {
-        repositorySession.storeDone(lastModifiedTimestamp.get());
+        sessionStoreDelegate.deferredStoreDelegate(executor).onStoreCompleted(lastModifiedTimestamp.get());
     }
 
     // Will be called from a thread dispatched by PayloadDispatcher.
     // NB: Access to `uploaderMeta.isUnlimited` is guarded by the payloadLock.
     /* package-local */ void setUnlimitedMode(boolean isUnlimited) {
         // If we know for sure that we're not in a batching mode,
         // consider our batch to be of unlimited size.
         this.uploaderMeta.setIsUnlimited(isUnlimited);
--- a/mobile/android/tests/background/junit3/src/org/mozilla/gecko/background/testhelpers/WBORepository.java
+++ b/mobile/android/tests/background/junit3/src/org/mozilla/gecko/background/testhelpers/WBORepository.java
@@ -182,19 +182,20 @@ public class WBORepository extends Repos
     @Override
     public void begin(RepositorySessionBeginDelegate delegate) throws InvalidSessionTransitionException {
       this.wbos = wboRepository.cloneWBOs();
       stats.begun = now();
       super.begin(delegate);
     }
 
     @Override
-    public void storeDone(long end) {
+    public void storeDone() {
       // TODO: this is not guaranteed to be called after all of the record
       // store callbacks have completed!
+      final long end = now();
       if (stats.storeBegan < 0) {
         stats.storeBegan = end;
       }
       stats.storeCompleted = end;
       storeDelegate.deferredStoreDelegate(delegateExecutor).onStoreCompleted(end);
     }
   }
 
--- a/mobile/android/tests/background/junit4/src/org/mozilla/android/sync/test/SynchronizerHelpers.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/android/sync/test/SynchronizerHelpers.java
@@ -198,17 +198,17 @@ public class SynchronizerHelpers {
         synchronized (batch) {
           flush();
           // Do this in a Runnable so that the timestamp is grabbed after any upload.
           final Runnable r = new Runnable() {
             @Override
             public void run() {
               synchronized (batch) {
                 Logger.trace("XXX", "Calling storeDone.");
-                storeDone(now());
+                storeDelegate.onStoreCompleted(now());
               }
             }
           };
           storeWorkQueue.execute(r);
         }
       }
     }
     public BatchFailStoreWBORepository(int batchSize) {
--- a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/background/testhelpers/WBORepository.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/background/testhelpers/WBORepository.java
@@ -181,19 +181,20 @@ public class WBORepository extends Repos
     @Override
     public void begin(RepositorySessionBeginDelegate delegate) throws InvalidSessionTransitionException {
       this.wbos = wboRepository.cloneWBOs();
       stats.begun = now();
       super.begin(delegate);
     }
 
     @Override
-    public void storeDone(long end) {
+    public void storeDone() {
       // TODO: this is not guaranteed to be called after all of the record
       // store callbacks have completed!
+      final long end = now();
       if (stats.storeBegan < 0) {
         stats.storeBegan = end;
       }
       stats.storeCompleted = end;
       storeDelegate.deferredStoreDelegate(delegateExecutor).onStoreCompleted(end);
     }
   }
 
--- a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/middleware/BufferingMiddlewareRepositorySessionTest.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/middleware/BufferingMiddlewareRepositorySessionTest.java
@@ -75,18 +75,18 @@ public class BufferingMiddlewareReposito
         verify(innerRepositorySession, never()).store(record);
         verify(innerRepositorySession, never()).store(record1);
         verify(innerRepositorySession, never()).store(record2);
     }
 
     @Test
     public void storeDone() throws Exception {
         // Trivial case, no records to merge.
-        bufferingSession.doMergeBuffer(123L);
-        verify(innerRepositorySession, times(1)).storeDone(123L);
+        bufferingSession.doMergeBuffer();
+        verify(innerRepositorySession, times(1)).storeDone();
         verify(innerRepositorySession, never()).store(any(Record.class));
 
         // Reset call counters.
         reset(innerRepositorySession);
 
         // Now store some records.
         MockRecord record = new MockRecord("guid1", null, 1, false);
         bufferingSession.store(record);
@@ -97,25 +97,25 @@ public class BufferingMiddlewareReposito
         MockRecord record3 = new MockRecord("guid3", null, 5, false);
         bufferingSession.store(record3);
 
         // NB: same guid as above.
         MockRecord record4 = new MockRecord("guid3", null, -1, false);
         bufferingSession.store(record4);
 
         // Done storing.
-        bufferingSession.doMergeBuffer(123L);
+        bufferingSession.doMergeBuffer();
 
         // Ensure all records where stored in the wrapped session.
         verify(innerRepositorySession, times(1)).store(record);
         verify(innerRepositorySession, times(1)).store(record2);
         verify(innerRepositorySession, times(1)).store(record4);
 
         // Ensure storeDone was called on the wrapped session.
-        verify(innerRepositorySession, times(1)).storeDone(123L);
+        verify(innerRepositorySession, times(1)).storeDone();
 
         // Ensure buffer wasn't cleared on the wrapped session.
         assertEquals(3, bufferStorage.all().size());
     }
 
     @Test
     public void storeFlush() throws Exception {
         verify(bufferStorageMocked, times(0)).flush();
--- a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/uploaders/BatchingUploaderTest.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/repositories/uploaders/BatchingUploaderTest.java
@@ -146,17 +146,17 @@ public class BatchingUploaderTest {
 
         @Override
         public void onStoreFailed(Exception e) {
 
         }
 
         @Override
         public RepositorySessionStoreDelegate deferredStoreDelegate(ExecutorService executor) {
-            return null;
+            return this;
         }
     }
 
     private ExecutorService workQueue;
     private RepositorySessionStoreDelegate storeDelegate;
 
     @Before
     public void setUp() throws Exception {