pastk/organicmaps
1
Fork 0
forked from organicmaps/organicmaps
Code Pull requests Activity

Allow to modify list of listeners in LocationHelper from callbacks

Browse source 
It's a legit case when a listener is removed from the list as a result of call to that listener. That causes the list of listeners to be modified during an iteration, which is not supported by standart collections.

The most efficient option to support that is to use ObserverList from Chromium project and this is done here.

Another options are a) to use CopyOnWriteArrayList b) to use Listeners class from the codebase. Both of them are less efficient and do more allocations.

Resolves #546

Signed-off-by: Alexey Krasilnikov <alexey@krasilnikov.me>
This commit is contained in:
Alexey Krasilnikov 2025-01-14 22:44:08 +07:00 • committed by Viktor Havaka
parent a1c6d532b5
commit aaad6ecb44
3 changed files with 584 additions and 15 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

35
android/app/src/main/java/app/organicmaps/location/LocationHelper.java
View file

@ -18,6 +18,8 @@ import androidx.core.content.ContextCompat;
import androidx.core.location.GnssStatusCompat;
import androidx.core.location.LocationManagerCompat;
import org.chromium.base.ObserverList;
import app.organicmaps.Framework;
import app.organicmaps.Map;
import app.organicmaps.MwmApplication;
@ -30,9 +32,6 @@ import app.organicmaps.util.LocationUtils;
import app.organicmaps.util.NetworkPolicy;
import app.organicmaps.util.log.Logger;
import java.util.LinkedHashSet;
import java.util.Set;
public class LocationHelper implements BaseLocationProvider.Listener
{
private static final long INTERVAL_FOLLOW_MS = 0;
@ -47,8 +46,10 @@ public class LocationHelper implements BaseLocationProvider.Listener
private final Context mContext;
private static final String TAG = LocationState.LOCATION_TAG;
@NonNull
private final Set<LocationListener> mListeners = new LinkedHashSet<>();
private final ObserverList<LocationListener> mListeners = new ObserverList<>();
private final ObserverList.RewindableIterator<LocationListener> mListenersIterator = mListeners.rewindableIterator();
@Nullable
private Location mSavedLocation;
private MapObject mMyPosition;
@ -156,8 +157,9 @@ public class LocationHelper implements BaseLocationProvider.Listener
mHandler.removeCallbacks(mLocationTimeoutRunnable);
mHandler.postDelayed(mLocationTimeoutRunnable, LOCATION_UPDATE_TIMEOUT_MS); // Reset the timeout.
for (LocationListener listener : mListeners)
listener.onLocationUpdated(mSavedLocation);
mListenersIterator.rewind();
while (mListenersIterator.hasNext())
mListenersIterator.next().onLocationUpdated(mSavedLocation);
// If we are still in the first run mode, i.e. user is staying on the first run screens,
// not on the map, we mustn't post location update to the core. Only this preserving allows us
@ -187,8 +189,9 @@ public class LocationHelper implements BaseLocationProvider.Listener
}
Logger.d(TAG);
for (LocationListener listener : mListeners)
listener.onLocationUpdateTimeout();
mListenersIterator.rewind();
while (mListenersIterator.hasNext())
mListenersIterator.next().onLocationUpdateTimeout();
}
@Override
@ -240,8 +243,9 @@ public class LocationHelper implements BaseLocationProvider.Listener
stop();
LocationState.nativeOnLocationError(LocationState.ERROR_GPS_OFF);
for (LocationListener listener : mListeners)
listener.onLocationResolutionRequired(pendingIntent);
mListenersIterator.rewind();
while (mListenersIterator.hasNext())
mListenersIterator.next().onLocationResolutionRequired(pendingIntent);
}
// Used by GoogleFusedLocationProvider.
@ -281,8 +285,9 @@ public class LocationHelper implements BaseLocationProvider.Listener
stop();
LocationState.nativeOnLocationError(LocationState.ERROR_GPS_OFF);
for (LocationListener listener : mListeners)
listener.onLocationDisabled();
mListenersIterator.rewind();
while (mListenersIterator.hasNext())
mListenersIterator.next().onLocationDisabled();
}
/**
@ -295,7 +300,7 @@ public class LocationHelper implements BaseLocationProvider.Listener
{
Logger.d(TAG, "listener: " + listener + " count was: " + mListeners.size());
mListeners.add(listener);
mListeners.addObserver(listener);
if (mSavedLocation != null)
listener.onLocationUpdated(mSavedLocation);
}
@ -308,7 +313,7 @@ public class LocationHelper implements BaseLocationProvider.Listener
public void removeListener(@NonNull LocationListener listener)
{
Logger.d(TAG, "listener: " + listener + " count was: " + mListeners.size());
mListeners.remove(listener);
mListeners.removeObserver(listener);
}
private long calcLocationUpdatesInterval()

251
android/app/src/main/java/org/chromium/base/ObserverList.java Normal file
View file

@ -0,0 +1,251 @@
// Copyright 2013 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
package org.chromium.base;
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;
import java.util.NoSuchElementException;
import javax.annotation.concurrent.NotThreadSafe;
/**
* A container for a list of observers.
* <p/>
* This container can be modified during iteration without invalidating the iterator.
* So, it safely handles the case of an observer removing itself or other observers from the list
* while observers are being notified.
* <p/>
* The implementation (and the interface) is heavily influenced by the C++ ObserverList.
* Notable differences:
* - The iterator implements NOTIFY_EXISTING_ONLY.
* - The range-based for loop is left to the clients to implement in terms of iterator().
* <p/>
* This class is not threadsafe. Observers MUST be added, removed and will be notified on the same
* thread this is created.
*
* @param <E> The type of observers that this list should hold.
*/
@NotThreadSafe
public class ObserverList<E> implements Iterable<E> {
/** Extended iterator interface that provides rewind functionality. */
public interface RewindableIterator<E> extends Iterator<E> {
/**
* Rewind the iterator back to the beginning.
*
* If we need to iterate multiple times, we can avoid iterator object reallocation by using
* this method.
*/
public void rewind();
}
public final List<E> mObservers = new ArrayList<E>();
private int mIterationDepth;
private int mCount;
private boolean mNeedsCompact;
/**
* Add an observer to the list.
* <p/>
* An observer should not be added to the same list more than once. If an iteration is already
* in progress, this observer will be not be visible during that iteration.
*
* @return true if the observer list changed as a result of the call.
*/
public boolean addObserver(@Nullable E obs) {
// Avoid adding null elements to the list as they may be removed on a compaction.
if (obs == null || mObservers.contains(obs)) {
return false;
}
// Structurally modifying the underlying list here. This means we
// cannot use the underlying list's iterator to iterate over the list.
boolean result = mObservers.add(obs);
assert result;
++mCount;
return true;
}
/**
* Remove an observer from the list if it is in the list.
*
* @return true if an element was removed as a result of this call.
*/
public boolean removeObserver(@Nullable E obs) {
if (obs == null) {
return false;
}
int index = mObservers.indexOf(obs);
if (index == -1) {
return false;
}
if (mIterationDepth == 0) {
// No one is iterating over the list.
mObservers.remove(index);
} else {
mNeedsCompact = true;
mObservers.set(index, null);
}
--mCount;
assert mCount >= 0;
return true;
}
public boolean hasObserver(@Nullable E obs) {
if (obs == null) {
return false;
}
return mObservers.contains(obs);
}
public void clear() {
mCount = 0;
if (mIterationDepth == 0) {
mObservers.clear();
return;
}
int size = mObservers.size();
mNeedsCompact |= size != 0;
for (int i = 0; i < size; i++) {
mObservers.set(i, null);
}
}
@NonNull
@Override
public Iterator<E> iterator() {
return new ObserverListIterator();
}
/**
* It's the same as {@link ObserverList#iterator()} but the return type is
* {@link RewindableIterator}. Use this iterator type if you need to use
* {@link RewindableIterator#rewind()}.
*/
public RewindableIterator<E> rewindableIterator() {
return new ObserverListIterator();
}
/**
* Returns the number of observers currently registered in the ObserverList.
* This is equivalent to the number of non-empty spaces in |mObservers|.
*/
public int size() {
return mCount;
}
/** Returns true if the ObserverList contains no observers. */
public boolean isEmpty() {
return mCount == 0;
}
/**
* Compact the underlying list be removing null elements.
* <p/>
* Should only be called when mIterationDepth is zero.
*/
private void compact() {
assert mIterationDepth == 0;
for (int i = mObservers.size() - 1; i >= 0; i--) {
if (mObservers.get(i) == null) {
mObservers.remove(i);
}
}
}
private void incrementIterationDepth() {
mIterationDepth++;
}
private void decrementIterationDepthAndCompactIfNeeded() {
mIterationDepth--;
assert mIterationDepth >= 0;
if (mIterationDepth > 0) return;
if (!mNeedsCompact) return;
mNeedsCompact = false;
compact();
}
/**
* Returns the size of the underlying storage of the ObserverList.
* It will take into account the empty spaces inside |mObservers|.
*/
private int capacity() {
return mObservers.size();
}
private E getObserverAt(int index) {
return mObservers.get(index);
}
private class ObserverListIterator implements RewindableIterator<E> {
private int mListEndMarker;
private int mIndex;
private boolean mIsExhausted;
private ObserverListIterator() {
ObserverList.this.incrementIterationDepth();
mListEndMarker = ObserverList.this.capacity();
}
@Override
public void rewind() {
compactListIfNeeded();
ObserverList.this.incrementIterationDepth();
mListEndMarker = ObserverList.this.capacity();
mIsExhausted = false;
mIndex = 0;
}
@Override
public boolean hasNext() {
int lookupIndex = mIndex;
while (lookupIndex < mListEndMarker
&& ObserverList.this.getObserverAt(lookupIndex) == null) {
lookupIndex++;
}
if (lookupIndex < mListEndMarker) return true;
// We have reached the end of the list, allow for compaction.
compactListIfNeeded();
return false;
}
@Override
public E next() {
// Advance if the current element is null.
while (mIndex < mListEndMarker && ObserverList.this.getObserverAt(mIndex) == null) {
mIndex++;
}
if (mIndex < mListEndMarker) return ObserverList.this.getObserverAt(mIndex++);
// We have reached the end of the list, allow for compaction.
compactListIfNeeded();
throw new NoSuchElementException();
}
@Override
public void remove() {
throw new UnsupportedOperationException();
}
private void compactListIfNeeded() {
if (!mIsExhausted) {
mIsExhausted = true;
ObserverList.this.decrementIterationDepthAndCompactIfNeeded();
}
}
}
}

