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