Skip to content

Commit 4c6a625

Browse files
roookeeeHeller, Nico [CBC]
and
Heller, Nico [CBC]
authored
Rewrite CompletableFutureExtension.allSettled to not use CompletableFuture.supplyAsync (#1872)
We noticed a big drop in performance after switching to the new `CompletableFuture` based `FederatedTypeResolver`. Our investigations led us to the implementation of `CompletableFutureExtension.allSettled` which has the following issues: - Bug: It uses `CompletableFuture.supplyAsync` which looses all thread local data (as we switch to another thread pool we have no control over), which breaks most tracing frameworks and all other thread local usages - Performance: 100 futures get wrapped into 100 separately scheduled futures, this is unnecessary as `thenApply` and `exceptionally` work without nesting of futures. This avoids non-neglegible overhead I changed the accompanying test to expect another exception type as only `get()` throw a `ExecutionException`. This should not affect your public API as its an internal method and I could not find any other references to `ExecutionException` in `graphql-kotlin`s codebase. Co-authored-by: Heller, Nico [CBC] <[email protected]>
1 parent 3ec66c1 commit 4c6a625

File tree

2 files changed

+4
-14
lines changed

2 files changed

+4
-14
lines changed

generator/graphql-kotlin-federation/src/main/kotlin/com/expediagroup/graphql/generator/federation/extensions/CompletableFutureExtensions.kt

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -42,18 +42,9 @@ internal fun <T : Any?> List<CompletableFuture<out T>>.joinAll(): CompletableFut
4242
@Suppress("TooGenericExceptionCaught")
4343
internal fun <T : Any?> List<CompletableFuture<out T>>.allSettled(): CompletableFuture<List<Result<T>>> {
4444
val resultFutures = map { future ->
45-
CompletableFuture.supplyAsync {
46-
try {
47-
Result.success(future.get())
48-
} catch (e: Exception) {
49-
Result.failure<T>(e)
50-
}
51-
}
45+
future.thenApply { result -> Result.success(result) }
46+
.exceptionally { exception -> Result.failure<T>(exception) }
5247
}
5348

54-
return CompletableFuture.allOf(
55-
*resultFutures.toTypedArray()
56-
).thenApply {
57-
resultFutures.map(CompletableFuture<Result<T>>::join)
58-
}
49+
return resultFutures.joinAll()
5950
}

generator/graphql-kotlin-federation/src/test/kotlin/com/expediagroup/graphql/generator/federation/extensions/CompletableFutureExtensionsKtTest.kt

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@ import reactor.kotlin.core.publisher.toMono
2727
import java.time.Duration
2828
import java.util.concurrent.CompletableFuture
2929
import java.util.concurrent.CompletionException
30-
import java.util.concurrent.ExecutionException
3130
import kotlin.test.assertEquals
3231
import kotlin.test.assertIs
3332
import kotlin.test.assertTrue
@@ -101,7 +100,7 @@ class CompletableFutureExtensionsKtTest {
101100
assertEquals("first promise", result[0].getOrNull())
102101
assertEquals("second promise", result[1].getOrNull())
103102
assertTrue(result[2].isFailure)
104-
assertIs<ExecutionException>(result[2].exceptionOrNull())
103+
assertIs<CompletionException>(result[2].exceptionOrNull())
105104
assertEquals("async exception", result[2].exceptionOrNull()?.cause?.message)
106105
}
107106
}

0 commit comments

Comments
 (0)