ExpediaGroup / graphql-kotlin

Libraries for running GraphQL in Kotlin
https://opensource.expediagroup.com/graphql-kotlin/
Apache License 2.0
1.74k stars 351 forks source link

Can not call `kotlinx.coroutines.future.await` on output from DataFetchingEnvironment::getValueFromDataLoader after updating to 7.1.x #2017

Open terminalnode opened 4 months ago

terminalnode commented 4 months ago

Library Version Tried on 7.1.1 and 7.1.4. Worked previously on 7.0.2.

Describe the bug kotlinx.coroutines.core has a function kotlinx.coroutines.future.await for converting CompletableFutures to coroutines. In previous versions of this library, calling await() on the output of DataFetchingEnvironment::getValueFromDataLoader was something we used extensively in our code, but now it causes all requests doing this to timeout.

To Reproduce I found one part in our code where I can easily fix/break it with only this await differing between the two.

This is the working variant, which is not using await()

suspend fun stuff(
    env: DataFetchingEnvironment,
    pagination: PaginationOptionsInputGql? = null,
    filter: FilterInput? = null,
): CompletableFuture<MyConnectionGql> = env.getValueFromDataLoader<MyKey, MyConnectionGql>(
    ActivityByMyKeyDataLoader::class.simpleName!!,
    MyKey(pagination, filter),
)

This is the broken variant which causes timeouts:

import kotlinx.coroutines.future.await

suspend fun stuff(
    env: DataFetchingEnvironment,
    pagination: PaginationOptionsInputGql? = null,
    filter: FilterInput? = null,
): MyConnectionGql = env.getValueFromDataLoader<MyKey, MyConnectionGql>(
    ActivityByMyKeyDataLoader::class.simpleName!!,
    MyKey(pagination, filter),
).await()

The only difference here is removal of CompletableFuture in the return type and addition of .await()

Expected behavior The two approaches should have the same effect.

samuelAndalon commented 4 months ago

please provide a repo link/zip file with repro steps. Out of curiosity for your "working variant" why would you mark your function as suspend if you are not executing any suspendable function or launching a coroutine inside ?

terminalnode commented 4 months ago

The working variant only has the suspend marker to keep the breaking changes as small as possible. It's not required at all here, but highlights that only adding the .await() is enough to break it.

As for repo to reproduce, I'll see what I can do.