BatteryService: Clean up init logic.

* Use getService instead of getTransport, because
  getService checks VINTF already. Init has fewer #
  of hwbinder calls and simpler logic.

* init() calls registerCallback() to HAL synchronously. Now that
  there is a way to check for equality of interfaces in Java,
  registerCallback can be called in HealthServiceWrapper.init()
  earlier, and registerCallback in service notification can
  be avoided when the service pre-exists.

* Instead of making hwbinder calls in a lock in hwbinder threads
  (service notification callback), post to a background HandlerThread.
  As a consequence, no lock is needed because ordering is ensured.
  (Making hwbinder calls in a lock can lead to deadlocks if an
  implementation calls back to system server and tries to acquire the
  same lock.)

Test: boot 20 times
Test: BatteryServiceTest

Change-Id: Id27b789da78f0df9f867cba75d15203a4fb74e04

2 files changed, 96 insertions, 43 deletions

diff --git a/services/core/java/com/android/server/BatteryService.java b/services/core/java/com/android/server/BatteryService.java
index 1b61866f81b6..7a60a5d1e11d 100644
--- a/services/core/java/com/android/server/BatteryService.java
+++ b/services/core/java/com/android/server/BatteryService.java
@@ -49,6 +49,7 @@ import android.os.BatteryProperty;
 import android.os.Binder;
 import android.os.FileUtils;
 import android.os.Handler;
+import android.os.HandlerThread;
 import android.os.IBatteryPropertiesListener;
 import android.os.IBatteryPropertiesRegistrar;
 import android.os.IBinder;
@@ -75,6 +76,7 @@ import java.io.PrintWriter;
 import java.util.Arrays;
 import java.util.List;
 import java.util.NoSuchElementException;
+import java.util.Objects;
 import java.util.concurrent.atomic.AtomicReference;
 
 /**
@@ -176,6 +178,7 @@ public final class BatteryService extends SystemService {
     private HealthServiceWrapper mHealthServiceWrapper;
     private HealthHalCallback mHealthHalCallback;
     private BatteryPropertiesRegistrar mBatteryPropertiesRegistrar;
+    private HandlerThread mHandlerThread;
 
     public BatteryService(Context context) {
         super(context);
@@ -1195,14 +1198,13 @@ public final class BatteryService extends SystemService {
                 Arrays.asList(INSTANCE_VENDOR, INSTANCE_HEALTHD);
 
         private final IServiceNotification mNotification = new Notification();
+        private final HandlerThread mHandlerThread = new HandlerThread("HealthServiceRefresh");
         // These variables are fixed after init.
         private Callback mCallback;
         private IHealthSupplier mHealthSupplier;
         private String mInstanceName;
 
-        private final Object mLastServiceSetLock = new Object();
         // Last IHealth service received.
-        // set must be also be guarded with mLastServiceSetLock to ensure ordering.
         private final AtomicReference<IHealth> mLastService = new AtomicReference<>();
 
         /**
@@ -1220,6 +1222,10 @@ public final class BatteryService extends SystemService {
          * Start monitoring registration of new IHealth services. Only instances that are in
          * {@code sAllInstances} and in device / framework manifest are used. This function should
          * only be called once.
+         *
+         * mCallback.onRegistration() is called synchronously (aka in init thread) before
+         * this method returns.
+         *
          * @throws RemoteException transaction error when talking to IServiceManager
          * @throws NoSuchElementException if one of the following cases:
          *         - No service manager;
@@ -1240,40 +1246,48 @@ public final class BatteryService extends SystemService {
             mCallback = callback;
             mHealthSupplier = healthSupplier;
 
-            traceBegin("HealthInitGetManager");
-            try {
-                manager = managerSupplier.get();
-            } finally {
-                traceEnd();
-            }
+            // Initialize mLastService and call callback for the first time (in init thread)
+            IHealth newService = null;
             for (String name : sAllInstances) {
-                traceBegin("HealthInitGetTransport_" + name);
+                traceBegin("HealthInitGetService_" + name);
                 try {
-                    if (manager.getTransport(IHealth.kInterfaceName, name) !=
-                            IServiceManager.Transport.EMPTY) {
-                        mInstanceName = name;
-                        break;
-                    }
+                    newService = healthSupplier.get(name);
+                } catch (NoSuchElementException ex) {
+                    /* ignored, handled below */
                 } finally {
                     traceEnd();
                 }
+                if (newService != null) {
+                    mInstanceName = name;
+                    mLastService.set(newService);
+                    break;
+                }
             }
 
