Lifecycle coroutine scope leak fixes (#141)

* - automatically cancel LifecycleCoroutineScope when lifecycle reaches DESTROYED (fixes #135)
- automatically remove lifecycleScope extension property from cache when lifecycle reaches DESTROYED (fixes #136)

* dispatch-internal-test-android module

* update lifecycle handling

* update lifecycle handling

* update lifecycle handling

* convert LifecycleScopeExtensionTest from Kotest to JUnit5 to fix some weird recursive behavior

* convert LifecycleScopeExtensionTest from Kotest to JUnit5 to fix some weird recursive behavior

* remove workspace.xml backup which shouldn't have been added to git

buildSrc/src/main/kotlin/Dependencies.kt

@@ -161,7 +161,7 @@ object Libs {
   }
 
   object Kotest {
-    private const val version = "4.1.0"
+    private const val version = "4.1.1"
     const val assertions = "io.kotest:kotest-assertions-core-jvm:$version"
     const val consoleRunner = "io.kotest:kotest-runner-console-jvm:$version"
     const val properties = "io.kotest:kotest-property-jvm:$version"

dispatch-android-lifecycle-extensions/src/main/java/dispatch/android/lifecycle/LifecycleScopeFactory.kt

@@ -15,6 +15,7 @@
 
 package dispatch.android.lifecycle
 
+import androidx.lifecycle.*
 import dispatch.android.lifecycle.LifecycleScopeFactory.reset
 import dispatch.core.*
 
@@ -47,7 +48,7 @@ public object LifecycleScopeFactory {
     _factory = factory
   }
 
-  internal fun create() = _factory.invoke()
+  internal fun create(lifecycle: Lifecycle) = LifecycleCoroutineScope(lifecycle, _factory.invoke())
 
   /**
    * Immediately resets the factory function to its default.

dispatch-android-lifecycle-extensions/src/main/java/dispatch/android/lifecycle/internal/LifecycleCoroutineScopeStore.kt

@@ -19,62 +19,61 @@ import android.os.*
 import androidx.lifecycle.*
 import dispatch.android.lifecycle.*
 import dispatch.core.*
-import kotlinx.coroutines.*
-import java.util.*
 import java.util.concurrent.*
-import kotlin.collections.set
 
-internal object LifecycleCoroutineScopeStore {
+@Suppress("MagicNumber")
+internal object LifecycleCoroutineScopeStore : LifecycleEventObserver {
 
   // ConcurrentHashMap can miss "put___" operations on API 21/22 https://issuetracker.google.com/issues/37042460
-  @Suppress("MagicNumber")
-  private val map = if (Build.VERSION.SDK_INT < 23) {
-    Collections.synchronizedMap<Lifecycle, LifecycleCoroutineScope>(mutableMapOf<Lifecycle, LifecycleCoroutineScope>())
-  } else {
-    ConcurrentHashMap<Lifecycle, LifecycleCoroutineScope>()
-  }
-
-  fun get(lifecycle: Lifecycle): LifecycleCoroutineScope {
+  private val map: MutableMap<Lifecycle, LifecycleCoroutineScope> =
+    if (Build.VERSION.SDK_INT < 23) {
+      mutableMapOf()
+    } else {
+      ConcurrentHashMap()
+    }
 
-    return map[lifecycle] ?: bindLifecycle(lifecycle, LifecycleScopeFactory.create())
+  override fun onStateChanged(source: LifecycleOwner, event: Lifecycle.Event) {
+    if (source.lifecycle.currentState <= Lifecycle.State.DESTROYED) {
+      map.remove(source.lifecycle)
+    }
   }
 
-  private fun bindLifecycle(
-    lifecycle: Lifecycle, coroutineScope: MainImmediateCoroutineScope
-  ): LifecycleCoroutineScope {
-
-    val scope = LifecycleCoroutineScope(lifecycle, coroutineScope)
-
-    map[lifecycle] = scope
+  fun get(lifecycle: Lifecycle): LifecycleCoroutineScope {
 
-    if (!lifecycle.currentState.isAtLeast(Lifecycle.State.INITIALIZED)) {
-      scope.coroutineContext.cancel()
-      return scope
+    if (lifecycle.currentState <= Lifecycle.State.DESTROYED) {
+      return LifecycleScopeFactory.create(lifecycle)
     }
 
-    val observer = object : LifecycleEventObserver {
-
-      override fun onStateChanged(source: LifecycleOwner, event: Lifecycle.Event) {
-
-        if (!lifecycle.currentState.isAtLeast(Lifecycle.State.INITIALIZED)) {
-
-          scope.coroutineContext.cancel()
-          lifecycle.removeObserver(this)
+    return when {
+      Build.VERSION.SDK_INT >= 24 -> {
+        map.computeIfAbsent(lifecycle) { bindLifecycle(lifecycle) }
+      }
+      Build.VERSION.SDK_INT == 23 -> {
+        /*
+        `getOrPut` by itself isn't atomic.  It is guaranteed to only ever return one instance
+         for a given key, but the lambda argument may be invoked unnecessarily.
+         */
+        (map as ConcurrentMap).atomicGetOrPut(lifecycle) { bindLifecycle(lifecycle) }
+      }
+      else                        -> {
+        synchronized(map) {
+          map.getOrPut(lifecycle) { bindLifecycle(lifecycle) }
         }
       }
     }
+  }
 
-    scope.launchMainImmediate {
+  private fun bindLifecycle(lifecycle: Lifecycle): LifecycleCoroutineScope {
+
+    val scope = LifecycleScopeFactory.create(lifecycle)
 
-      if (lifecycle.currentState.isAtLeast(Lifecycle.State.INITIALIZED)) {
-        lifecycle.addObserver(observer)
-      } else {
-        scope.coroutineContext.cancel()
+    scope.launchMainImmediate {
+      if (lifecycle.currentState >= Lifecycle.State.INITIALIZED) {
+        lifecycle.addObserver(this@LifecycleCoroutineScopeStore)
       }
     }
 
     return scope
   }
 
 }
-

dispatch-android-lifecycle-extensions/src/main/java/dispatch/android/lifecycle/internal/atomic.kt

@@ -0,0 +1,60 @@
+/*
+ * Copyright (C) 2020 Rick Busarow
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package dispatch.android.lifecycle.internal
+
+import androidx.lifecycle.*
+import dispatch.android.lifecycle.*
+import dispatch.core.*
+import kotlinx.coroutines.*
+import java.util.concurrent.*
+
+/**
+ * Normal [getOrPut] is guaranteed to only return a single instance for a given key,
+ * but the [defaultValue] lambda may be invoked unnecessarily.  This would create a new instance
+ * of [LifecycleCoroutineScope] without actually returning it.
+ *
+ * This extra [LifecycleCoroutineScope] would still be active and observing the [lifecycle][key].
+ *
+ * This function compares the result of the lambda to the result of [getOrPut],
+ * and cancels the lambda's result if it's different.
+ *
+ * @see getOrPut
+ */
+internal inline fun ConcurrentMap<Lifecycle, LifecycleCoroutineScope>.atomicGetOrPut(
+  key: Lifecycle,
+  defaultValue: () -> LifecycleCoroutineScope
+): LifecycleCoroutineScope {
+
+  var newInstance: LifecycleCoroutineScope? = null
+
+  val existingOrNew = getOrPut(key) {
+    newInstance = defaultValue.invoke()
+    newInstance!!
+  }
+
+  // If the second value is different, that means the first was never inserted into the map.
+  // Cancel the observer for `newInstance` and cancel the coroutineContext.
+  newInstance?.let { new ->
+    if (new != existingOrNew) {
+      existingOrNew.launchMainImmediate {
+
+        new.coroutineContext[Job]?.cancel()
+      }
+    }
+  }
+
+  return existingOrNew
+}

dispatch-android-lifecycle-extensions/src/main/java/dispatch/android/lifecycle/LifecycleScope.kt → dispatch-android-lifecycle-extensions/src/main/java/dispatch/android/lifecycle/lifecycleScope.kt

@@ -28,7 +28,8 @@ import kotlinx.coroutines.*
  *
  * The type of [CoroutineScope] created is configurable via [LifecycleScopeFactory.set].
  *
- * The `viewModelScope` is automatically cancelled when the [LifecycleOwner]'s [lifecycle][LifecycleOwner.getLifecycle]'s [Lifecycle.State] drops to [Lifecycle.State.DESTROYED].
+ * The `viewModelScope` is automatically cancelled when the [LifecycleOwner]'s
+ * [lifecycle][LifecycleOwner.getLifecycle]'s [Lifecycle.State] drops to [Lifecycle.State.DESTROYED].
  *
  * @sample samples.LifecycleCoroutineScopeSample.lifecycleCoroutineScopeSample
  */