From 41f3c330c30b58fd20106bf81a71b0667bb887c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Paczos?= Date: Tue, 31 Jul 2018 19:42:09 +0200 Subject: [PATCH] [android] shutting down thread pool of the CustomGeometrySource when the source is destroyed --- .../style/sources/CustomGeometrySource.java | 83 +++++++++++++++---- .../testapp/style/CustomGeometrySourceTest.kt | 68 +++++++++++++++ .../activity/style/GridSourceActivity.java | 12 ++- platform/android/src/native_map_view.cpp | 4 +- .../style/sources/custom_geometry_source.cpp | 39 ++++++++- .../style/sources/custom_geometry_source.hpp | 5 ++ platform/android/src/style/sources/source.cpp | 7 +- platform/android/src/style/sources/source.hpp | 6 +- 8 files changed, 198 insertions(+), 26 deletions(-) create mode 100644 platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/style/CustomGeometrySourceTest.kt 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 1f6029e2a2c..6c0b76f00ef 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.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 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 CustomGeometrySource(String id, GeometryTileProvider provider) { * Create a CustomGeometrySource with non-default CustomGeometrySourceOptions. *

Supported options are minZoom, maxZoom, buffer, and tolerance.

* - * @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 void invalidateRegion(LatLngBounds bounds) { * 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 void invalidateTile(int zoomLevel, int x, int y) { * 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 @@ private void fetchTile(int z, int x, int y) { 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 @@ private void cancelTile(int z, int x, int y) { } } + @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 TileID(int _z, int _x, int _y) { } 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 boolean equals(Object object) { } 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 void run() { 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 00000000000..1496cb704d1 --- /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 fdc3826fb11..195aa63edde 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 @@ */ 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 void onMapReady(@NonNull final MapboxMap map) { 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 44b04fc5380..8da44c10cb4 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 obj, jlong sou assert(sourcePtr != 0); mbgl::android::Source *source = reinterpret_cast(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, 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 b38405a3b16..9c51f70ab56 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(*_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(*_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(CustomGeometrySource::javaClass.New(env, constructor, reinterpret_cast(this)).Get()); } + void CustomGeometrySource::addToMap(JNIEnv& env, jni::Object obj, mbgl::Map& map, AndroidRendererFrontend& frontend) { + Source::addToMap(env, obj, map, frontend); + startThreads(); + } + + bool CustomGeometrySource::removeFromMap(JNIEnv& env, jni::Object 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::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 1dc1c07b4fa..c38926a5b90 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 @@ class CustomGeometrySource : public Source { ~CustomGeometrySource(); + bool removeFromMap(JNIEnv&, jni::Object, mbgl::Map&) override; + void addToMap(JNIEnv&, jni::Object, 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 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 413530a5ec3..3c26c39b1fa 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, mbgl::Map& map) { + bool Source::removeFromMap(JNIEnv&, jni::Object, 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 718f60b3810..6b906eb9c0b 100644 --- a/platform/android/src/style/sources/source.hpp +++ b/platform/android/src/style/sources/source.hpp @@ -35,9 +35,11 @@ class Source : private mbgl::util::noncopyable { virtual ~Source(); - void addToMap(JNIEnv&, jni::Object, mbgl::Map&, AndroidRendererFrontend&); + virtual void addToMap(JNIEnv&, jni::Object, mbgl::Map&, AndroidRendererFrontend&); - void removeFromMap(JNIEnv&, jni::Object, mbgl::Map&); + virtual bool removeFromMap(JNIEnv&, jni::Object, mbgl::Map&); + + void releaseJavaPeer(); jni::String getId(jni::JNIEnv&);