Bug 1243585 - Stop IntentService if there is a connection failure. r=sebastian draft
authorMichael Comella <michael.l.comella@gmail.com>
Fri, 29 Apr 2016 15:18:07 -0700
changeset 357996 8663d541504f8577b516e5aee58c22fe83410160
parent 357995 4396470ee7aa7e06ff3a7dec7117e654b8164b41
child 357997 5ea90ce2d56f7afe4170949faa303ab56dc6f13d
child 357998 d0e36a04e97b009f743801018b7e5f18a7ab3c93
push id16902
push usermichael.l.comella@gmail.com
push dateFri, 29 Apr 2016 22:32:18 +0000
reviewerssebastian
bugs1243585
milestone49.0a1
Bug 1243585 - Stop IntentService if there is a connection failure. r=sebastian I would have folded this but I screwed up but the version control is getting messy. MozReview-Commit-ID: 3LaYkrao4dD
mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryUploadService.java
--- a/mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryUploadService.java
+++ b/mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryUploadService.java
@@ -57,22 +57,33 @@ public class TelemetryUploadService exte
     public void onHandleIntent(final Intent intent) {
         Log.d(LOGTAG, "Service started");
 
         if (!isReadyToUpload(this, intent)) {
             return;
         }
 
         final TelemetryPingStore store = intent.getParcelableExtra(EXTRA_STORE);
-        uploadPendingPingsFromStore(this, store);
+        final boolean wereAllUploadsSuccessful = uploadPendingPingsFromStore(this, store);
         store.maybePrunePings();
         Log.d(LOGTAG, "Service finished: upload and prune attempts completed");
+
+        if (!wereAllUploadsSuccessful) {
+            // If we had an upload failure, we should stop the IntentService and drop any
+            // pending Intents in the queue so we don't waste resources (e.g. battery)
+            // trying to upload when there's likely to be another connection failure.
+            Log.d(LOGTAG, "Clearing Intent queue due to connection failures");
+            stopSelf();
+        }
     }
 
-    private static void uploadPendingPingsFromStore(final Context context, final TelemetryPingStore store) {
+    /**
+     * @return true if all pings were uploaded successfully, false otherwise.
+     */
+    private static boolean uploadPendingPingsFromStore(final Context context, final TelemetryPingStore store) {
         final ArrayList<TelemetryPingFromStore> pingsToUpload = store.getAllPings();
         if (pingsToUpload.isEmpty()) {
             return true;
         }
 
         final String serverSchemeHostPort = getServerSchemeHostPort(context);
         final HashSet<Integer> successfulUploadIDs = new HashSet<>(pingsToUpload.size()); // used for side effects.
         final PingResultDelegate delegate = new PingResultDelegate(successfulUploadIDs);
@@ -83,21 +94,23 @@ public class TelemetryUploadService exte
             }
 
             // TODO: It'd be great to re-use the same HTTP connection for each upload request.
             delegate.setPingID(ping.getUniqueID());
             final String url = serverSchemeHostPort + "/" + ping.getURLPath();
             uploadPayload(url, ping.getPayload(), delegate);
         }
 
-        if (!delegate.hadConnectionError()) {
+        final boolean wereAllUploadsSuccessful = !delegate.hadConnectionError();
+        if (wereAllUploadsSuccessful) {
             // We don't log individual successful uploads to avoid log spam.
             Log.d(LOGTAG, "Telemetry upload success!");
         }
         store.onUploadAttemptComplete(successfulUploadIDs);
+        return wereAllUploadsSuccessful;
     }
 
     private static String getServerSchemeHostPort(final Context context) {
         // TODO (bug 1241685): Sync this preference with the gecko preference or a Java-based pref.
         // We previously had this synced with profile prefs with no way to change it in the client, so
         // we don't have to worry about losing data by switching it to app prefs, which is more convenient for now.
         final SharedPreferences sharedPrefs = GeckoSharedPrefs.forApp(context);
         return sharedPrefs.getString(TelemetryConstants.PREF_SERVER_URL, TelemetryConstants.DEFAULT_SERVER_URL);
@@ -202,26 +215,20 @@ public class TelemetryUploadService exte
     /**
      * Logs on success & failure and appends the set ID to the given ArrayList on success.
      *
      * Note: you *must* set the ping ID before attempting upload or we'll throw!
      *
      * We use mutation on the set ID and the successful upload array to avoid object allocation.
      */
     private static class PingResultDelegate extends ResultDelegate {
-        // Since an IntentService handles one Intent at a time and queues subsequent Intents,
-        // an upload attempt can block the next upload attempt. This is problematic in the case
-        // that we don't have a connection – e.g. if we block for 2 minutes on each upload attempt,
-        // then each Intent will keep the radio active for two minutes and our users will have a
-        // bad day. Therefore, keep these durations short - we will try to upload these pings later
-        // and will generally not lose data.
-        //
-        // We can prevent this issue with better upload scheduling (bug 1268730).
-        private static final int SOCKET_TIMEOUT_MILLIS = (int) TimeUnit.SECONDS.toMillis(20);
-        private static final int CONNECTION_TIMEOUT_MILLIS = (int) TimeUnit.SECONDS.toMillis(15);
+        // We persist pings and don't need to worry about losing data so we keep these
+        // durations short to save resources (e.g. battery).
+        private static final int SOCKET_TIMEOUT_MILLIS = (int) TimeUnit.SECONDS.toMillis(30);
+        private static final int CONNECTION_TIMEOUT_MILLIS = (int) TimeUnit.SECONDS.toMillis(30);
 
         /** The store ID of the ping currently being uploaded. Use {@link #getPingID()} to access it. */
         private int pingID = -1;
         private final HashSet<Integer> successfulUploadIDs;
 
         private boolean hadConnectionError = false;
 
         public PingResultDelegate(final HashSet<Integer> successfulUploadIDs) {