Fix compute with null #45

Author: sksamuel

aedile-core/src/main/kotlin/com/sksamuel/aedile/core/LoadingCache.kt

@@ -12,8 +12,6 @@ import java.util.concurrent.CompletableFuture
 import kotlin.coroutines.coroutineContext
 
 class LoadingCache<K : Any, V>(
-   private val defaultScope: CoroutineScope,
-   private val useCallingContext: Boolean,
    private val cache: AsyncLoadingCache<K, V>
 ) {
 
@@ -68,7 +66,7 @@ class LoadingCache<K : Any, V>(
     * See full docs at [AsyncCache.getAll].
     */
    suspend fun getAll(keys: Collection<K>, compute: suspend (Set<K>) -> Map<K, V>): Map<K, V> {
-      val scope = scope()
+      val scope = CoroutineScope(coroutineContext)
       return cache.getAll(keys) { k, _ -> scope.async { compute(k.toSet()) }.asCompletableFuture() }.await()
    }
 
@@ -83,10 +81,29 @@ class LoadingCache<K : Any, V>(
     * and enters it into this cache unless null.
     *
     * If the suspendable computation throws, the entry will be automatically removed from this cache.
+    *
+    * See full docs at [AsyncLoadingCache.get].
     */
    suspend fun get(key: K, compute: suspend (K) -> V): V {
-      val scope = scope()
-      return cache.get(key) { k, _ -> scope.async { compute(k) }.asCompletableFuture() }.await()
+
+      val scope = CoroutineScope(coroutineContext)
+      var error: Throwable? = null
+      val value = cache.get(key) { k, _ ->
+         val asCompletableFuture = scope.async {
+            // if compute throws, then it will cause the parent coroutine to be cancelled as well
+            // we don't want that, as want to throw the exception back to the caller.
+            // so we must capture it and throw it manually
+            try {
+               compute(k)
+            } catch (e: Throwable) {
+               error = e
+               null
+            }
+         }.asCompletableFuture()
+         asCompletableFuture.thenApply { it ?: throw error ?: NullPointerException() }
+      }.await()
+      error?.let { throw it }
+      return value
    }
 
    /**
@@ -102,7 +119,7 @@ class LoadingCache<K : Any, V>(
     * @return the present value, the computed value, or throws.
     */
    suspend fun getOrNull(key: K, compute: suspend (K) -> V?): V? {
-      val scope = scope()
+      val scope = CoroutineScope(coroutineContext)
       return cache.get(key) { k, _ -> scope.async { compute(k) }.asCompletableFuture() }.await()
    }
 
@@ -118,10 +135,10 @@ class LoadingCache<K : Any, V>(
     * If the suspendable computation throws, the entry will be automatically removed.
     *
     * @param key the key to associate the computed value with
-    * @param compute the suspendable function that generate the value.
+    * @param compute the suspendable function that generates the value.
     */
    suspend fun put(key: K, compute: suspend () -> V) {
-      val scope = scope()
+      val scope = CoroutineScope(coroutineContext)
       cache.put(key, scope.async { compute() }.asCompletableFuture())
    }
 
@@ -194,8 +211,4 @@ class LoadingCache<K : Any, V>(
    suspend fun refreshAll(keys: Collection<K>): Map<K, V> {
       return cache.synchronous().refreshAll(keys).await()
    }
-
-   private suspend fun scope(): CoroutineScope {
-      return if (useCallingContext) CoroutineScope(coroutineContext) else defaultScope
-   }
 }

aedile-core/src/main/kotlin/com/sksamuel/aedile/core/builders.kt

@@ -20,23 +20,7 @@ import java.util.concurrent.Executor
  * If the suspendable computation throws or computes a null value, then the
  * entry will be automatically removed.
  */
-fun <K : Any, V : Any> Caffeine<in K, in V>.asCache(): Cache<K, V> {
-   val scope = CoroutineScope(Dispatchers.IO + CoroutineName("Aedile-AsyncLoadingCache-Scope") + SupervisorJob())
-   return asCache(scope)
-}
-
-/**
- * Returns a [Cache] which suspends when requesting values.
- *
- * If the key is not present in the cache, it returns null, unless a compute function
- * is provided with the key.
- *
- * If the suspendable computation throws or computes a null value, then the
- * entry will be automatically removed.
- */
-fun <K : Any, V : Any> Caffeine<in K, in V>.asCache(scope: CoroutineScope): Cache<K, V> {
-   return Cache(buildAsync())
-}
+fun <K : Any, V : Any> Caffeine<in K, in V>.asCache(): Cache<K, V> = Cache(buildAsync())
 
 /**
  * Returns a [LoadingCache] which uses the provided [compute] function
@@ -54,16 +38,16 @@ fun <K : Any, V : Any> Caffeine<in K, in V>.asLoadingCache(compute: suspend (K)
  * Returns a [LoadingCache] which uses the provided [compute] function
  * to compute a value, unless a specific compute has been provided with the key.
  *
- * The compute function will execute on the given [scope].
+ * The [compute] function will execute on the given [scope].
  *
- * If the suspendable computation throws or computes a null value then the
+ * If the suspendable computation throws or computes a null value, then the
  * entry will be automatically removed.
  */
 fun <K : Any, V : Any> Caffeine<in K, in V>.asLoadingCache(
    scope: CoroutineScope,
    compute: suspend (K) -> V
 ): LoadingCache<K, V> {
-   return LoadingCache(scope, true, buildAsync { key, _ -> scope.async { compute(key) }.asCompletableFuture() })
+   return LoadingCache(buildAsync { key, _ -> scope.async { compute(key) }.asCompletableFuture() })
 }
 
 /**
@@ -92,20 +76,20 @@ fun <K : Any, V : Any> Caffeine<in K, in V>.asLoadingCache(
  * If the key does not exist, then the suspendable [compute] function is invoked
  * to compute a value, unless a specific compute has been provided with the key.
  *
- * If the suspendable computation throws or computes a null value then the
+ * If the suspendable computation throws or computes a null value, then the
  * entry will be automatically removed.
  *
  * The [reloadCompute] function is invoked to refresh an entry if refreshAfterWrite
  * is enabled or refresh is invoked. See full docs [AsyncCacheLoader.asyncReload].
  *
- * The compute functions will execute on the given [scope].
+ * The given compute functions will execute on the given [scope].
  */
 fun <K : Any, V : Any> Caffeine<in K, in V>.asLoadingCache(
    scope: CoroutineScope,
    compute: suspend (K) -> V,
    reloadCompute: suspend (K, V) -> V,
 ): LoadingCache<K, V> {
-   return LoadingCache(scope, true, buildAsync(object : AsyncCacheLoader<K, V> {
+   return LoadingCache(buildAsync(object : AsyncCacheLoader<K, V> {
       override fun asyncLoad(key: K, executor: Executor): CompletableFuture<out V> {
          return scope.async { compute(key) }.asCompletableFuture()
       }
@@ -122,7 +106,7 @@ fun <K : Any, V : Any> Caffeine<in K, in V>.asLoadingCache(
  * If a requested key does not exist, then the suspendable [compute] function is invoked
  * to compute the required values.
  *
- * If the suspendable computation throws or computes a null value then the
+ * If the suspendable computation throws or computes a null value, then the
  * entry will be automatically removed.
  */
 fun <K : Any, V : Any> Caffeine<in K, in V>.asBulkLoadingCache(compute: suspend (Set<K>) -> Map<K, V>): LoadingCache<K, V> {
@@ -145,7 +129,7 @@ fun <K : Any, V : Any> Caffeine<in K, in V>.asBulkLoadingCache(
    scope: CoroutineScope,
    compute: suspend (Set<K>) -> Map<K, V>
 ): LoadingCache<K, V> {
-   return LoadingCache(scope, true, buildAsync(AsyncCacheLoader.bulk { keys, _ ->
+   return LoadingCache(buildAsync(AsyncCacheLoader.bulk { keys, _ ->
       scope.async { compute(keys) }.asCompletableFuture()
    }))
 }

aedile-core/src/test/kotlin/com/sksamuel/aedile/core/AsLoadingCacheTest.kt

@@ -106,8 +106,15 @@ class AsLoadingCacheTest : FunSpec() {
             keys.associateWith { "$it-value" }
          }
          shouldThrow<IllegalStateException> {
-            supervisorScope {
-               cache.get("foo") { error("kapow") }
+            cache.get("foo") { error("kapow") }
+         }
+      }
+
+      test("get should propagate exceptions in the compute function") {
+         val cache = Caffeine.newBuilder().asCache<String, String>()
+         shouldThrow<IllegalStateException> {
+            cache.get("foo") {
+               error("kapow")
             }
          }
       }