-            if (mInstanceName == null) {
+            if (mInstanceName == null || newService == null) {
                 throw new NoSuchElementException(String.format(
                         "No IHealth service instance among %s is available. Perhaps no permission?",
                         sAllInstances.toString()));
             }
+            mCallback.onRegistration(null, newService, mInstanceName);
 
+            // Register for future service registrations
             traceBegin("HealthInitRegisterNotification");
+            mHandlerThread.start();
             try {
-                manager.registerForNotifications(IHealth.kInterfaceName, mInstanceName, mNotification);
+                managerSupplier.get().registerForNotifications(
+                        IHealth.kInterfaceName, mInstanceName, mNotification);
             } finally {
                 traceEnd();
             }
             Slog.i(TAG, "health: HealthServiceWrapper listening to instance " + mInstanceName);
         }
 
+        @VisibleForTesting
+        HandlerThread getHandlerThread() {
+            return mHandlerThread;
+        }
+
         interface Callback {
             /**
              * This function is invoked asynchronously when a new and related IServiceNotification
@@ -1302,7 +1316,7 @@ public final class BatteryService extends SystemService {
          */
         interface IHealthSupplier {
             default IHealth get(String name) throws NoSuchElementException, RemoteException {
-                return IHealth.getService(name);
+                return IHealth.getService(name, true /* retry */);
             }
         }
 
@@ -1312,18 +1326,27 @@ public final class BatteryService extends SystemService {
                     boolean preexisting) {
                 if (!IHealth.kInterfaceName.equals(interfaceName)) return;
                 if (!mInstanceName.equals(instanceName)) return;
-                try {
-                    // ensures the order of multiple onRegistration on different threads.
-                    synchronized (mLastServiceSetLock) {
-                        IHealth newService = mHealthSupplier.get(instanceName);
-                        IHealth oldService = mLastService.getAndSet(newService);
-                        Slog.i(TAG, "health: new instance registered " + instanceName);
-                        mCallback.onRegistration(oldService, newService, instanceName);
+
+                // This runnable only runs on mHandlerThread and ordering is ensured, hence
+                // no locking is needed inside the runnable.
+                mHandlerThread.getThreadHandler().post(new Runnable() {
+                    @Override
+                    public void run() {
+                        try {
+                            IHealth newService = mHealthSupplier.get(mInstanceName);
+                            IHealth oldService = mLastService.getAndSet(newService);
+
+                            // preexisting may be inaccurate (race). Check for equality here.
+                            if (Objects.equals(newService, oldService)) return;
+
+                            Slog.i(TAG, "health: new instance registered " + mInstanceName);
+                            mCallback.onRegistration(oldService, newService, mInstanceName);
+                        } catch (NoSuchElementException | RemoteException ex) {
+                            Slog.e(TAG, "health: Cannot get instance '" + mInstanceName
+                                    + "': " + ex.getMessage() + ". Perhaps no permission?");
+                        }
                     }
-                } catch (NoSuchElementException | RemoteException ex) {
-                    Slog.e(TAG, "health: Cannot get instance '" + instanceName + "': " +
-                           ex.getMessage() + ". Perhaps no permission?");
-                }
+                });
             }
         }
     }
diff --git a/services/tests/servicestests/src/com/android/server/BatteryServiceTest.java b/services/tests/servicestests/src/com/android/server/BatteryServiceTest.java
index daaad7a8c0b6..106f9e8c2705 100644
--- a/services/tests/servicestests/src/com/android/server/BatteryServiceTest.java
+++ b/services/tests/servicestests/src/com/android/server/BatteryServiceTest.java
@@ -36,12 +36,14 @@ import org.mockito.ArgumentMatcher;
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
 import org.mockito.Spy;
+import org.mockito.invocation.InvocationOnMock;
 
 
 public class BatteryServiceTest extends AndroidTestCase {
 
     @Mock IServiceManager mMockedManager;
     @Mock IHealth mMockedHal;
+    @Mock IHealth mMockedHal2;
 
     @Mock BatteryService.HealthServiceWrapper.Callback mCallback;
     @Mock BatteryService.HealthServiceWrapper.IServiceManagerSupplier mManagerSupplier;
@@ -56,6 +58,12 @@ public class BatteryServiceTest extends AndroidTestCase {
         MockitoAnnotations.initMocks(this);
     }
 
+    @Override
+    public void tearDown() {
+        if (mWrapper != null)
+            mWrapper.getHandlerThread().quitSafely();
+    }
+
     public static <T> ArgumentMatcher<T> isOneOf(Collection<T> collection) {
         return new ArgumentMatcher<T>() {
             @Override public boolean matches(T e) {
@@ -70,42 +78,64 @@ public class BatteryServiceTest extends AndroidTestCase {
     private void initForInstances(String... instanceNamesArr) throws Exception {
         final Collection<String> instanceNames = Arrays.asList(instanceNamesArr);
         doAnswer((invocation) -> {
-                Slog.e("BatteryServiceTest", "health: onRegistration " + invocation.getArguments()[2]);
-                ((IServiceNotification)invocation.getArguments()[2]).onRegistration(
-                        IHealth.kInterfaceName,
-                        (String)invocation.getArguments()[1],
-                        true /* preexisting */);
+                // technically, preexisting is ignored by
+                // BatteryService.HealthServiceWrapper.Notification, but still call it correctly.
+                sendNotification(invocation, true);
+                sendNotification(invocation, true);
+                sendNotification(invocation, false);
                 return null;
             }).when(mMockedManager).registerForNotifications(
                 eq(IHealth.kInterfaceName),
                 argThat(isOneOf(instanceNames)),
                 any(IServiceNotification.class));
 
-        doReturn(mMockedHal).when(mMockedManager)
-            .get(eq(IHealth.kInterfaceName), argThat(isOneOf(instanceNames)));
-
-        doReturn(IServiceManager.Transport.HWBINDER).when(mMockedManager)
-            .getTransport(eq(IHealth.kInterfaceName), argThat(isOneOf(instanceNames)));
-
         doReturn(mMockedManager).when(mManagerSupplier).get();
-        doReturn(mMockedHal).when(mHealthServiceSupplier)
-            .get(argThat(isOneOf(instanceNames)));
+        doReturn(mMockedHal)        // init calls this
+            .doReturn(mMockedHal)   // notification 1
+            .doReturn(mMockedHal)   // notification 2
+            .doReturn(mMockedHal2)  // notification 3
+            .doThrow(new RuntimeException("Should not call getService for more than 4 times"))
+            .when(mHealthServiceSupplier).get(argThat(isOneOf(instanceNames)));
 
         mWrapper = new BatteryService.HealthServiceWrapper();
     }
 
+    private void waitHandlerThreadFinish() throws Exception {
+        for (int i = 0; i < 5; i++) {
+            if (!mWrapper.getHandlerThread().getThreadHandler().hasMessagesOrCallbacks()) {
+                return;
+            }
+            Thread.sleep(300);
+        }
+        assertFalse(mWrapper.getHandlerThread().getThreadHandler().hasMessagesOrCallbacks());
+    }
+
+    private static void sendNotification(InvocationOnMock invocation, boolean preexisting)
+            throws Exception {
+        ((IServiceNotification)invocation.getArguments()[2]).onRegistration(
+                IHealth.kInterfaceName,
+                (String)invocation.getArguments()[1],
+                preexisting);
+    }
+
     @SmallTest
     public void testWrapPreferVendor() throws Exception {
         initForInstances(VENDOR, HEALTHD);
         mWrapper.init(mCallback, mManagerSupplier, mHealthServiceSupplier);
-        verify(mCallback).onRegistration(same(null), same(mMockedHal), eq(VENDOR));
+        waitHandlerThreadFinish();
+        verify(mCallback, times(1)).onRegistration(same(null), same(mMockedHal), eq(VENDOR));
+        verify(mCallback, never()).onRegistration(same(mMockedHal), same(mMockedHal), anyString());
+        verify(mCallback, times(1)).onRegistration(same(mMockedHal), same(mMockedHal2), eq(VENDOR));
     }
 
     @SmallTest
     public void testUseHealthd() throws Exception {
         initForInstances(HEALTHD);
         mWrapper.init(mCallback, mManagerSupplier, mHealthServiceSupplier);
-        verify(mCallback).onRegistration(same(null), same(mMockedHal), eq(HEALTHD));
+        waitHandlerThreadFinish();
+        verify(mCallback, times(1)).onRegistration(same(null), same(mMockedHal), eq(HEALTHD));
+        verify(mCallback, never()).onRegistration(same(mMockedHal), same(mMockedHal), anyString());
+        verify(mCallback, times(1)).onRegistration(same(mMockedHal), same(mMockedHal2), eq(HEALTHD));
     }
 
     @SmallTest