Skip to content

Prevent duplicate relation count queries - #5717

Open
ydah wants to merge 1 commit into
rmosolgo:masterfrom
ydah:avoid-duplicate-relation-counts
Open

Prevent duplicate relation count queries#5717
ydah wants to merge 1 commit into
rmosolgo:masterfrom
ydah:avoid-duplicate-relation-counts

Conversation

@ydah

@ydah ydah commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

This follows up on #5708, which prevents concurrent Fibers from loading the same relation page more than once.

RelationConnection#limited_nodes remains independently memoized outside that lock. With last: pagination and no existing relation limit, it calls relation_count(sliced_nodes). If that query yields, another Fiber can enter the same @limited_nodes ||= ... block before the first one finishes.

Under AsyncDataloader, this can happen when connection fields resolve separately:

  • edges or nodes calls load_nodes
  • pageInfo.hasPreviousPage calls limited_nodes directly

Both Fibers can therefore issue the same COUNT query.

connection = Connection.new(
  relation,
  last: 2,
  context: { dataloader: GraphQL::Dataloader::AsyncDataloader.new },
)

Sync do
  [
    Async { connection.nodes },
    Async { connection.has_previous_page },
  ].each(&:wait)
end

Before this change, the relation is counted twice. After this change, it is counted once.

This PR protects both limited_nodes construction and the final node load with the same Monitor. Monitor is reentrant, so load_nodes can safely call limited_nodes while holding it without risking ThreadError: deadlock; recursive locking.

The synchronization is only used with AsyncDataloader; other dataloaders retain the existing unsynchronized path and its current overhead.

@rmosolgo rmosolgo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I think it needs one change now from a fix on master.

I haven't seen Monitor before -- what's the advantage over Mutex?

Comment thread lib/graphql/pagination/relation_connection.rb Outdated
@ydah
ydah force-pushed the avoid-duplicate-relation-counts branch 2 times, most recently from 969e7bf to 8be8baf Compare August 29, 2026 23:30
@ydah
ydah force-pushed the avoid-duplicate-relation-counts branch from 8be8baf to 8d34406 Compare August 29, 2026 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants