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
diff --git a/services/core/java/com/android/server/BatteryService.java b/services/core/java/com/android/server/BatteryService.java
index 1b61866..7a60a5d 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.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.util.Arrays;
import java.util.List;
import java.util.NoSuchElementException;
+import java.util.Objects;
import java.util.concurrent.atomic.AtomicReference;
/**
@@ -176,6 +178,7 @@
private HealthServiceWrapper mHealthServiceWrapper;
private HealthHalCallback mHealthHalCallback;
private BatteryPropertiesRegistrar mBatteryPropertiesRegistrar;
+ private HandlerThread mHandlerThread;
public BatteryService(Context context) {
super(context);
@@ -1195,14 +1198,13 @@
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 @@
* 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 @@
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 @@
*/
interface IHealthSupplier {
default IHealth get(String name) throws NoSuchElementException, RemoteException {
- return IHealth.getService(name);
+ return IHealth.getService(name, true /* retry */);
}
}
@@ -1312,18 +1326,27 @@
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 daaad7a8..106f9e8 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.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 @@
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 @@
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