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

Recreate loader if old loader is on different loop #395

Merged
merged 2 commits into from
Oct 9, 2023

Conversation

zeptonaut
Copy link
Contributor

@zeptonaut zeptonaut commented Jun 13, 2023

Fix for the problem encountered #35 (comment)

This change makes it so that the batched data loader is recreated at field resolution time if the current event loop is different than the event loop for the loader.

Previously, the loaders were only created at GraphQL Schema creation time. In many systems (including FastAPI and Starlette's TestClient), the Schema object is required in order to initiate the mount of the GraphQL routes. Thereafter, a new event loop is created (owned by the request handler).

This means that, almost by definition, in many frameworks the initial loaders will be created on one event loop and used on a different event loop in the future. To resolve this problem, this PR makes it so that we only use the memoized loader if it was created on the same event loop as where the resolution is happening.

@zeptonaut zeptonaut closed this Jun 13, 2023
@zeptonaut zeptonaut reopened this Jun 13, 2023
@zeptonaut
Copy link
Contributor Author

@erikwrede any way to get a review on this? Would love to hear if this matches your mental model of how the batching loaders should work.

andrew-oreshko added a commit to andrew-oreshko/graphene-sqlalchemy that referenced this pull request Aug 28, 2023
@erikwrede
Copy link
Member

erikwrede commented Oct 6, 2023

Hey @zeptonaut, please excuse the long wait on this issue. The last months have been very busy so this slipped through. The issues with dataloader and event loops are known and a better solution is definitely necessary 😊

I'll discuss your approach with the others in our discord meeting on monday @ 9PM CEST and will get back to you.

@zeptonaut
Copy link
Contributor Author

Hi @erikwrede : for what it's worth, this is no longer blocking us as we figured out an alternate way to do data loaders. Ultimately, we ran into problems when we updated to a newer version of Graphene or Graphene SQLAlchemy (can't remember) and this patch started causing problems.

I do think though that the general idea though in this PR holds and that, like you said, figuring out a better mapping between data loaders and event loops probably needs to happen.

@erikwrede
Copy link
Member

@zeptonaut can you share how you handle dataloading now? Always looking for some new, proven approaches. A QueryBuilder of some sort is definitely on our list for the future, due to the known inefficiencies with dataloading and coroutine state explosion in large queries.

@erikwrede erikwrede merged commit b94230e into graphql-python:master Oct 9, 2023
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.

None yet

3 participants