Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize dataloader fetching for lists #5989

Closed
wants to merge 3 commits into from

Conversation

PascalSenn
Copy link
Member

@PascalSenn PascalSenn commented Mar 25, 2023

Dataloader currently have a issue with batching.

The first call to "LoadAsync" will initiate a Schedule on the batch dispatcher. In a specfic combination of cases, a fetch of a list of keys will lead to two fetches instead of one.

  1. The flag "dispatchOnSchedule" in the batch dispatcher is true (e.g. Mutation Resolver)
  2. You fetch more than one 1. key

What happens is the following. While we are iterating through all the requested keys, the first key will call schedule on the batch dispatcher. When we are calling LoadAsync from a mutation, Schedule will immediately dispatch. We continue synchronously and enter reenter the lock of the dataloader that we arleady own. We immediatly fetch with just one key instead of all the keys.

@sonarcloud
Copy link

sonarcloud bot commented May 19, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link
Member

@michaelstaib michaelstaib left a comment

Choose a reason for hiding this comment

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

The changes lead to dead locks.

Copy link

Qodana for .NET

1 new problem were found

Inspection name Severity Problems
Possible null reference argument for a parameter. 🔴 Failure 1

💡 Qodana analysis was run in the pull request mode: only the changed files were checked
☁️ View the detailed Qodana report

Contact Qodana team

Contact us at qodana-support@jetbrains.com

Copy link

codecov bot commented Jan 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.53%. Comparing base (2f25317) to head (0383f0a).
Report is 355 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5989      +/-   ##
==========================================
- Coverage   73.03%   69.53%   -3.51%     
==========================================
  Files        2485     2485              
  Lines      125534   125534              
==========================================
- Hits        91690    87285    -4405     
- Misses      33844    38249    +4405     
Flag Coverage Δ
unittests 69.53% <100.00%> (-3.51%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@michaelstaib michaelstaib force-pushed the main branch 2 times, most recently from 8a97a79 to e2d2300 Compare June 17, 2024 17:49
@michaelstaib michaelstaib self-assigned this Aug 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants