AsyncDataloader: when Fibers share a connection, only load connection nodes once - #5708
Open
drhops wants to merge 1 commit into
Open
AsyncDataloader: when Fibers share a connection, only load connection nodes once#5708drhops wants to merge 1 commit into
drhops wants to merge 1 commit into
Conversation
`AsyncDataloader` resolves sibling fields of one connection (`edges` and
`pageInfo`, for instance) as separate jobs, each in its own Fiber. Both
reach `RelationConnection#load_nodes`, where `@nodes ||= limited_nodes.to_a`
is not atomic across Fibers, so both can call `.to_a` on the same memoized
`limited_nodes` relation.
That runs the page query twice. With ActiveRecord it can also raise
`ActiveRecord::UnmodifiableRelation` in the slower Fiber: loading a
Relation marks it immutable, and building Arel for it can write back to
the Relation (a dotted `group("users.id")`, say, appends to
`references_values`), which then trips `assert_modifiable!`.
Guard the load with a Fiber-aware `Mutex`, behind a memoized fast path so
the lock is only taken until the first load completes.
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.
Problem
Under
AsyncDataloader, sibling fields of one connection (edges,pageInfo, …) are separate jobs, each in its own Fiber, and all of them callRelationConnection#load_nodeson the same connection object.@nodes ||= limited_nodes.to_ais not atomic across Fibers. The first caller suspends insideto_awhile the query is in flight,@nodesis still nil, so every other caller that arrives during the query loads the same relation again.This duplicates the page query runs once per Fiber. With ActiveRecord it can also raise
ActiveRecord::UnmodifiableRelation: the first load marks the Relation@loaded, and a second Fiber still building Arel for it then tripsassert_modifiable!(build_arelwritesreferences_valuesback to the Relation whengroup/selectuse a dotted column name).Production stacktrace
Puma, Ruby 3.4.9, Rails 8.1,
graphql2.6.8,async2.36.0,config.active_support.isolation_level = :fiber. The document selectededges,pageInfo { hasNextPage endCursor }andtotalCounton a connection overUser.joins(...).group("users.id").edgesloaded the relation first; thepageInfo.endCursorFiber was already insidebuild_arel:It is rare because the second Fiber has to suspend inside
build_arel— in practice a cold schema cache (columns_hashhitting the DB). The duplicate query happens every time.Minimal repro
No GraphQL execution needed — two Fibers and one connection are the whole trigger:
Root cause
load_nodesdoes a read (@nodes), an IO-bound call (to_a) and a write, with a suspension point in the middle, on alimited_nodesobject that is itself memoized and therefore shared by every caller.ArrayConnection#load_nodeshas the same shape but is a pure array slice with no suspension point, so racing it is harmless;RelationConnectionis the one that does IO on shared state.Fix
Guard the load with a
Mutex, behind the existing memo as a fast path:Mutexis Fiber-aware: a waiting Fiber yields to the scheduler rather than blocking the thread, and wakes to find@nodesfilled. It lives inPagination::RelationConnectionso Sequel and Mongoid connections get it too. Non-async dataloaders pay one uncontendedsynchronizeper connection, on the firstnodes/cursor_forcall.Loading a copy instead (
limited_nodes.dup.to_a) would avoid the exception but still run the query once per Fiber, so the lock is the suggested root fix.Tests
spec/graphql/pagination/active_record_relation_connection_spec.rb: "loads the page only once when several Fibers resolve it". Guarded byRUBY_VERSION >= "3.2.0"like the otherasyncspecs. Fails onmaster, passes with this change.