graphql-kotlin icon indicating copy to clipboard operation
graphql-kotlin copied to clipboard

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

Open terminalnode opened this issue 1 year ago • 2 comments

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.

terminalnode avatar Jul 15 '24 15:07 terminalnode

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 ?

samuelAndalon avatar Jul 15 '24 17:07 samuelAndalon

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.

terminalnode avatar Jul 16 '24 07:07 terminalnode

As I'm not working with the affected project or graphql-kotlin anymore I will close this issue.

terminalnode avatar Dec 01 '24 10:12 terminalnode