From 6fb781eb5792338f6cfc984a80a243ac9df4b3cb Mon Sep 17 00:00:00 2001 From: Daniel Hopkins Date: Fri, 21 Aug 2026 03:57:19 -0700 Subject: [PATCH] Pagination: load connection nodes once when Fibers share a connection `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. --- lib/graphql/pagination/relation_connection.rb | 11 ++++- .../active_record_relation_connection_spec.rb | 48 +++++++++++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/lib/graphql/pagination/relation_connection.rb b/lib/graphql/pagination/relation_connection.rb index 0e6b091474f..785e89d5c46 100644 --- a/lib/graphql/pagination/relation_connection.rb +++ b/lib/graphql/pagination/relation_connection.rb @@ -221,7 +221,16 @@ def limited_nodes # returns an array of nodes def load_nodes # Return an array so we can consistently use `.index(node)` on it - @nodes ||= limited_nodes.to_a + return @nodes if @nodes + # `AsyncDataloader` may resolve sibling fields (eg, `edges` and `pageInfo`) + # in separate Fibers, so several callers can get here before `@nodes` is set. + # The lock makes the later ones wait and reuse the result instead of loading + # the same relation again -- with ActiveRecord, that second load can raise + # `UnmodifiableRelation`. `Mutex` is Fiber-aware: waiting yields to the + # scheduler rather than blocking the thread. + (@load_lock ||= Mutex.new).synchronize do + @nodes ||= limited_nodes.to_a + end end end end diff --git a/spec/graphql/pagination/active_record_relation_connection_spec.rb b/spec/graphql/pagination/active_record_relation_connection_spec.rb index f8573c3814b..9f7bbc918d3 100644 --- a/spec/graphql/pagination/active_record_relation_connection_spec.rb +++ b/spec/graphql/pagination/active_record_relation_connection_spec.rb @@ -253,5 +253,53 @@ def total_count include ConnectionAssertions end + + if RUBY_VERSION >= "3.2.0" + require "async" + + describe "when Fibers share a connection" do + # `sleep` yields to the scheduler like a slow query would, so the other + # Fibers reach `#load_nodes` before this load finishes. + class SlowLoadingRelation + attr_reader :load_count + + def initialize(relation) + @relation = relation + @load_count = 0 + end + + def to_a + @load_count += 1 + sleep(0.1) + @relation.to_a + end + end + + class SlowLoadingRelationConnection < GraphQL::Pagination::ActiveRecordRelationConnection + def load_count + @slow_nodes ? @slow_nodes.load_count : 0 + end + + private + + def limited_nodes + @slow_nodes ||= SlowLoadingRelation.new(super) + end + end + + it "loads the page only once when several Fibers resolve it" do + connection = SlowLoadingRelationConnection.new(Food.all, first: 2, max_page_size: 10) + results = [] + + Sync do + 3.times.map { Async { results << connection.nodes } }.each(&:wait) + end + + assert_equal 1, connection.load_count, "It runs the query once" + assert_equal 3, results.size + results.each { |nodes| assert_equal 2, nodes.size } + end + end + end end end