Shutting down thread pool of the CustomGeometrySource when the source is destroyed

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/style/sources/CustomGeometrySource.java

@@ -15,26 +15,34 @@
 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<>();
 
   /**
    * 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.
    * <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 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);
       }
     }

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)
+    }
+  }
+}

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"))
     );

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) {

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();

platform/android/src/style/sources/custom_geometry_source.hpp

@@ -28,8 +28,13 @@ class CustomGeometrySource : public Source {
 
     ~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);

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;
         }

platform/android/src/style/sources/source.hpp

@@ -35,9 +35,11 @@ class Source : private mbgl::util::noncopyable {
 
     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&);