Make NetworkMonitor less aggressive on broken networks.

1. Retry validation 3 times instead of 10.

Reducing the number of retries speeds up dead network detection
and saves battery on networks that are don't have Internet access
(or block our connectivity checks).

Retrying 10 times is overkill. Each attempt sends 5-10 DNS
requests, waits 10 seconds to transfer only 3 or 4 packets, and
can last up to tens of seconds. Also, in MR1 we now retry every
10 minutes.

2. Make periodic revalidation only try once.

When validating periodically to see if a non-working connection
is working again, only make one attempt every 10 minutes, not
three.

3. Rename "retries" to "attempts" to make things a bit clearer.

Bug: 18922569
Bug: 18869302
Change-Id: I5f70b420c4f1bac0c33bb9ab6afb322902cdac4b

1 files changed, 30 insertions, 13 deletions

diff --git a/services/core/java/com/android/server/connectivity/NetworkMonitor.java b/services/core/java/com/android/server/connectivity/NetworkMonitor.java
index af5ed83..f900d0d 100644
--- a/services/core/java/com/android/server/connectivity/NetworkMonitor.java
+++ b/services/core/java/com/android/server/connectivity/NetworkMonitor.java
@@ -163,6 +163,7 @@ public class NetworkMonitor extends StateMachine {
     /**
      * Force evaluation even if it has succeeded in the past.
      * arg1 = UID responsible for requesting this reeval.  Will be billed for data.
+     * arg2 = Number of evaluation attempts to make. (If 0, make INITIAL_ATTEMPTS attempts.)
      */
     public static final int CMD_FORCE_REEVALUATION = BASE + 8;
 
@@ -212,11 +213,14 @@ public class NetworkMonitor extends StateMachine {
 
     // Negative values disable reevaluation.
     private static final String REEVALUATE_DELAY_PROPERTY = "persist.netmon.reeval_delay";
-    // Default to 5s reevaluation delay.
+    // When connecting, attempt to validate 3 times, pausing 5s between them.
     private static final int DEFAULT_REEVALUATE_DELAY_MS = 5000;
-    private static final int MAX_RETRIES = 10;
-    // Between groups of MAX_RETRIES evaluation attempts, pause 10 mins in hopes ISP outage passes.
+    private static final int INITIAL_ATTEMPTS = 3;
+    // If a network is not validated, make one attempt every 10 mins to see if it starts working.
     private static final int REEVALUATE_PAUSE_MS = 10*60*1000;
+    private static final int PERIODIC_ATTEMPTS = 1;
+    // When an application calls reportBadNetwork, only make one attempt.
+    private static final int REEVALUATE_ATTEMPTS = 1;
     private final int mReevaluateDelayMs;
     private int mReevaluateToken = 0;
     private static final int INVALID_UID = -1;
@@ -236,6 +240,14 @@ public class NetworkMonitor extends StateMachine {
     // Set if the user explicitly selected "Do not use this network" in captive portal sign-in app.
     private boolean mUserDoesNotWant = false;
 
+    // How many times we should attempt validation. Only checked in EvaluatingState; must be set
+    // before entering EvaluatingState. Note that whatever code causes us to transition to
+    // EvaluatingState last decides how many attempts will be made, so if one codepath were to
+    // enter EvaluatingState with a specific number of attempts, and then another were to enter it
+    // with a different number of attempts, the second number would be used. This is not currently
+    // a problem because EvaluatingState is not reentrant.
+    private int mMaxAttempts;
+
     public boolean systemReady = false;
 
     private final State mDefaultState = new DefaultState();
@@ -305,6 +317,7 @@ public class NetworkMonitor extends StateMachine {
                     return HANDLED;
                 case CMD_NETWORK_CONNECTED:
                     if (DBG) log("Connected");
+                    mMaxAttempts = INITIAL_ATTEMPTS;
                     transitionTo(mEvaluatingState);
                     return HANDLED;
                 case CMD_NETWORK_DISCONNECTED:
@@ -318,6 +331,7 @@ public class NetworkMonitor extends StateMachine {
                 case CMD_FORCE_REEVALUATION:
                     if (DBG) log("Forcing reevaluation");
                     mUidResponsibleForReeval = message.arg1;
+                    mMaxAttempts = message.arg2 != 0 ? message.arg2 : REEVALUATE_ATTEMPTS;
                     transitionTo(mEvaluatingState);
                     return HANDLED;
                 case CMD_CAPTIVE_PORTAL_APP_FINISHED:
@@ -347,7 +361,10 @@ public class NetworkMonitor extends StateMachine {
         public void enter() {
             mConnectivityServiceHandler.sendMessage(obtainMessage(EVENT_NETWORK_TESTED,
                     NETWORK_TEST_RESULT_INVALID, 0, mNetworkAgentInfo));
-            if (!mUserDoesNotWant) sendMessageDelayed(CMD_FORCE_REEVALUATION, REEVALUATE_PAUSE_MS);
+            if (!mUserDoesNotWant) {
+                sendMessageDelayed(CMD_FORCE_REEVALUATION, 0 /* no UID */,
+                        PERIODIC_ATTEMPTS, REEVALUATE_PAUSE_MS);
+            }
         }
 
         @Override
@@ -413,11 +430,11 @@ public class NetworkMonitor extends StateMachine {
     // Being in the EvaluatingState State indicates the Network is being evaluated for internet
     // connectivity.
     private class EvaluatingState extends State {
-        private int mRetries;
+        private int mAttempt;
 
         @Override
         public void enter() {
-            mRetries = 0;
+            mAttempt = 1;
             sendMessage(CMD_REEVALUATE, ++mReevaluateToken, 0);
             if (mUidResponsibleForReeval != INVALID_UID) {
                 TrafficStats.setThreadStatsUid(mUidResponsibleForReeval);
@@ -454,18 +471,18 @@ public class NetworkMonitor extends StateMachine {
                         transitionTo(mValidatedState);
                         return HANDLED;
                     }
-                    // Note: This call to isCaptivePortal() could take minutes.  Resolving the
-                    // server's IP addresses could hit the DNS timeout and attempting connections
-                    // to each of the server's several (e.g. 11) IP addresses could each take
-                    // SOCKET_TIMEOUT_MS.  During this time this StateMachine will be unresponsive.
-                    // isCaptivePortal() could be executed on another Thread if this is found to
-                    // cause problems.
+                    // Note: This call to isCaptivePortal() could take up to a minute. Resolving the
+                    // server's IP addresses could hit the DNS timeout, and attempting connections
+                    // to each of the server's several IP addresses (currently one IPv4 and one
+                    // IPv6) could each take SOCKET_TIMEOUT_MS.  During this time this StateMachine
+                    // will be unresponsive. isCaptivePortal() could be executed on another Thread
+                    // if this is found to cause problems.
                     int httpResponseCode = isCaptivePortal();
                     if (httpResponseCode == 204) {
                         transitionTo(mValidatedState);
                     } else if (httpResponseCode >= 200 && httpResponseCode <= 399) {
                         transitionTo(mCaptivePortalState);
-                    } else if (++mRetries > MAX_RETRIES) {
+                    } else if (++mAttempt > mMaxAttempts) {
                         transitionTo(mOfflineState);
                     } else if (mReevaluateDelayMs >= 0) {
                         Message msg = obtainMessage(CMD_REEVALUATE, ++mReevaluateToken, 0);