Fix suspend resolvers hanging when awaiting a data loader - #831
Open
oryan-block wants to merge 1 commit into
Open
oryan-block wants to merge 1 commit into
oryan-block wants to merge 1 commit into
Conversation
Suspend resolver methods were started with the default coroutine start, which dispatches the body to the configured dispatcher and returns the future straight away. graphql-java dispatches the DataLoaders of a level once the data fetchers of that level have returned, so a resolver that awaited DataLoader.load or loadMany often registered its keys only after that dispatch had happened and then waited forever for a batch that never ran. Start the coroutine with CoroutineStart.UNDISPATCHED so the resolver runs on the calling thread until its first suspension. Loads are then queued before the data fetcher returns, as graphql-java expects, and the rest of the coroutine still resumes on the configured context. An undispatched coroutine runs even if its context is already cancelled, so check for that first to keep cancelled resolvers from being invoked. Loads issued after the resolver has suspended, such as a second load that depends on the first, still rely on graphql-java's data loader chaining or exhausted dispatching options, as with plain fetchers. Fixes #419 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Fixes #419
Checklist
Description
A suspend resolver that calls
DataLoader#loadorloadManyand then awaits the result hangs forever. This still happens on master (graphql-java 26.1) with default options, for root and nested fields.MethodFieldResolverDataFetcher#getstarted suspend resolvers with the default coroutine start, which hands the body to the configured dispatcher (Dispatchers.Defaultunless configured otherwise) and returns the future straight away. graphql-java dispatches a level's DataLoaders once that level's data fetchers have returned, so the load usually got queued after the dispatch had already run, and theawaitwaited for a batch that never ran. It's basically thesupplyAsyncexample the graphql-java batching docs tell you not to do, as someone pointed out on the issue.The coroutine now starts with
CoroutineStart.UNDISPATCHED, so the resolver runs on the calling thread until its first suspension. Loads are queued before the data fetcher returns and the rest of the coroutine still resumes on the configured context. An undispatched coroutine runs its body even if its context is already cancelled, which a default start never does, so the block callsensureActive()first. That way a resolver still isn't invoked when, for example, theJobpassed throughSchemaParserOptions.coroutineContextis cancelled. Callingdispatch()by hand, like the workaround on the issue does, shouldn't be needed anymore for this case.SuspendFunctionDataLoaderTestcovers a root suspend field awaitingloadManyand a nested suspend field awaitingload. Each runs 20 executions with a 5s timeout so a hang fails the test instead of blocking the build. Both time out on master. There's also a new test inMethodFieldResolverDataFetcherTestfor the cancelled context.This only covers loads issued before the resolver first suspends. A load issued after a real suspension, like a second load that depends on the first or a load after
withContext(Dispatchers.IO), still hangs with default options, on master and with this change. A plain fetcher that chainsCompletableFutureloads hangs the same way, so I didn't try to work around it here. For those, graphql-java'sDataLoaderDispatchingContextKeys.ENABLE_DATA_LOADER_CHAININGorENABLE_DATA_LOADER_EXHAUSTED_DISPATCHINGwork together with this change. Chaining only helps loaders that come fromenv.getDataLoader, not ones held on a custom context object.Dispatchers.Unconfinedisn't a full workaround either. It still hangs on master for suspend fields nested under another suspend field. Subscriptions are left alone.Behaviour change: code in a suspend resolver up to its first suspension, including the resolver method call itself, now runs on the graphql-java calling thread instead of the dispatcher from
SchemaParserOptions.coroutineContext/coroutineContextProvider. After the first suspension it resumes on the configured context as before, and the coroutine context itself is unchanged. So a suspend function that does blocking work without ever suspending (e.g. blocking JDBC with nowithContext) now blocks the calling thread, same as a non-suspend resolver. Wrapping it inwithContext(Dispatchers.IO)moves it off again. Exceptions thrown before the first suspension still fail the field the same way as before.🤖 Generated with Claude Code