313
android/app/src/test/java/org/chromium/base/ObserverListTest.java Normal file
View file

@ -0,0 +1,313 @@
// Copyright 2017 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
package org.chromium.base;
import org.junit.Assert;
import org.junit.Test;
import java.util.Collection;
import java.util.Iterator;
import java.util.NoSuchElementException;
/** Tests for (@link ObserverList}. */
public class ObserverListTest {
interface Observer {
void observe(int x);
}
private static class Foo implements Observer {
private final int mScalar;
private int mTotal;
Foo(int scalar) {
mScalar = scalar;
}
@Override
public void observe(int x) {
mTotal += x * mScalar;
}
}
/** An observer which add a given Observer object to the list when observe is called. */
private static class FooAdder implements Observer {
private final ObserverList<Observer> mList;
private final Observer mLucky;
FooAdder(ObserverList<Observer> list, Observer oblivious) {
mList = list;
mLucky = oblivious;
}
@Override
public void observe(int x) {
mList.addObserver(mLucky);
}
}
/** An observer which removes a given Observer object from the list when observe is called. */
private static class FooRemover implements Observer {
private final ObserverList<Observer> mList;
private final Observer mDoomed;
FooRemover(ObserverList<Observer> list, Observer innocent) {
mList = list;
mDoomed = innocent;
}
@Override
public void observe(int x) {
mList.removeObserver(mDoomed);
}
}
private static <T> int getSizeOfIterable(Iterable<T> iterable) {
if (iterable instanceof Collection<?>) return ((Collection<?>) iterable).size();
int num = 0;
for (T el : iterable) num++;
return num;
}
@Test
public void testRemoveWhileIteration() {
ObserverList<Observer> observerList = new ObserverList<Observer>();
Foo a = new Foo(1);
Foo b = new Foo(-1);
Foo c = new Foo(1);
Foo d = new Foo(-1);
Foo e = new Foo(-1);
FooRemover evil = new FooRemover(observerList, c);
observerList.addObserver(a);
observerList.addObserver(b);
for (Observer obs : observerList) obs.observe(10);
// Removing an observer not in the list should do nothing.
observerList.removeObserver(e);
observerList.addObserver(evil);
observerList.addObserver(c);
observerList.addObserver(d);
for (Observer obs : observerList) obs.observe(10);
// observe should be called twice on a.
Assert.assertEquals(20, a.mTotal);
// observe should be called twice on b.
Assert.assertEquals(-20, b.mTotal);
// evil removed c from the observerList before it got any callbacks.
Assert.assertEquals(0, c.mTotal);
// observe should be called once on d.
Assert.assertEquals(-10, d.mTotal);
// e was never added to the list, observe should not be called.
Assert.assertEquals(0, e.mTotal);
}
@Test
public void testAddWhileIteration() {
ObserverList<Observer> observerList = new ObserverList<Observer>();
Foo a = new Foo(1);
Foo b = new Foo(-1);
Foo c = new Foo(1);
FooAdder evil = new FooAdder(observerList, c);
observerList.addObserver(evil);
observerList.addObserver(a);
observerList.addObserver(b);
for (Observer obs : observerList) obs.observe(10);
Assert.assertTrue(observerList.hasObserver(c));
Assert.assertEquals(10, a.mTotal);
Assert.assertEquals(-10, b.mTotal);
Assert.assertEquals(0, c.mTotal);
}
@Test
public void testIterator() {
ObserverList<Integer> observerList = new ObserverList<Integer>();
observerList.addObserver(5);
observerList.addObserver(10);
observerList.addObserver(15);
Assert.assertEquals(3, getSizeOfIterable(observerList));
observerList.removeObserver(10);
Assert.assertEquals(2, getSizeOfIterable(observerList));
Iterator<Integer> it = observerList.iterator();
Assert.assertTrue(it.hasNext());
Assert.assertEquals(5, (int) it.next());
Assert.assertTrue(it.hasNext());
Assert.assertEquals(15, (int) it.next());
Assert.assertFalse(it.hasNext());
boolean removeExceptionThrown = false;
try {
it.remove();
Assert.fail("Expecting UnsupportedOperationException to be thrown here.");
} catch (UnsupportedOperationException e) {
removeExceptionThrown = true;
}
Assert.assertTrue(removeExceptionThrown);
Assert.assertEquals(2, getSizeOfIterable(observerList));
boolean noElementExceptionThrown = false;
try {
it.next();
Assert.fail("Expecting NoSuchElementException to be thrown here.");
} catch (NoSuchElementException e) {
noElementExceptionThrown = true;
}
Assert.assertTrue(noElementExceptionThrown);
}
@Test
public void testRewindableIterator() {
ObserverList<Integer> observerList = new ObserverList<Integer>();
observerList.addObserver(5);
observerList.addObserver(10);
observerList.addObserver(15);
Assert.assertEquals(3, getSizeOfIterable(observerList));
ObserverList.RewindableIterator<Integer> it = observerList.rewindableIterator();
Assert.assertTrue(it.hasNext());
Assert.assertEquals(5, (int) it.next());
Assert.assertTrue(it.hasNext());
Assert.assertEquals(10, (int) it.next());
Assert.assertTrue(it.hasNext());
Assert.assertEquals(15, (int) it.next());
Assert.assertFalse(it.hasNext());
it.rewind();
Assert.assertTrue(it.hasNext());
Assert.assertEquals(5, (int) it.next());
Assert.assertTrue(it.hasNext());
Assert.assertEquals(10, (int) it.next());
Assert.assertTrue(it.hasNext());
Assert.assertEquals(15, (int) it.next());
Assert.assertEquals(5, (int) observerList.mObservers.get(0));
observerList.removeObserver(5);
Assert.assertEquals(null, observerList.mObservers.get(0));
it.rewind();
Assert.assertEquals(10, (int) observerList.mObservers.get(0));
Assert.assertTrue(it.hasNext());
Assert.assertEquals(10, (int) it.next());
Assert.assertTrue(it.hasNext());
Assert.assertEquals(15, (int) it.next());
}
@Test
public void testAddObserverReturnValue() {
ObserverList<Object> observerList = new ObserverList<Object>();
Object a = new Object();
Assert.assertTrue(observerList.addObserver(a));
Assert.assertFalse(observerList.addObserver(a));
Object b = new Object();
Assert.assertTrue(observerList.addObserver(b));
Assert.assertFalse(observerList.addObserver(null));
}
@Test
public void testRemoveObserverReturnValue() {
ObserverList<Object> observerList = new ObserverList<Object>();
Object a = new Object();
Object b = new Object();
observerList.addObserver(a);
observerList.addObserver(b);
Assert.assertTrue(observerList.removeObserver(a));
Assert.assertFalse(observerList.removeObserver(a));
Assert.assertFalse(observerList.removeObserver(new Object()));
Assert.assertTrue(observerList.removeObserver(b));
Assert.assertFalse(observerList.removeObserver(null));
// If we remove an object while iterating, it will be replaced by 'null'.
observerList.addObserver(a);
Assert.assertTrue(observerList.removeObserver(a));
Assert.assertFalse(observerList.removeObserver(null));
}
@Test
public void testSize() {
ObserverList<Object> observerList = new ObserverList<Object>();
Assert.assertEquals(0, observerList.size());
Assert.assertTrue(observerList.isEmpty());
observerList.addObserver(null);
Assert.assertEquals(0, observerList.size());
Assert.assertTrue(observerList.isEmpty());
Object a = new Object();
observerList.addObserver(a);
Assert.assertEquals(1, observerList.size());
Assert.assertFalse(observerList.isEmpty());
observerList.addObserver(a);
Assert.assertEquals(1, observerList.size());
Assert.assertFalse(observerList.isEmpty());
observerList.addObserver(null);
Assert.assertEquals(1, observerList.size());
Assert.assertFalse(observerList.isEmpty());
Object b = new Object();
observerList.addObserver(b);
Assert.assertEquals(2, observerList.size());
Assert.assertFalse(observerList.isEmpty());
observerList.removeObserver(null);
Assert.assertEquals(2, observerList.size());
Assert.assertFalse(observerList.isEmpty());
observerList.removeObserver(new Object());
Assert.assertEquals(2, observerList.size());
Assert.assertFalse(observerList.isEmpty());
observerList.removeObserver(b);
Assert.assertEquals(1, observerList.size());
Assert.assertFalse(observerList.isEmpty());
observerList.removeObserver(b);
Assert.assertEquals(1, observerList.size());
Assert.assertFalse(observerList.isEmpty());
observerList.removeObserver(a);
Assert.assertEquals(0, observerList.size());
Assert.assertTrue(observerList.isEmpty());
observerList.removeObserver(a);
observerList.removeObserver(b);
observerList.removeObserver(null);
observerList.removeObserver(new Object());
Assert.assertEquals(0, observerList.size());
Assert.assertTrue(observerList.isEmpty());
observerList.addObserver(new Object());
observerList.addObserver(new Object());
observerList.addObserver(new Object());
observerList.addObserver(a);
Assert.assertEquals(4, observerList.size());
Assert.assertFalse(observerList.isEmpty());
observerList.clear();
Assert.assertEquals(0, observerList.size());
Assert.assertTrue(observerList.isEmpty());
observerList.removeObserver(a);
observerList.removeObserver(b);
observerList.removeObserver(null);
observerList.removeObserver(new Object());
Assert.assertEquals(0, observerList.size());
Assert.assertTrue(observerList.isEmpty());
}
}