From 8aea09fecd307d8a89ee5814501fa358655a457f Mon Sep 17 00:00:00 2001
From: Soonil Nagarkar <sooniln@google.com>
Date: Wed, 18 Mar 2020 19:17:52 -0700
Subject: DO NOT MERGE Don't lock listener delivery

Previous versions of GNSS listeners commited to not holding locks even
when running directly unfortunately. We do some extra work to avoid
holding locks while running listeners.

Bug: 151291181
Test: manual
Change-Id: I0b7f46c30df2f97ce11db45b69443a9994885309
---
 .../android/location/AbstractListenerManager.java  | 36 ++++++++++++++--------
 1 file changed, 23 insertions(+), 13 deletions(-)

(limited to 'location/java')

diff --git a/location/java/android/location/AbstractListenerManager.java b/location/java/android/location/AbstractListenerManager.java
index 3dc7cfce2d92..36b86899f2d8 100644
--- a/location/java/android/location/AbstractListenerManager.java
+++ b/location/java/android/location/AbstractListenerManager.java
@@ -85,11 +85,13 @@ abstract class AbstractListenerManager<TRequest, TListener> {
         }
     }
 
-    @GuardedBy("mListeners")
-    private final ArrayMap<Object, Registration<TRequest, TListener>> mListeners =
+    private final Object mLock = new Object();
+
+    @GuardedBy("mLock")
+    private volatile ArrayMap<Object, Registration<TRequest, TListener>> mListeners =
             new ArrayMap<>();
 
-    @GuardedBy("mListeners")
+    @GuardedBy("mLock")
     @Nullable
     private TRequest mMergedRequest;
 
@@ -129,10 +131,16 @@ abstract class AbstractListenerManager<TRequest, TListener> {
             throws RemoteException {
         Preconditions.checkNotNull(registration);
 
-        synchronized (mListeners) {
+        synchronized (mLock) {
             boolean initialRequest = mListeners.isEmpty();
 
-            Registration<TRequest, TListener> oldRegistration = mListeners.put(key, registration);
+            ArrayMap<Object, Registration<TRequest, TListener>> newListeners = new ArrayMap<>(
+                    mListeners.size() + 1);
+            newListeners.putAll(mListeners);
+            Registration<TRequest, TListener> oldRegistration = newListeners.put(key,
+                    registration);
+            mListeners = newListeners;
+
             if (oldRegistration != null) {
                 oldRegistration.unregister();
             }
@@ -151,8 +159,12 @@ abstract class AbstractListenerManager<TRequest, TListener> {
     }
 
     public void removeListener(Object listener) throws RemoteException {
-        synchronized (mListeners) {
-            Registration<TRequest, TListener> oldRegistration = mListeners.remove(listener);
+        synchronized (mLock) {
+            ArrayMap<Object, Registration<TRequest, TListener>> newListeners = new ArrayMap<>(
+                    mListeners);
+            Registration<TRequest, TListener> oldRegistration = newListeners.remove(listener);
+            mListeners = newListeners;
+
             if (oldRegistration == null) {
                 return;
             }
@@ -190,18 +202,16 @@ abstract class AbstractListenerManager<TRequest, TListener> {
     }
 
     protected void execute(Consumer<TListener> operation) {
-        synchronized (mListeners) {
-            for (Registration<TRequest, TListener> registration : mListeners.values()) {
-                registration.execute(operation);
-            }
+        for (Registration<TRequest, TListener> registration : mListeners.values()) {
+            registration.execute(operation);
         }
     }
 
-    @GuardedBy("mListeners")
+    @GuardedBy("mLock")
     @SuppressWarnings("unchecked")
     @Nullable
     private TRequest mergeRequests() {
-        Preconditions.checkState(Thread.holdsLock(mListeners));
+        Preconditions.checkState(Thread.holdsLock(mLock));
 
         if (mListeners.isEmpty()) {
             return null;
-- 
cgit v1.2.3-59-g8ed1b