summaryrefslogtreecommitdiff
path: root/platform
diff options
context:
space:
mode:
authorŁukasz Paczos <lukas.paczos@gmail.com>2018-07-31 19:42:09 +0200
committerŁukasz Paczos <lukasz.paczos@mapbox.com>2018-08-10 18:31:52 +0200
commit63d4ac05f20190d7ac3df1f852840e9678b9e7dd (patch)
tree563c6308ca6c0c8cad0e4a10a1c76352c2e31cc8 /platform
parentb500577bfa9759241b374c161a06c1bef79cf4cd (diff)
downloadqtlocation-mapboxgl-63d4ac05f20190d7ac3df1f852840e9678b9e7dd.tar.gz
[android] shutting down thread pool of the CustomGeometrySource when the source is destroyed
Diffstat (limited to 'platform')
-rw-r--r--platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/style/sources/CustomGeometrySource.java83
-rw-r--r--platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/style/CustomGeometrySourceTest.kt68
-rw-r--r--platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/style/GridSourceActivity.java12
-rwxr-xr-xplatform/android/src/native_map_view.cpp4
-rw-r--r--platform/android/src/style/sources/custom_geometry_source.cpp39
-rw-r--r--platform/android/src/style/sources/custom_geometry_source.hpp5
-rw-r--r--platform/android/src/style/sources/source.cpp7
-rw-r--r--platform/android/src/style/sources/source.hpp6
8 files changed, 198 insertions, 26 deletions
diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/style/sources/CustomGeometrySource.java b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/style/sources/CustomGeometrySource.java
index 1f6029e2a2..6c0b76f00e 100644
--- a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/style/sources/CustomGeometrySource.java
+++ b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/style/sources/CustomGeometrySource.java
@@ -15,18 +15,26 @@ import java.lang.ref.WeakReference;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
+import java.util.Locale;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
+import java.util.concurrent.ThreadFactory;
import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
/**
* Custom Vector Source, allows using FeatureCollections.
- *
*/
@UiThread
public class CustomGeometrySource extends Source {
+ public static final String THREAD_PREFIX = "CustomGeom";
+ public static final int THREAD_POOL_LIMIT = 4;
+ private static final AtomicInteger poolCount = new AtomicInteger();
+ private final Lock executorLock = new ReentrantLock();
private ExecutorService executor;
private GeometryTileProvider provider;
private final Map<TileID, AtomicBoolean> cancelledTileRequests = new ConcurrentHashMap<>();
@@ -34,7 +42,7 @@ public class CustomGeometrySource extends Source {
/**
* Create a CustomGeometrySource
*
- * @param id The source id.
+ * @param id The source id.
* @param provider The tile provider that returns geometry data for this source.
*/
public CustomGeometrySource(String id, GeometryTileProvider provider) {
@@ -45,21 +53,20 @@ public class CustomGeometrySource extends Source {
* Create a CustomGeometrySource with non-default CustomGeometrySourceOptions.
* <p>Supported options are minZoom, maxZoom, buffer, and tolerance.</p>
*
- * @param id The source id.
+ * @param id The source id.
* @param provider The tile provider that returns geometry data for this source.
- * @param options CustomGeometrySourceOptions.
+ * @param options CustomGeometrySourceOptions.
*/
public CustomGeometrySource(String id, GeometryTileProvider provider, CustomGeometrySourceOptions options) {
super();
this.provider = provider;
- executor = Executors.newFixedThreadPool(4);
initialize(id, options);
}
/**
- * Invalidate previously provided features within a given bounds at all zoom levels.
- * Invoking this method will result in new requests to `GeometryTileProvider` for regions
- * that contain, include, or intersect with the provided bounds.
+ * Invalidate previously provided features within a given bounds at all zoom levels.
+ * Invoking this method will result in new requests to `GeometryTileProvider` for regions
+ * that contain, include, or intersect with the provided bounds.
*
* @param bounds The region in which features should be invalidated at all zoom levels
*/
@@ -73,8 +80,8 @@ public class CustomGeometrySource extends Source {
* in new requests to `GeometryTileProvider` for visible tiles.
*
* @param zoomLevel Tile zoom level.
- * @param x Tile X coordinate.
- * @param y Tile Y coordinate.
+ * @param x Tile X coordinate.
+ * @param y Tile Y coordinate.
*/
public void invalidateTile(int zoomLevel, int x, int y) {
checkThread();
@@ -87,9 +94,9 @@ public class CustomGeometrySource extends Source {
* background threads.
*
* @param zoomLevel Tile zoom level.
- * @param x Tile X coordinate.
- * @param y Tile Y coordinate.
- * @param data Feature collection for the tile.
+ * @param x Tile X coordinate.
+ * @param y Tile Y coordinate.
+ * @param data Feature collection for the tile.
*/
public void setTileData(int zoomLevel, int x, int y, FeatureCollection data) {
checkThread();
@@ -140,7 +147,15 @@ public class CustomGeometrySource extends Source {
TileID tileID = new TileID(z, x, y);
cancelledTileRequests.put(tileID, cancelFlag);
GeometryTileRequest request = new GeometryTileRequest(tileID, provider, this, cancelFlag);
- executor.execute(request);
+
+ executorLock.lock();
+ try {
+ if (executor != null && !executor.isShutdown()) {
+ executor.execute(request);
+ }
+ } finally {
+ executorLock.unlock();
+ }
}
@WorkerThread
@@ -152,6 +167,40 @@ public class CustomGeometrySource extends Source {
}
}
+ @Keep
+ private void startThreads() {
+ executorLock.lock();
+ try {
+ if (executor != null && !executor.isShutdown()) {
+ executor.shutdownNow();
+ }
+
+ executor = Executors.newFixedThreadPool(THREAD_POOL_LIMIT, new ThreadFactory() {
+ final AtomicInteger threadCount = new AtomicInteger();
+ final int poolId = poolCount.getAndIncrement();
+
+ @Override
+ public Thread newThread(@NonNull Runnable runnable) {
+ return new Thread(
+ runnable,
+ String.format(Locale.US, "%s-%d-%d", THREAD_PREFIX, poolId, threadCount.getAndIncrement()));
+ }
+ });
+ } finally {
+ executorLock.unlock();
+ }
+ }
+
+ @Keep
+ private void releaseThreads() {
+ executorLock.lock();
+ try {
+ executor.shutdownNow();
+ } finally {
+ executorLock.unlock();
+ }
+ }
+
private static class TileID {
public int z;
public int x;
@@ -164,7 +213,7 @@ public class CustomGeometrySource extends Source {
}
public int hashCode() {
- return Arrays.hashCode(new int[]{z, x, y});
+ return Arrays.hashCode(new int[] {z, x, y});
}
public boolean equals(Object object) {
@@ -177,7 +226,7 @@ public class CustomGeometrySource extends Source {
}
if (object instanceof TileID) {
- TileID other = (TileID)object;
+ TileID other = (TileID) object;
return this.z == other.z && this.x == other.x && this.y == other.y;
}
return false;
@@ -205,7 +254,7 @@ public class CustomGeometrySource extends Source {
FeatureCollection data = provider.getFeaturesForBounds(LatLngBounds.from(id.z, id.x, id.y), id.z);
CustomGeometrySource source = sourceRef.get();
- if (!isCancelled() && source != null && data != null) {
+ if (!isCancelled() && source != null && data != null) {
source.setTileData(id, data);
}
}
diff --git a/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/style/CustomGeometrySourceTest.kt b/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/style/CustomGeometrySourceTest.kt
new file mode 100644
index 0000000000..1496cb704d
--- /dev/null
+++ b/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/style/CustomGeometrySourceTest.kt
@@ -0,0 +1,68 @@
+package com.mapbox.mapboxsdk.testapp.style
+
+import android.support.test.espresso.Espresso.onView
+import android.support.test.espresso.matcher.ViewMatchers.isRoot
+import com.mapbox.mapboxsdk.style.sources.CustomGeometrySource
+import com.mapbox.mapboxsdk.style.sources.CustomGeometrySource.THREAD_POOL_LIMIT
+import com.mapbox.mapboxsdk.style.sources.CustomGeometrySource.THREAD_PREFIX
+import com.mapbox.mapboxsdk.testapp.action.MapboxMapAction.invoke
+import com.mapbox.mapboxsdk.testapp.action.OrientationChangeAction.orientationLandscape
+import com.mapbox.mapboxsdk.testapp.action.OrientationChangeAction.orientationPortrait
+import com.mapbox.mapboxsdk.testapp.activity.BaseActivityTest
+import com.mapbox.mapboxsdk.testapp.activity.style.GridSourceActivity
+import com.mapbox.mapboxsdk.testapp.activity.style.GridSourceActivity.ID_GRID_LAYER
+import com.mapbox.mapboxsdk.testapp.activity.style.GridSourceActivity.ID_GRID_SOURCE
+import org.junit.Assert
+import org.junit.Test
+
+class CustomGeometrySourceTest : BaseActivityTest() {
+
+ override fun getActivityClass(): Class<*> = GridSourceActivity::class.java
+
+ @Test
+ fun sourceNotLeakingThreadsTest() {
+ validateTestSetup()
+ waitAction(4000)
+ onView(isRoot()).perform(orientationLandscape())
+ waitAction(2000)
+ onView(isRoot()).perform(orientationPortrait())
+ waitAction(2000)
+ Assert.assertFalse("Threads should be shutdown when the source is destroyed.",
+ Thread.getAllStackTraces().keys.filter {
+ it.name.startsWith(THREAD_PREFIX)
+ }.count() > THREAD_POOL_LIMIT)
+ }
+
+ @Test
+ fun threadsShutdownWhenSourceRemovedTest() {
+ validateTestSetup()
+ invoke(mapboxMap) { uiController, mapboxMap ->
+ mapboxMap.removeLayer(ID_GRID_LAYER)
+ uiController.loopMainThreadForAtLeast(3000)
+ mapboxMap.removeSource(ID_GRID_SOURCE)
+ uiController.loopMainThreadForAtLeast(1000)
+ Assert.assertTrue("There should be no threads running when the source is removed.",
+ Thread.getAllStackTraces().keys.filter {
+ it.name.startsWith(CustomGeometrySource.THREAD_PREFIX)
+ }.count() == 0)
+ }
+ }
+
+ @Test
+ fun threadsRestartedWhenSourceReAddedTest() {
+ validateTestSetup()
+ invoke(mapboxMap) { uiController, mapboxMap ->
+ mapboxMap.removeLayer((rule.activity as GridSourceActivity).layer)
+ uiController.loopMainThreadForAtLeast(3000)
+ mapboxMap.removeSource(ID_GRID_SOURCE)
+ uiController.loopMainThreadForAtLeast(1000)
+ mapboxMap.addSource((rule.activity as GridSourceActivity).source)
+ mapboxMap.addLayer((rule.activity as GridSourceActivity).layer)
+ uiController.loopMainThreadForAtLeast(1000)
+ Assert.assertTrue("Threads should be restarted when the source is re-added to the map.",
+ Thread.getAllStackTraces().keys.filter {
+ it.name.startsWith(CustomGeometrySource.THREAD_PREFIX)
+ }.count() == CustomGeometrySource.THREAD_POOL_LIMIT)
+ }
+ }
+} \ No newline at end of file
diff --git a/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/style/GridSourceActivity.java b/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/style/GridSourceActivity.java
index fdc3826fb1..195aa63edd 100644
--- a/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/style/GridSourceActivity.java
+++ b/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/style/GridSourceActivity.java
@@ -30,12 +30,16 @@ import static com.mapbox.mapboxsdk.style.layers.PropertyFactory.lineColor;
*/
public class GridSourceActivity extends AppCompatActivity implements OnMapReadyCallback {
- private static final String ID_GRID_SOURCE = "grid_source";
- private static final String ID_GRID_LAYER = "grid_layer";
+ public static final String ID_GRID_SOURCE = "grid_source";
+ public static final String ID_GRID_LAYER = "grid_layer";
private MapView mapView;
private MapboxMap mapboxMap;
+ // public for testing purposes
+ public CustomGeometrySource source;
+ public LineLayer layer;
+
/**
* Implementation of GeometryTileProvider that returns features representing a zoom-dependent
* grid.
@@ -101,11 +105,11 @@ public class GridSourceActivity extends AppCompatActivity implements OnMapReadyC
mapboxMap = map;
// add source
- CustomGeometrySource source = new CustomGeometrySource(ID_GRID_SOURCE, new GridProvider());
+ source = new CustomGeometrySource(ID_GRID_SOURCE, new GridProvider());
mapboxMap.addSource(source);
// add layer
- LineLayer layer = new LineLayer(ID_GRID_LAYER, ID_GRID_SOURCE);
+ layer = new LineLayer(ID_GRID_LAYER, ID_GRID_SOURCE);
layer.setProperties(
lineColor(Color.parseColor("#000000"))
);
diff --git a/platform/android/src/native_map_view.cpp b/platform/android/src/native_map_view.cpp
index 44b04fc538..8da44c10cb 100755
--- a/platform/android/src/native_map_view.cpp
+++ b/platform/android/src/native_map_view.cpp
@@ -892,7 +892,9 @@ void NativeMapView::removeSource(JNIEnv& env, jni::Object<Source> obj, jlong sou
assert(sourcePtr != 0);
mbgl::android::Source *source = reinterpret_cast<mbgl::android::Source *>(sourcePtr);
- source->removeFromMap(env, obj, *map);
+ if (source->removeFromMap(env, obj, *map)) {
+ source->releaseJavaPeer();
+ }
}
void NativeMapView::addImage(JNIEnv& env, jni::String name, jni::Object<Bitmap> bitmap, jni::jfloat scale, jni::jboolean sdf) {
diff --git a/platform/android/src/style/sources/custom_geometry_source.cpp b/platform/android/src/style/sources/custom_geometry_source.cpp
index b38405a3b1..9c51f70ab5 100644
--- a/platform/android/src/style/sources/custom_geometry_source.cpp
+++ b/platform/android/src/style/sources/custom_geometry_source.cpp
@@ -54,7 +54,9 @@ namespace android {
: Source(env, coreSource, createJavaPeer(env), frontend) {
}
- CustomGeometrySource::~CustomGeometrySource() = default;
+ CustomGeometrySource::~CustomGeometrySource() {
+ releaseThreads();
+ }
void CustomGeometrySource::fetchTile (const mbgl::CanonicalTileID& tileID) {
android::UniqueEnv _env = android::AttachEnv();
@@ -78,6 +80,28 @@ namespace android {
peer.Call(*_env, cancelTile, (int)tileID.z, (int)tileID.x, (int)tileID.y);
};
+ void CustomGeometrySource::startThreads() {
+ android::UniqueEnv _env = android::AttachEnv();
+
+ static auto startThreads = javaClass.GetMethod<void ()>(*_env, "startThreads");
+
+ assert(javaPeer);
+
+ auto peer = jni::Cast(*_env, *javaPeer, javaClass);
+ peer.Call(*_env, startThreads);
+ }
+
+ void CustomGeometrySource::releaseThreads() {
+ android::UniqueEnv _env = android::AttachEnv();
+
+ static auto releaseThreads = javaClass.GetMethod<void ()>(*_env, "releaseThreads");
+
+ assert(javaPeer);
+
+ auto peer = jni::Cast(*_env, *javaPeer, javaClass);
+ peer.Call(*_env, releaseThreads);
+ };
+
void CustomGeometrySource::setTileData(jni::JNIEnv& env,
jni::jint z,
jni::jint x,
@@ -120,6 +144,19 @@ namespace android {
return jni::Object<Source>(CustomGeometrySource::javaClass.New(env, constructor, reinterpret_cast<jni::jlong>(this)).Get());
}
+ void CustomGeometrySource::addToMap(JNIEnv& env, jni::Object<Source> obj, mbgl::Map& map, AndroidRendererFrontend& frontend) {
+ Source::addToMap(env, obj, map, frontend);
+ startThreads();
+ }
+
+ bool CustomGeometrySource::removeFromMap(JNIEnv& env, jni::Object<Source> source, mbgl::Map& map) {
+ bool successfullyRemoved = Source::removeFromMap(env, source, map);
+ if (successfullyRemoved) {
+ releaseThreads();
+ }
+ return successfullyRemoved;
+ }
+
void CustomGeometrySource::registerNative(jni::JNIEnv& env) {
// Lookup the class
CustomGeometrySource::javaClass = *jni::Class<CustomGeometrySource>::Find(env).NewGlobalRef(env).release();
diff --git a/platform/android/src/style/sources/custom_geometry_source.hpp b/platform/android/src/style/sources/custom_geometry_source.hpp
index 1dc1c07b4f..c38926a5b9 100644
--- a/platform/android/src/style/sources/custom_geometry_source.hpp
+++ b/platform/android/src/style/sources/custom_geometry_source.hpp
@@ -28,8 +28,13 @@ public:
~CustomGeometrySource();
+ bool removeFromMap(JNIEnv&, jni::Object<Source>, mbgl::Map&) override;
+ void addToMap(JNIEnv&, jni::Object<Source>, mbgl::Map&, AndroidRendererFrontend&) override;
+
void fetchTile(const mbgl::CanonicalTileID& tileID);
void cancelTile(const mbgl::CanonicalTileID& tileID);
+ void startThreads();
+ void releaseThreads();
void setTileData(jni::JNIEnv& env, jni::jint z, jni::jint x, jni::jint y, jni::Object<geojson::FeatureCollection> jf);
void invalidateTile(jni::JNIEnv& env, jni::jint z, jni::jint x, jni::jint y);
diff --git a/platform/android/src/style/sources/source.cpp b/platform/android/src/style/sources/source.cpp
index 5f68e40467..8f14ebc43e 100644
--- a/platform/android/src/style/sources/source.cpp
+++ b/platform/android/src/style/sources/source.cpp
@@ -109,7 +109,7 @@ namespace android {
rendererFrontend = &frontend;
}
- void Source::removeFromMap(JNIEnv&, jni::Object<Source>, mbgl::Map& map) {
+ bool Source::removeFromMap(JNIEnv&, jni::Object<Source>, mbgl::Map& map) {
// Cannot remove if not attached yet
if (ownedSource) {
throw std::runtime_error("Cannot remove detached source");
@@ -119,6 +119,11 @@ namespace android {
ownedSource = map.getStyle().removeSource(source.getID());
// The source may not be removed if any layers still reference it
+ return ownedSource != nullptr;
+ }
+
+ void Source::releaseJavaPeer() {
+ // We can't release the peer if the source was not removed from the map
if (!ownedSource) {
return;
}
diff --git a/platform/android/src/style/sources/source.hpp b/platform/android/src/style/sources/source.hpp
index 718f60b381..6b906eb9c0 100644
--- a/platform/android/src/style/sources/source.hpp
+++ b/platform/android/src/style/sources/source.hpp
@@ -35,9 +35,11 @@ public:
virtual ~Source();
- void addToMap(JNIEnv&, jni::Object<Source>, mbgl::Map&, AndroidRendererFrontend&);
+ virtual void addToMap(JNIEnv&, jni::Object<Source>, mbgl::Map&, AndroidRendererFrontend&);
- void removeFromMap(JNIEnv&, jni::Object<Source>, mbgl::Map&);
+ virtual bool removeFromMap(JNIEnv&, jni::Object<Source>, mbgl::Map&);
+
+ void releaseJavaPeer();
jni::String getId(jni::JNIEnv&);