From 678064a8f745ac89c2d3a5c48e09d1c10eb35dea Mon Sep 17 00:00:00 2001 From: Pavel Maltsev Date: Wed, 12 Apr 2017 14:33:09 -0700 Subject: Fixing sync in Car(Sensor|Diagnostic)Manager Bug: b/37254546 Test: verified e2e tests no longer crashing with ConcurrentModificationException Change-Id: I2e295bfec36f6e0b4a6c4187ad8d8c7eaf9250c7 --- .../android/car/hardware/CarDiagnosticManager.java | 14 ++++++-- .../src/android/car/hardware/CarSensorManager.java | 37 +++++++++++----------- .../android/car/internal/CarRatedListeners.java | 6 ++-- 3 files changed, 33 insertions(+), 24 deletions(-) diff --git a/car-lib/src/android/car/hardware/CarDiagnosticManager.java b/car-lib/src/android/car/hardware/CarDiagnosticManager.java index d3ad137405..6d8b5ba58a 100644 --- a/car-lib/src/android/car/hardware/CarDiagnosticManager.java +++ b/car-lib/src/android/car/hardware/CarDiagnosticManager.java @@ -33,6 +33,7 @@ import com.android.car.internal.CarRatedListeners; import com.android.car.internal.SingleMessageHandler; import java.lang.ref.WeakReference; +import java.util.ArrayList; import java.util.List; import java.util.function.Consumer; @@ -60,8 +61,10 @@ public final class CarDiagnosticManager implements CarManagerBase { MSG_DIAGNOSTIC_EVENTS) { @Override protected void handleEvent(CarDiagnosticEvent event) { - CarDiagnosticListeners listeners = - mActiveListeners.get(event.frameType); + CarDiagnosticListeners listeners; + synchronized (mActiveListeners) { + listeners = mActiveListeners.get(event.frameType); + } if (listeners != null) { listeners.onDiagnosticEvent(event); } @@ -292,7 +295,12 @@ public final class CarDiagnosticManager implements CarManagerBase { final CarDiagnosticEvent eventToDispatch = hasVendorExtensionPermission ? event : event.withVendorSensorsRemoved(); - getListeners().forEach(new Consumer() { + List listeners; + synchronized (mActiveListeners) { + listeners = new ArrayList<>(getListeners()); + } + listeners.forEach(new Consumer() { + @Override public void accept(OnDiagnosticEventListener listener) { listener.onDiagnosticEvent(eventToDispatch); diff --git a/car-lib/src/android/car/hardware/CarSensorManager.java b/car-lib/src/android/car/hardware/CarSensorManager.java index 0878e1f8d2..6e81b08da0 100644 --- a/car-lib/src/android/car/hardware/CarSensorManager.java +++ b/car-lib/src/android/car/hardware/CarSensorManager.java @@ -26,10 +26,11 @@ import android.car.CarManagerBase; import android.car.CarNotConnectedException; import android.content.Context; import android.os.Handler; -import android.os.Handler.Callback; import android.os.IBinder; import android.os.RemoteException; import android.util.Log; +import android.util.SparseArray; +import android.util.SparseIntArray; import com.android.car.internal.CarRatedListeners; import com.android.car.internal.SingleMessageHandler; @@ -37,7 +38,7 @@ import com.android.car.internal.SingleMessageHandler; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.ref.WeakReference; -import java.util.HashMap; +import java.util.ArrayList; import java.util.Iterator; import java.util.List; import java.util.function.Consumer; @@ -192,8 +193,7 @@ public final class CarSensorManager implements CarManagerBase { * To keep record of locally active sensors. Key is sensor type. This is used as a basic lock * for all client accesses. */ - private final HashMap mActiveSensorListeners = - new HashMap(); + private final SparseArray mActiveSensorListeners = new SparseArray<>(); /** Handles call back into clients. */ private final SingleMessageHandler mHandlerCallback; @@ -206,8 +206,10 @@ public final class CarSensorManager implements CarManagerBase { MSG_SENSOR_EVENTS) { @Override protected void handleEvent(CarSensorEvent event) { - CarSensorListeners listeners = - mActiveSensorListeners.get(event.sensorType); + CarSensorListeners listeners; + synchronized (mActiveSensorListeners) { + listeners = mActiveSensorListeners.get(event.sensorType); + } if (listeners != null) { listeners.onSensorChanged(event); } @@ -344,10 +346,8 @@ public final class CarSensorManager implements CarManagerBase { public void unregisterListener(OnSensorChangedListener listener) { //TODO: removing listener should reset update rate, bug: 32060307 synchronized(mActiveSensorListeners) { - Iterator sensorIterator = mActiveSensorListeners.keySet().iterator(); - while (sensorIterator.hasNext()) { - Integer sensor = sensorIterator.next(); - doUnregisterListenerLocked(listener, sensor, sensorIterator); + for (int i = 0; i < mActiveSensorListeners.size(); i++) { + doUnregisterListenerLocked(listener, mActiveSensorListeners.keyAt(i)); } } } @@ -360,12 +360,11 @@ public final class CarSensorManager implements CarManagerBase { */ public void unregisterListener(OnSensorChangedListener listener, @SensorType int sensorType) { synchronized(mActiveSensorListeners) { - doUnregisterListenerLocked(listener, sensorType, null); + doUnregisterListenerLocked(listener, sensorType); } } - private void doUnregisterListenerLocked(OnSensorChangedListener listener, Integer sensor, - Iterator sensorIterator) { + private void doUnregisterListenerLocked(OnSensorChangedListener listener, Integer sensor) { CarSensorListeners listeners = mActiveSensorListeners.get(sensor); if (listeners != null) { boolean needsServerUpdate = false; @@ -379,11 +378,7 @@ public final class CarSensorManager implements CarManagerBase { } catch (RemoteException e) { //ignore } - if (sensorIterator == null) { - mActiveSensorListeners.remove(sensor); - } else { - sensorIterator.remove(); - } + mActiveSensorListeners.remove(sensor); } else if (needsServerUpdate) { try { registerOrUpdateSensorListener(sensor, listeners.getRate()); @@ -479,7 +474,11 @@ public final class CarSensorManager implements CarManagerBase { return; } mLastUpdateTime = updateTime; - getListeners().forEach(new Consumer() { + List listeners; + synchronized (mActiveSensorListeners) { + listeners = new ArrayList<>(getListeners()); + } + listeners.forEach(new Consumer() { @Override public void accept(OnSensorChangedListener listener) { listener.onSensorChanged(event); diff --git a/car-lib/src/com/android/car/internal/CarRatedListeners.java b/car-lib/src/com/android/car/internal/CarRatedListeners.java index 4d976dc7b2..6b26e612c1 100644 --- a/car-lib/src/com/android/car/internal/CarRatedListeners.java +++ b/car-lib/src/com/android/car/internal/CarRatedListeners.java @@ -16,6 +16,7 @@ package com.android.car.internal; +import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.Map; @@ -25,7 +26,8 @@ import java.util.Map; * @hide */ public class CarRatedListeners { - private final Map mListenersToRate = new HashMap<>(); + private final Map mListenersToRate = new HashMap<>(4); + private int mUpdateRate; protected long mLastUpdateTime = -1; @@ -83,7 +85,7 @@ public class CarRatedListeners { return false; } - public Iterable getListeners() { + public Collection getListeners() { return mListenersToRate.keySet(); } } -- cgit v1.2.3