Backport #9984: run resolver cleanup before scheduler completion (15.x) - #10364
Open
erf1nd0r wants to merge 1 commit into
Open
Backport #9984: run resolver cleanup before scheduler completion (15.x)#10364erf1nd0r wants to merge 1 commit into
erf1nd0r wants to merge 1 commit into
Conversation
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.
Backport of #9984 to
main-version-15. Fixes #9971 for the 15.1.x line.On 15.1.17 and 15.1.18,
ResolverTask.ExecuteAsyncsignalsScheduler.Complete(this)before running the resolver's cleanup tasks. When the resolver was the last running task, the scheduler completes the request and returns theOperationContextto the pool while cleanup is still pending. Any cleanup that touches the operation context, such as theCompleteUnsafeAsyncregistered byIResolverContext.Clone(), then throwsObjectDisposedException("The specified object was not initialized and is no longer usable"). The faulted task is never awaited, so it surfaces as anUnobservedTaskExceptionon the finalizer thread.The fix on
mainreorders thefinallyblock so cleanup runs first andScheduler.Completeplus the pool return happen in a nestedfinally. This is a cherry-pick of 6e5b640 with no API changes. The only adjustment is in the test: this branch uses xunit v2, soTestContext.Current.CancellationTokenwas replaced withCancellationToken.None.Verified locally on this branch (net9.0): the backported test
ResolveAsync_Should_NotFaultClonedContext_When_EntitiesResolvedConcurrentlyfails within ~650 ms on the unpatchedmain-version-15source with a list ofObjectDisposedExceptions, and passes with the fix applied.We hit this in production on 15.1.17 through the Relay
node(id:)field, which clones the resolver context inNodeFieldResolvers. Any@refetchableRelay fragment on aNodetype triggers it, so it is not limited to Apollo Federation_entitiesas described in #9971. Responses stay correct, but the exception is logged on every occurrence. Upgrading to 16.x is not an option for us yet because a shared internal package pins HotChocolate to[15.1.14, 16.0.0).