Do not synchronize boolean reads/writes

This patch removes the synchronization around the private variable
mRunning inside of IpReachabilityMonitor and instead qualifeis the field
as volatile.

Synchronization is not needed for reads/writes on native fields or
object references because they are already guaranteed to be atomic.

Synchronization here was used for enforcing memory visibility across
concurrent threads indirectly through monitor acquire/release.
The volatile keyword achieves this in a more explicit way.

Also, this patch changes the way that probeAll() copies the
IpReachabilityMonitor's mIpWatchList by temporary holding mIpWatchList
keys into an ArrayList instead of a more expensive HashSet. Since Java
HashSet are just degenerated HashMaps, and that key iteration order is
based on key hash, the iteration order over this temporary collection
will be consistent for the same mIpWatchList.

Test: refactoring CL. Existing unit tests still pass.

(cherry picked from commit b0f1186c034c4df9eb54ed29944d16ce6d7ade56)

Change-Id: I48d2b4d837a459150cd431b400ec01b87b48c014

diff --git a/services/net/java/android/net/ip/IpReachabilityMonitor.java b/services/net/java/android/net/ip/IpReachabilityMonitor.java
index a6bb40c..a883e28 100644 (file)
--- a/services/net/java/android/net/ip/IpReachabilityMonitor.java
+++ b/services/net/java/android/net/ip/IpReachabilityMonitor.java
@@ -50,9 +50,9 @@ import java.net.NetworkInterface;
 import java.net.SocketAddress;
 import java.net.SocketException;
 import java.nio.ByteBuffer;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -163,8 +163,7 @@ public class IpReachabilityMonitor {
     private Map<InetAddress, Short> mIpWatchList = new HashMap<>();
     @GuardedBy("mLock")
     private int mIpWatchListVersion;
-    @GuardedBy("mLock")
-    private boolean mRunning;
+    private volatile boolean mRunning;
     // Time in milliseconds of the last forced probe request.
     private volatile long mLastProbeTimeMs;
 
@@ -246,7 +245,7 @@ public class IpReachabilityMonitor {
     }
 
     public void stop() {
-        synchronized (mLock) { mRunning = false; }
+        mRunning = false;
         clearLinkProperties();
         mNetlinkSocketObserver.clearNetlinkSocket();
     }
@@ -281,12 +280,6 @@ public class IpReachabilityMonitor {
         }
     }
 
-    private boolean stillRunning() {
-        synchronized (mLock) {
-            return mRunning;
-        }
-    }
-
     private static boolean isOnLink(List<RouteInfo> routes, InetAddress ip) {
         for (RouteInfo route : routes) {
             if (!route.hasGateway() && route.matches(ip)) {
@@ -390,12 +383,12 @@ public class IpReachabilityMonitor {
     }
 
     public void probeAll() {
-        Set<InetAddress> ipProbeList = new HashSet<InetAddress>();
+        final List<InetAddress> ipProbeList;
         synchronized (mLock) {
-            ipProbeList.addAll(mIpWatchList.keySet());
+            ipProbeList = new ArrayList<>(mIpWatchList.keySet());
         }
 
-        if (!ipProbeList.isEmpty() && stillRunning()) {
+        if (!ipProbeList.isEmpty() && mRunning) {
             // Keep the CPU awake long enough to allow all ARP/ND
             // probes a reasonable chance at success. See b/23197666.
             //
@@ -406,7 +399,7 @@ public class IpReachabilityMonitor {
         }
 
         for (InetAddress target : ipProbeList) {
-            if (!stillRunning()) {
+            if (!mRunning) {
                 break;
             }
             final int returnValue = probeNeighbor(mInterfaceIndex, target);
@@ -451,21 +444,21 @@ public class IpReachabilityMonitor {
         @Override
         public void run() {
             if (VDBG) { Log.d(TAG, "Starting observing thread."); }
-            synchronized (mLock) { mRunning = true; }
+            mRunning = true;
 
             try {
                 setupNetlinkSocket();
             } catch (ErrnoException | SocketException e) {
                 Log.e(TAG, "Failed to suitably initialize a netlink socket", e);
-                synchronized (mLock) { mRunning = false; }
+                mRunning = false;
             }
 
-            ByteBuffer byteBuffer;
-            while (stillRunning()) {
+            while (mRunning) {
+                final ByteBuffer byteBuffer;
                 try {
                     byteBuffer = recvKernelReply();
                 } catch (ErrnoException e) {
-                    if (stillRunning()) { Log.w(TAG, "ErrnoException: ", e); }
+                    if (mRunning) { Log.w(TAG, "ErrnoException: ", e); }
                     break;
                 }
                 final long whenMs = SystemClock.elapsedRealtime();
@@ -477,7 +470,7 @@ public class IpReachabilityMonitor {
 
             clearNetlinkSocket();
 
-            synchronized (mLock) { mRunning = false; }
+            mRunning = false; // Not a no-op when ErrnoException happened.
             if (VDBG) { Log.d(TAG, "Finishing observing thread."); }
         }