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
--- 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 {