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

graphql-ruby lazy_resolve, Async::Semaphore #4

Merged
merged 6 commits into from
Mar 11, 2021

Conversation

trevorturk
Copy link
Collaborator

Re: #2 (comment)

See commit history for a glimpse into my struggles -- the first commit works, the second doesn't, and the third works again. I'm not understanding why the second commit didn't work -- I think I'm missing something similar to the issue in #3?

Copy link

@josh josh left a comment

Choose a reason for hiding this comment

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

In 7be7c5e, your delay_1_data method is returning an Async::Task not a JSON object.

I'm unsure how safe caching Async::Task objects is. It might not be safe to wait them multiple times and they may have references to other objects related to that original run. But I don't really know 🤷🏻‍♂️

Instead, I'd suggest moving Async { } to the top-level of the resolver method. Or use that async method decorator. You always need to ensure a Async::Task gets returned from those resolvers and that ensures it will.

@trevorturk trevorturk changed the title Rails async http falcon graphql lazy resolve rack async http falcon graphql lazy resolve Mar 10, 2021
@trevorturk trevorturk changed the title rack async http falcon graphql lazy resolve graphql-ruby lazy_resolve Mar 10, 2021
@trevorturk trevorturk changed the title graphql-ruby lazy_resolve graphql-ruby lazy_resolve, Async::Semaphore Mar 10, 2021
@trevorturk trevorturk merged commit fbc5b37 into main Mar 11, 2021
@trevorturk trevorturk deleted the rails-async-http-falcon-graphql-lazy-resolve branch March 11, 2021 20:29
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.

3 participants