Use synchronization rather than locking in WifiTracker. (ce6bab87) · Commits · e / os / android_frameworks_base

packages/SettingsLib/src/com/android/settingslib/wifi/WifiTracker.java

+150 −151

Original line number	Diff line number	Diff line
		@@ -15,6 +15,7 @@
		*/
		package com.android.settingslib.wifi;

		import android.annotation.MainThread;
		import android.content.BroadcastReceiver;
		import android.content.Context;
		import android.content.Intent;
		@@ -40,6 +41,7 @@ import android.os.Handler;
		import android.os.Looper;
		import android.os.Message;
		import android.provider.Settings;
		import android.support.annotation.GuardedBy;
		import android.util.ArraySet;
		import android.util.Log;
		import android.util.SparseArray;
		@@ -64,8 +66,6 @@ import java.util.concurrent.atomic.AtomicBoolean;
		* Tracks saved or available wifi networks and their state.
		*/
		public class WifiTracker {
		// TODO(sghuman): Document remaining methods with @UiThread and @WorkerThread where possible.
		// TODO(sghuman): Refactor to avoid calling certain methods on the UiThread.
		// TODO(b/36733768): Remove flag includeSaved and includePasspoints.

		private static final String TAG = "WifiTracker";
		@@ -96,34 +96,34 @@ public class WifiTracker {
		private WifiTrackerNetworkCallback mNetworkCallback;

		private int mNumSavedNetworks;

		@GuardedBy("mLock")
		private boolean mRegistered;

		/** Updated using main handler. Clone of this collection is returned
		* from {@link #getAccessPoints()}
		/**
		* The externally visible access point list.
		*
		* Updated using main handler. Clone of this collection is returned from
		* {@link #getAccessPoints()}
		*/
		private final List<AccessPoint> mAccessPoints = new ArrayList<>();

		/**
		* Protects APs contained in {@link #mInternalAccessPoints} from being modified concurrently
		* while its being read. Usage contract:
		* The internal list of access points, synchronized on itself.
		*
		* 1. MainHandler opens the condition after copying the states thereby
		* allowing WorkerHandler to mutate the contents.
		* 2. WorkerHandler after mutating the contents, sends a message to MainHandler to copy the
		* states and closes the condition.
		*
		* This is better than synchronizing on a variable because it prevents MainHandler from
		* unnecessarily blocking/waiting to acquire lock for copying states. When MainHandler is about
		* to access {@link #mInternalAccessPoints}, it is assumed that it has exclusively lock on the
		* contents.
		*/
		private final ConditionVariable mInternalAccessPointsWriteLock = new ConditionVariable(true);

		/** Guarded by mInternalAccessPointsWriteLock, updated using worker handler.
		* Never exposed outside this class.
		*/
		@GuardedBy("mLock")
		private final List<AccessPoint> mInternalAccessPoints = new ArrayList<>();

		/**
		* Synchronization lock for managing concurrency between main and worker threads.
		*
		* <p>This lock should be held for all background work.
		* TODO(b/37674366): Remove the worker thread so synchronization is no longer necessary.
		*/
		private final Object mLock = new Object();

		//visible to both worker and main thread. Guarded by #mInternalAccessPoints
		private final AccessPointListenerAdapter mAccessPointListenerAdapter
		= new AccessPointListenerAdapter();
		@@ -137,10 +137,12 @@ public class WifiTracker {

		private final NetworkScoreManager mNetworkScoreManager;
		private final WifiNetworkScoreCache mScoreCache;
		private final Set<NetworkKey> mRequestedScores = new ArraySet<>();
		private boolean mNetworkScoringUiEnabled;
		private final ContentObserver mObserver;

		@GuardedBy("mLock")
		private final Set<NetworkKey> mRequestedScores = new ArraySet<>();

		@VisibleForTesting
		Scanner mScanner;

		@@ -211,13 +213,17 @@ public class WifiTracker {

		mNetworkScoreManager = networkScoreManager;

		mScoreCache = new WifiNetworkScoreCache(context, new CacheListener(mMainHandler) {
		mScoreCache = new WifiNetworkScoreCache(context, new CacheListener(mWorkHandler) {
		@Override
		public void networkCacheUpdated(List<ScoredNetwork> networks) {
		synchronized (mLock) {
		if (!mRegistered) return;
		}

		if (Log.isLoggable(TAG, Log.VERBOSE)) {
		Log.v(TAG, "Score cache was updated with networks: " + networks);
		}
		Message.obtain(mWorkHandler, WorkHandler.MSG_UPDATE_NETWORK_SCORES).sendToTarget();
		updateNetworkScores();
		}
		});

		@@ -235,18 +241,20 @@ public class WifiTracker {
		/**
		* Synchronously update the list of access points with the latest information.
		*/
		@MainThread
		public void forceUpdate() {
		synchronized (mLock) {
		mWorkHandler.removeMessages(WorkHandler.MSG_UPDATE_ACCESS_POINTS);

		mLastInfo = mWifiManager.getConnectionInfo();
		mLastNetworkInfo = mConnectivityManager.getNetworkInfo(mWifiManager.getCurrentNetwork());
		updateAccessPoints();
		updateAccessPointsLocked();

		// Synchronously copy access points
		mMainHandler.removeMessages(MainHandler.MSG_ACCESS_POINT_CHANGED);
		mMainHandler.handleMessage(
		Message.obtain(mMainHandler, MainHandler.MSG_ACCESS_POINT_CHANGED));
		}
		}

		/**
		* Force a scan for wifi networks to happen now.
		@@ -289,14 +297,16 @@ public class WifiTracker {
		* <p>Registers listeners and starts scanning for wifi networks. If this is not called
		* then forceUpdate() must be called to populate getAccessPoints().
		*/
		@MainThread
		public void startTracking() {
		synchronized (mLock) {
		registerScoreCache();

		mContext.getContentResolver().registerContentObserver(
		Settings.Global.getUriFor(Settings.Global.NETWORK_SCORING_UI_ENABLED),
		false /* notifyForDescendants */,
		mObserver);
		mObserver.onChange(false /* selfChange */); // Set the initial value for mScoringUiEnabled
		mObserver.onChange(false /* selfChange */); // Set mScoringUiEnabled

		resumeScanning();
		if (!mRegistered) {
		@@ -307,6 +317,7 @@ public class WifiTracker {
		mRegistered = true;
		}
		}
		}

		private void registerScoreCache() {
		mNetworkScoreManager.registerNetworkScoreCache(
		@@ -322,44 +333,49 @@ public class WifiTracker {
		Log.d(TAG, "Requesting scores for Network Keys: " + keys);
		}
		mNetworkScoreManager.requestScores(keys.toArray(new NetworkKey[keys.size()]));
		synchronized (mLock) {
		mRequestedScores.addAll(keys);
		}
		}

		/**
		* Stop tracking wifi networks and scores.
		*
		* <p>Unregisters all listeners and stops scanning for wifi networks. This should always
		* be called when done with a WifiTracker (if startTracking was called) to ensure
		* proper cleanup.
		* <p>This should always be called when done with a WifiTracker (if startTracking was called) to
		* ensure proper cleanup and prevent any further callbacks from occuring.
		*/
		@MainThread
		public void stopTracking() {
		synchronized (mLock) {
		if (mRegistered) {
		mContext.unregisterReceiver(mReceiver);
		mConnectivityManager.unregisterNetworkCallback(mNetworkCallback);
		mRegistered = false;
		}
		mWorkHandler.removePendingMessages();
		mMainHandler.removePendingMessages();

		pauseScanning();

		unregisterAndClearScoreCache();

		pauseScanning();
		mContext.getContentResolver().unregisterContentObserver(mObserver);

		mWorkHandler.removePendingMessages();
		mMainHandler.removePendingMessages();
		}
		}

		private void unregisterAndClearScoreCache() {
		mNetworkScoreManager.unregisterNetworkScoreCache(NetworkKey.TYPE_WIFI, mScoreCache);
		mScoreCache.clearScores();

		// Clear the scores on the work handler to avoid concurrent modification exceptions
		mWorkHandler.post(() -> mRequestedScores.clear());
		// Synchronize on mLock to avoid concurrent modification during updateAccessPointsLocked
		synchronized (mLock) {
		mRequestedScores.clear();
		}
		}

		/**
		* Gets the current list of access points. Should be called from main thread, otherwise
		* expect inconsistencies
		*/
		@MainThread
		public List<AccessPoint> getAccessPoints() {
		return new ArrayList<>(mAccessPoints);
		}
		@@ -440,16 +456,20 @@ public class WifiTracker {
		return null;
		}

		private void updateAccessPoints() {
		// Wait until main worker is done copying the states. This is done to prevent propagation
		// of accesspoint states while the update is in progress.
		long before = System.currentTimeMillis();
		mInternalAccessPointsWriteLock.block();
		if (DBG) {
		Log.d(TAG, "Acquired AP lock on WorkerHandler. Time to wait = "
		+ (System.currentTimeMillis() - before) + " ms.");
		/** Safely modify {@link #mInternalAccessPoints} by acquiring {@link #mLock} first. */
		private void updateAccessPointsLocked() {
		synchronized (mLock) {
		updateAccessPoints();
		}
		}

		/**
		* Update the internal list of access points.
		*
		* <p>Should never be called directly, use {@link #updateAccessPointsLocked()} instead.
		*/
		@GuardedBy("mLock")
		private void updateAccessPoints() {
		// Swap the current access points into a cached list.
		List<AccessPoint> cachedAccessPoints = new ArrayList<>(mInternalAccessPoints);
		ArrayList<AccessPoint> accessPoints = new ArrayList<>();
		@@ -583,7 +603,8 @@ public class WifiTracker {

		mInternalAccessPoints.clear();
		mInternalAccessPoints.addAll(accessPoints);
		mMainHandler.scheduleAPCopyingAndCloseWriteLock();

		mMainHandler.sendEmptyMessage(MainHandler.MSG_ACCESS_POINT_CHANGED);
		}

		@VisibleForTesting
		@@ -640,17 +661,10 @@ public class WifiTracker {
		connectionConfig = getWifiConfigurationForNetworkId(mLastInfo.getNetworkId());
		}

		// Lock required to prevent accidental copying of AccessPoint states while the modification
		// is in progress. see #copyAndNotifyListeners
		long before = System.currentTimeMillis();
		mInternalAccessPointsWriteLock.block();
		if (DBG) {
		Log.d(TAG, "Acquired AP lock on WorkerHandler for updating NetworkInfo. Wait time = " +
		(System.currentTimeMillis() - before) + "ms.");
		}

		boolean updated = false;
		boolean reorder = false; // Only reorder if connected AP was changed

		synchronized (mLock) {
		for (int i = mInternalAccessPoints.size() - 1; i >= 0; --i) {
		AccessPoint ap = mInternalAccessPoints.get(i);
		boolean previouslyConnected = ap.isActive();
		@@ -666,33 +680,29 @@ public class WifiTracker {

		if (reorder) Collections.sort(mInternalAccessPoints);

		if (updated) mMainHandler.scheduleAPCopyingAndCloseWriteLock();
		if (updated) mMainHandler.sendEmptyMessage(MainHandler.MSG_ACCESS_POINT_CHANGED);
		}
		}

		/**
		* Update all the internal access points rankingScores, badge and metering.
		*
		* <p>Will trigger a resort and notify listeners of changes if applicable.
		*
		* <p>Synchronized on {@link #mLock}.
		*/
		private void updateNetworkScores() {
		// Lock required to prevent accidental copying of AccessPoint states while the modification
		// is in progress. see #copyAndNotifyListeners
		long before = System.currentTimeMillis();
		mInternalAccessPointsWriteLock.block();
		if (DBG) {
		Log.d(TAG, "Acquired AP lock on WorkerHandler for inserting NetworkScores. Wait time = " +
		(System.currentTimeMillis() - before) + "ms.");
		}

		boolean reorder = false;
		synchronized (mLock) {
		boolean updated = false;
		for (int i = 0; i < mInternalAccessPoints.size(); i++) {
		if (mInternalAccessPoints.get(i).update(mScoreCache, mNetworkScoringUiEnabled)) {
		reorder = true;
		updated = true;
		}
		}
		if (reorder) {
		if (updated) {
		Collections.sort(mInternalAccessPoints);
		mMainHandler.scheduleAPCopyingAndCloseWriteLock();
		mMainHandler.sendEmptyMessage(MainHandler.MSG_ACCESS_POINT_CHANGED);
		}
		}
		}

		@@ -732,7 +742,10 @@ public class WifiTracker {
		.sendToTarget();
		mWorkHandler.sendEmptyMessage(WorkHandler.MSG_UPDATE_ACCESS_POINTS);
		} else if (WifiManager.RSSI_CHANGED_ACTION.equals(action)) {
		mWorkHandler.sendEmptyMessage(WorkHandler.MSG_UPDATE_NETWORK_INFO);
		NetworkInfo info =
		mConnectivityManager.getNetworkInfo(mWifiManager.getCurrentNetwork());
		mWorkHandler.obtainMessage(WorkHandler.MSG_UPDATE_NETWORK_INFO, info)
		.sendToTarget();
		}
		}
		};
		@@ -789,19 +802,8 @@ public class WifiTracker {
		}
		}

		void scheduleAPCopyingAndCloseWriteLock() {
		//prevent worker thread from modifying mInternalAccessPoints
		mInternalAccessPointsWriteLock.close();
		sendEmptyMessage(MSG_ACCESS_POINT_CHANGED);
		}

		void removePendingMessages() {
		// Only release the lock if there was a pending message which would have done the same
		if (mMainHandler.hasMessages(MSG_ACCESS_POINT_CHANGED)) {
		mMainHandler.removeMessages(MSG_ACCESS_POINT_CHANGED);
		mInternalAccessPointsWriteLock.open();
		}

		removeMessages(MSG_ACCESS_POINT_CHANGED);
		removeMessages(MSG_CONNECTED_CHANGED);
		removeMessages(MSG_WIFI_STATE_CHANGED);
		removeMessages(MSG_PAUSE_SCANNING);
		@@ -814,7 +816,6 @@ public class WifiTracker {
		private static final int MSG_UPDATE_NETWORK_INFO = 1;
		private static final int MSG_RESUME = 2;
		private static final int MSG_UPDATE_WIFI_STATE = 3;
		private static final int MSG_UPDATE_NETWORK_SCORES = 4;

		public WorkHandler(Looper looper) {
		super(looper);
		@@ -822,9 +823,18 @@ public class WifiTracker {

		@Override
		public void handleMessage(Message msg) {
		synchronized (mLock) {
		processMessage(msg);
		}
		}

		@GuardedBy("mLock")
		private void processMessage(Message msg) {
		if (!mRegistered) return;

		switch (msg.what) {
		case MSG_UPDATE_ACCESS_POINTS:
		updateAccessPoints();
		updateAccessPointsLocked();
		break;
		case MSG_UPDATE_NETWORK_INFO:
		updateNetworkInfo((NetworkInfo) msg.obj);
		@@ -849,9 +859,6 @@ public class WifiTracker {
		mMainHandler.obtainMessage(MainHandler.MSG_WIFI_STATE_CHANGED, msg.arg1, 0)
		.sendToTarget();
		break;
		case MSG_UPDATE_NETWORK_SCORES:
		updateNetworkScores();
		break;
		}
		}

		@@ -860,7 +867,6 @@ public class WifiTracker {
		removeMessages(MSG_UPDATE_NETWORK_INFO);
		removeMessages(MSG_RESUME);
		removeMessages(MSG_UPDATE_WIFI_STATE);
		removeMessages(MSG_UPDATE_NETWORK_SCORES);
		}
		}

		@@ -980,11 +986,12 @@ public class WifiTracker {

		/**
		* Responsible for copying access points from {@link #mInternalAccessPoints} and notifying
		* accesspoint listeners. Mutation of the accesspoints returned is done on the main thread.
		* accesspoint listeners.
		*
		* @param notifyListeners if true, accesspoint listeners are notified, otherwise notifications
		* dropped.
		*/
		@MainThread
		private void copyAndNotifyListeners(boolean notifyListeners) {
		// Need to watch out for memory allocations on main thread.
		SparseArray<AccessPoint> oldAccessPoints = new SparseArray<>();
		@@ -995,9 +1002,6 @@ public class WifiTracker {
		oldAccessPoints.put(accessPoint.mId, accessPoint);
		}

		//synchronize to prevent modification of "mInternalAccessPoints" by worker thread.
		long before = System.currentTimeMillis();
		try {
		if (DBG) {
		Log.d(TAG, "Starting to copy AP items on the MainHandler");
		}
		@@ -1006,6 +1010,7 @@ public class WifiTracker {
		}

		mAccessPointListenerAdapter.mPendingNotifications.clear();
		synchronized (mLock) {
		for (AccessPoint internalAccessPoint : mInternalAccessPoints) {
		AccessPoint accessPoint = oldAccessPoints.get(internalAccessPoint.mId);
		if (accessPoint == null) {
		@@ -1015,12 +1020,6 @@ public class WifiTracker {
		}
		updatedAccessPoints.add(accessPoint);
		}
		} finally {
		mInternalAccessPointsWriteLock.open();
		if (DBG) {
		Log.d(TAG, "Opened AP Write lock on the MainHandler. Time to copy = " +
		(System.currentTimeMillis() - before) + " ms.");
		}
		}

		mAccessPoints.clear();

packages/SettingsLib/src/com/android/settingslib/wifi/WifiTrackerFactory.java

+1 −3

Original line number	Diff line number	Diff line
		@@ -23,8 +23,6 @@ import android.support.annotation.Keep;
		* Factory method used to inject WifiTracker instances.
		*/
		public class WifiTrackerFactory {
		private static boolean sTestingMode = false;

		private static WifiTracker sTestingWifiTracker;

		@Keep // Keep proguard from stripping this method since it is only used in tests
		@@ -35,7 +33,7 @@ public class WifiTrackerFactory {
		public static WifiTracker create(
		Context context, WifiTracker.WifiListener wifiListener, Looper workerLooper,
		boolean includeSaved, boolean includeScans, boolean includePasspoints) {
		if(sTestingMode) {
		if(sTestingWifiTracker != null) {
		return sTestingWifiTracker;
		}
		return new WifiTracker(

packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/WifiTrackerTest.java

+12 −13

Original line number	Diff line number	Diff line
		@@ -27,6 +27,8 @@ import static org.mockito.Mockito.atMost;
		import static org.mockito.Mockito.doAnswer;
		import static org.mockito.Mockito.doNothing;
		import static org.mockito.Mockito.doThrow;
		import static org.mockito.Mockito.mock;
		import static org.mockito.Mockito.times;
		import static org.mockito.Mockito.verify;
		import static org.mockito.Mockito.verifyNoMoreInteractions;
		import static org.mockito.Mockito.when;
		@@ -253,9 +255,7 @@ public class WifiTrackerTest {
		tracker.mReceiver.onReceive(mContext, intent);
		}

		mAccessPointsChangedLatch = new CountDownLatch(1);
		sendScanResultsAndProcess(tracker);
		assertTrue(mAccessPointsChangedLatch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS));

		return tracker;
		}
		@@ -465,9 +465,10 @@ public class WifiTrackerTest {
		private void updateScoresAndWaitForAccessPointsChangedCallback() throws InterruptedException {
		// Updating scores can happen together or one after the other, so the latch countdown is set
		// to 2.
		mAccessPointsChangedLatch = new CountDownLatch(2);
		mAccessPointsChangedLatch = new CountDownLatch(3);
		updateScores();
		mAccessPointsChangedLatch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS);
		assertTrue("onAccessPointChanged was not called three times",
		mAccessPointsChangedLatch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS));
		}

		@Test
		@@ -651,12 +652,6 @@ public class WifiTrackerTest {
		WifiTracker tracker = createTrackerWithScanResultsAndAccessPoint1Connected();
		assertThat(tracker.getAccessPoints().get(0).isActive()).isTrue();

		WifiConfiguration configuration = new WifiConfiguration();
		configuration.SSID = SSID_1;
		configuration.BSSID = BSSID_1;
		configuration.networkId = CONNECTED_NETWORK_ID;
		when(mockWifiManager.getConfiguredNetworks()).thenReturn(Arrays.asList(configuration));

		int newRssi = CONNECTED_RSSI + 10;
		WifiInfo info = new WifiInfo(CONNECTED_AP_1_INFO);
		info.setRssi(newRssi);
		@@ -681,7 +676,8 @@ public class WifiTrackerTest {

		@Test
		public void forceUpdateShouldSynchronouslyFetchLatestInformation() throws Exception {
		// TODO(sghuman): Fix flakiness of this test
		Network mockNetwork = mock(Network.class);
		when(mockWifiManager.getCurrentNetwork()).thenReturn(mockNetwork);

		when(mockWifiManager.getConnectionInfo()).thenReturn(CONNECTED_AP_1_INFO);

		@@ -697,9 +693,12 @@ public class WifiTrackerTest {
		when(mockConnectivityManager.getNetworkInfo(any(Network.class))).thenReturn(networkInfo);

		WifiTracker tracker = createMockedWifiTracker();
		startTracking(tracker);
		tracker.forceUpdate();

		verify(mockWifiManager).getConnectionInfo();
		verify(mockWifiManager, times(2)).getConfiguredNetworks();
		verify(mockConnectivityManager).getNetworkInfo(any(Network.class));

		verify(mockWifiListener).onAccessPointsChanged();
		assertThat(tracker.getAccessPoints().size()).isEqualTo(2);
		assertThat(tracker.getAccessPoints().get(0).isActive()).isTrue();
		@@ -736,7 +735,7 @@ public class WifiTrackerTest {
		verify(mockWifiListener, atMost(1)).onWifiStateChanged(anyInt());

		lock.countDown();
		assertTrue(latch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS));
		assertTrue("Latch timed out", latch.await(LATCH_TIMEOUT, TimeUnit.MILLISECONDS));

		assertThat(tracker.mMainHandler.hasMessages(
		WifiTracker.MainHandler.MSG_ACCESS_POINT_CHANGED)).isFalse();