From db539089b3c587ba6dba3de197c75336c054058e Mon Sep 17 00:00:00 2001 From: Marina Samuel Date: Thu, 15 Mar 2012 01:25:14 -0700 Subject: [PATCH] Bug 729248 - Smarter upload of our clients record. r=rnewman --- .../android/base/sync/SyncConfiguration.java | 10 ++ .../sync/delegates/ClientsDataDelegate.java | 1 + .../android/ClientsDatabaseAccessor.java | 4 +- .../repositories/domain/ClientRecord.java | 23 +++- .../domain/ClientRecordFactory.java | 1 - mobile/android/base/sync/setup/Constants.java | 5 - .../sync/stage/SyncClientsEngineStage.java | 115 +++++++++++++++--- .../base/sync/syncadapter/SyncAdapter.java | 5 + 8 files changed, 132 insertions(+), 32 deletions(-) diff --git a/mobile/android/base/sync/SyncConfiguration.java b/mobile/android/base/sync/SyncConfiguration.java index eb993d721d3..ca615a1b846 100644 --- a/mobile/android/base/sync/SyncConfiguration.java +++ b/mobile/android/base/sync/SyncConfiguration.java @@ -190,6 +190,8 @@ public class SyncConfiguration implements CredentialsSource { public String prefsPath; public PrefsSource prefsSource; + public static final String CLIENT_RECORD_TIMESTAMP = "serverClientRecordTimestamp"; + /** * Create a new SyncConfiguration instance. Pass in a PrefsSource to * provide access to preferences. @@ -369,4 +371,12 @@ public class SyncConfiguration implements CredentialsSource { public Editor getEditor() { return this.getPrefs().edit(); } + + public void persistServerClientRecordTimestamp(long timestamp) { + getEditor().putLong(SyncConfiguration.CLIENT_RECORD_TIMESTAMP, timestamp).commit(); + } + + public long getPersistedServerClientRecordTimestamp() { + return getPrefs().getLong(SyncConfiguration.CLIENT_RECORD_TIMESTAMP, 0); + } } diff --git a/mobile/android/base/sync/delegates/ClientsDataDelegate.java b/mobile/android/base/sync/delegates/ClientsDataDelegate.java index 84b555f99f3..c03a903085d 100644 --- a/mobile/android/base/sync/delegates/ClientsDataDelegate.java +++ b/mobile/android/base/sync/delegates/ClientsDataDelegate.java @@ -9,4 +9,5 @@ public interface ClientsDataDelegate { public String getClientName(); public void setClientsCount(int clientsCount); public int getClientsCount(); + public boolean isLocalGUID(String guid); } diff --git a/mobile/android/base/sync/repositories/android/ClientsDatabaseAccessor.java b/mobile/android/base/sync/repositories/android/ClientsDatabaseAccessor.java index 102a2fec3eb..4bf3af6bfc7 100644 --- a/mobile/android/base/sync/repositories/android/ClientsDatabaseAccessor.java +++ b/mobile/android/base/sync/repositories/android/ClientsDatabaseAccessor.java @@ -11,13 +11,13 @@ import java.util.Map; import org.mozilla.gecko.sync.repositories.NullCursorException; import org.mozilla.gecko.sync.repositories.domain.ClientRecord; -import org.mozilla.gecko.sync.setup.Constants; import android.content.Context; import android.database.Cursor; public class ClientsDatabaseAccessor { + public static final String PROFILE_ID = "default"; // Generic profile id for now, until multiple profiles are implemented. public static final String LOG_TAG = "ClientsDatabaseAccessor"; private ClientsDatabase db; @@ -102,7 +102,7 @@ public class ClientsDatabaseAccessor { } private String getProfileId() { - return Constants.PROFILE_ID; + return ClientsDatabaseAccessor.PROFILE_ID; } public void wipe() { diff --git a/mobile/android/base/sync/repositories/domain/ClientRecord.java b/mobile/android/base/sync/repositories/domain/ClientRecord.java index bfb4ff89649..d0763a22ad1 100644 --- a/mobile/android/base/sync/repositories/domain/ClientRecord.java +++ b/mobile/android/base/sync/repositories/domain/ClientRecord.java @@ -4,14 +4,23 @@ package org.mozilla.gecko.sync.repositories.domain; +import org.json.simple.JSONArray; import org.mozilla.gecko.sync.ExtendedJSONObject; +import org.mozilla.gecko.sync.Logger; +import org.mozilla.gecko.sync.NonArrayJSONException; import org.mozilla.gecko.sync.Utils; import org.mozilla.gecko.sync.repositories.android.RepoUtils; -import org.mozilla.gecko.sync.setup.Constants; public class ClientRecord extends Record { + private static final String LOG_TAG = "ClientRecord"; - public static final String COLLECTION_NAME = "clients"; + public static final String CLIENT_TYPE = "mobile"; + public static final String COLLECTION_NAME = "clients"; + public static final String DEFAULT_CLIENT_NAME = "Default Name"; + + public String name = ClientRecord.DEFAULT_CLIENT_NAME; + public String type = ClientRecord.CLIENT_TYPE; + public JSONArray commands; public ClientRecord(String guid, String collection, long lastModified, boolean deleted) { super(guid, collection, lastModified, deleted); @@ -33,13 +42,17 @@ public class ClientRecord extends Record { this(Utils.generateGuid(), COLLECTION_NAME, 0, false); } - public String name = Constants.DEFAULT_CLIENT_NAME; - public String type = Constants.CLIENT_TYPE; - @Override protected void initFromPayload(ExtendedJSONObject payload) { this.name = (String) payload.get("name"); this.type = (String) payload.get("type"); + + try { + commands = payload.getArray("commands"); + } catch (NonArrayJSONException e) { + Logger.debug(LOG_TAG, "Got non-array commands in client record " + guid, e); + commands = null; + } } @Override diff --git a/mobile/android/base/sync/repositories/domain/ClientRecordFactory.java b/mobile/android/base/sync/repositories/domain/ClientRecordFactory.java index 9a549f3507a..318fd3bab22 100644 --- a/mobile/android/base/sync/repositories/domain/ClientRecordFactory.java +++ b/mobile/android/base/sync/repositories/domain/ClientRecordFactory.java @@ -8,7 +8,6 @@ import org.mozilla.gecko.sync.CryptoRecord; import org.mozilla.gecko.sync.repositories.RecordFactory; public class ClientRecordFactory extends RecordFactory { - @Override public Record createRecord(Record record) { ClientRecord r = new ClientRecord(); diff --git a/mobile/android/base/sync/setup/Constants.java b/mobile/android/base/sync/setup/Constants.java index de04a7c475a..bdde3e33b15 100644 --- a/mobile/android/base/sync/setup/Constants.java +++ b/mobile/android/base/sync/setup/Constants.java @@ -17,11 +17,6 @@ public class Constants { public static final String CLIENT_NAME = "account.clientName"; public static final String NUM_CLIENTS = "account.numClients"; - // Constants for client records. - public static final String PROFILE_ID = "default"; // Generic profile id for now, until multiple profiles are implemented. - public static final String CLIENT_TYPE = "mobile"; - public static final String DEFAULT_CLIENT_NAME = "Default Name"; - // Constants for Activities. public static final String INTENT_EXTRA_IS_SETUP = "isSetup"; public static final String INTENT_EXTRA_IS_PAIR = "isPair"; diff --git a/mobile/android/base/sync/stage/SyncClientsEngineStage.java b/mobile/android/base/sync/stage/SyncClientsEngineStage.java index b2f73f1c669..089857aa3e1 100644 --- a/mobile/android/base/sync/stage/SyncClientsEngineStage.java +++ b/mobile/android/base/sync/stage/SyncClientsEngineStage.java @@ -7,12 +7,15 @@ package org.mozilla.gecko.sync.stage; import java.io.UnsupportedEncodingException; import java.net.URI; import java.net.URISyntaxException; +import java.util.concurrent.atomic.AtomicInteger; +import org.json.simple.JSONArray; import org.mozilla.gecko.sync.CryptoRecord; import org.mozilla.gecko.sync.GlobalSession; import org.mozilla.gecko.sync.HTTPFailureException; import org.mozilla.gecko.sync.Logger; import org.mozilla.gecko.sync.NoCollectionKeysSetException; +import org.mozilla.gecko.sync.Utils; import org.mozilla.gecko.sync.crypto.CryptoException; import org.mozilla.gecko.sync.crypto.KeyBundle; import org.mozilla.gecko.sync.delegates.ClientsDataDelegate; @@ -27,9 +30,13 @@ import org.mozilla.gecko.sync.repositories.android.RepoUtils; import org.mozilla.gecko.sync.repositories.domain.ClientRecord; import org.mozilla.gecko.sync.repositories.domain.ClientRecordFactory; +import ch.boye.httpclientandroidlib.HttpStatus; + public class SyncClientsEngineStage implements GlobalSyncStage { - protected static final String LOG_TAG = "SyncClientsEngineStage"; - protected static final String COLLECTION_NAME = "clients"; + public static final String LOG_TAG = "SyncClientsEngineStage"; + public static final String COLLECTION_NAME = "clients"; + public static final int CLIENTS_TTL_REFRESH = 604800000; // 7 days + public static final int MAX_UPLOAD_FAILURE_COUNT = 5; protected GlobalSession session; protected final ClientRecordFactory factory = new ClientRecordFactory(); @@ -37,8 +44,9 @@ public class SyncClientsEngineStage implements GlobalSyncStage { protected ClientDownloadDelegate clientDownloadDelegate; protected ClientsDatabaseAccessor db; - // Account/Profile info - protected boolean shouldWipe; + protected volatile boolean shouldWipe; + protected volatile boolean commandsProcessedShouldUpload; + protected final AtomicInteger uploadAttemptsCount = new AtomicInteger(); /** * The following two delegates, ClientDownloadDelegate and ClientUploadDelegate @@ -53,6 +61,9 @@ public class SyncClientsEngineStage implements GlobalSyncStage { */ public class ClientDownloadDelegate extends WBOCollectionRequestDelegate { + // We use this on each WBO, so lift it out. + final ClientsDataDelegate clientsDelegate = session.getClientsDelegate(); + @Override public String credentials() { return session.credentials(); @@ -67,15 +78,22 @@ public class SyncClientsEngineStage implements GlobalSyncStage { @Override public void handleRequestSuccess(SyncStorageResponse response) { BaseResource.consumeEntity(response); // We don't need the response at all. + final int clientsCount; try { - clientUploadDelegate = new ClientUploadDelegate(); - session.getClientsDelegate().setClientsCount(db.clientsCount()); - checkAndUpload(); + clientsCount = db.clientsCount(); } finally { // Close the database to clear cached readableDatabase/writableDatabase // after we've completed our last transaction (db.store()). db.close(); } + + Logger.debug(LOG_TAG, "Database contains " + clientsCount + " clients."); + Logger.debug(LOG_TAG, "Server response asserts " + response.weaveRecords() + " records."); + + // TODO: persist the response timestamp to know whether to download next time (Bug 726055). + clientUploadDelegate = new ClientUploadDelegate(); + clientsDelegate.setClientsCount(clientsCount); + checkAndUpload(); } @Override @@ -106,6 +124,15 @@ public class SyncClientsEngineStage implements GlobalSyncStage { ClientRecord r; try { r = (ClientRecord) factory.createRecord(record.decrypt()); + if (clientsDelegate.isLocalGUID(r.guid)) { + // Oh hey! Our record is on the server. This is the authoritative + // server timestamp, so let's hang on to it to decide whether we + // need to upload. + session.config.persistServerClientRecordTimestamp(r.lastModified); + + // Process commands. + processCommands(r.commands); + } RepoUtils.logClient(r); } catch (Exception e) { session.abort(e, "Exception handling client WBO."); @@ -135,21 +162,47 @@ public class SyncClientsEngineStage implements GlobalSyncStage { @Override public String ifUnmodifiedSince() { - // TODO last client upload time? - return null; + Long timestampInMilliseconds = session.config.getPersistedServerClientRecordTimestamp(); + + // It's the first upload so we don't care about X-If-Unmodified-Since. + if (timestampInMilliseconds == 0) { + return null; + } + + return Utils.millisecondsToDecimalSecondsString(timestampInMilliseconds); } @Override public void handleRequestSuccess(SyncStorageResponse response) { + Logger.debug(LOG_TAG, "Upload succeeded."); + commandsProcessedShouldUpload = false; + uploadAttemptsCount.set(0); + session.config.persistServerClientRecordTimestamp(response.normalizedWeaveTimestamp()); + BaseResource.consumeEntity(response); session.advance(); } @Override public void handleRequestFailure(SyncStorageResponse response) { - Logger.info(LOG_TAG, "Client upload failed. Aborting sync."); - BaseResource.consumeEntity(response); // The exception thrown should need the response body. - session.abort(new HTTPFailureException(response), "Client upload failed."); + int statusCode = response.getStatusCode(); + + // If upload failed because of `ifUnmodifiedSince` then there are new + // commands uploaded to our record. We must download and process them first. + if (!commandsProcessedShouldUpload || + statusCode == HttpStatus.SC_PRECONDITION_FAILED || + uploadAttemptsCount.incrementAndGet() > MAX_UPLOAD_FAILURE_COUNT) { + Logger.debug(LOG_TAG, "Client upload failed. Aborting sync."); + BaseResource.consumeEntity(response); // The exception thrown should need the response body. + session.abort(new HTTPFailureException(response), "Client upload failed."); + return; + } + Logger.trace(LOG_TAG, "Retrying upload…"); + // Preconditions: + // commandsProcessedShouldUpload == true && + // statusCode != 412 && + // uploadAttemptCount < MAX_UPLOAD_FAILURE_COUNT + checkAndUpload(); } @Override @@ -200,26 +253,49 @@ public class SyncClientsEngineStage implements GlobalSyncStage { return true; } - // TODO: Bug 729248 - Smarter upload of client records. protected boolean shouldUpload() { - return true; + if (commandsProcessedShouldUpload) { + return true; + } + + long lastUpload = session.config.getPersistedServerClientRecordTimestamp(); // Defaults to 0. + if (lastUpload == 0) { + return true; + } + + // Note the opportunity for clock drift problems here. + // TODO: if we track download times, we can use the timestamp of most + // recent download response instead of the current time. + long now = System.currentTimeMillis(); + long age = now - lastUpload; + return age >= CLIENTS_TTL_REFRESH; + } + + protected void processCommands(JSONArray commands) { + if (commands == null || + commands.size() == 0) { + return; + } + + commandsProcessedShouldUpload = true; + + // TODO: Bug 715792 - Process commands here. } protected void checkAndUpload() { if (!shouldUpload()) { - Logger.trace(LOG_TAG, "Not uploading client record."); + Logger.debug(LOG_TAG, "Not uploading client record."); session.advance(); return; } // Generate CryptoRecord from ClientRecord to upload. - String encryptionFailure = "Couldn't encrypt new client record."; - ClientRecord localClient = newLocalClientRecord(session.getClientsDelegate()); - CryptoRecord cryptoRecord = localClient.getEnvelope(); + final String encryptionFailure = "Couldn't encrypt new client record."; + final ClientRecord localClient = newLocalClientRecord(session.getClientsDelegate()); try { + CryptoRecord cryptoRecord = localClient.getEnvelope(); cryptoRecord.keyBundle = clientUploadDelegate.keyBundle(); cryptoRecord.encrypt(); - this.wipeAndStore(localClient); this.uploadClientRecord(cryptoRecord); } catch (UnsupportedEncodingException e) { session.abort(e, encryptionFailure + " Unsupported encoding."); @@ -246,6 +322,7 @@ public class SyncClientsEngineStage implements GlobalSyncStage { } protected void uploadClientRecord(CryptoRecord record) { + Logger.debug(LOG_TAG, "Uploading client record " + record.guid); try { URI putURI = session.config.wboURI(COLLECTION_NAME, record.guid); diff --git a/mobile/android/base/sync/syncadapter/SyncAdapter.java b/mobile/android/base/sync/syncadapter/SyncAdapter.java index 19e65dcfb0b..85c90ac7ab8 100644 --- a/mobile/android/base/sync/syncadapter/SyncAdapter.java +++ b/mobile/android/base/sync/syncadapter/SyncAdapter.java @@ -422,6 +422,11 @@ public class SyncAdapter extends AbstractThreadedSyncAdapter implements GlobalSe Integer.toString(clientsCount)); } + @Override + public boolean isLocalGUID(String guid) { + return getAccountGUID().equals(guid); + } + @Override public synchronized int getClientsCount() { String clientsCount = mAccountManager.getUserData(localAccount, Constants.NUM_CLIENTS);