graphql-ruby icon indicating copy to clipboard operation
graphql-ruby copied to clipboard

Try to make Dataloader work with Async gem

Open rmosolgo opened this issue 3 years ago • 1 comments

I'm hoping to get Dataloader working with the async gem, but it seems to work a bit differently than the others.

I tried using Fiber.schedule { ... }, but since it runs the fiber immediately, Fiber.yield doesn't work the same way from inside the fiber.

Also, the code previously expected spawn_fiber to return a non-running fiber, but Fiber.schedule returns an already-running fiber. So, gotta handle that nicely.

cc @bruno- Do you have any further suggestions about how this might work?

rmosolgo avatar Mar 21 '22 14:03 rmosolgo

UPDATE: Oh, I just realized my question below might have already been answered in https://github.com/rmosolgo/graphql-ruby/issues/4003#issuecomment-1081748792. The scheduling logic inside graphql-ruby is still desired.

Although not directly related to the method in this PR, I wonder if it makes sense to implement a new dataloader class using Async directly instead of only Async::Schduler? After reading the D -> A -> B -> C algorithm in Dataloader#run, I was wondering if such scheduling logic can be solved by async gem.

I am very new to Async and Fiber Scheduler, but if this approach make sense, following are the pros and cons I can think of:

Pros:

  1. grpahql-ruby wouldn't need to implement the scheduling logic because Async will be responsible for spawning and scheduling fibers.
  2. The problems that make CI fail currently can be bypassed.

Cons:

  1. Such new dataloader class will be somewhat duplicated with the existing AsyncDatalaoder, which may still need to be maintained for backward compatibility.
  2. async will become a dependency of graphql-ruby.

choznerol avatar May 24 '22 12:05 choznerol

I've added an example of how to make a non-async specific dataloader using Fiber.schedule.

https://github.com/socketry/async/compare/cfc045ea921b...b88db6f6bc8e

ioquatix avatar Oct 06 '22 01:10 ioquatix

Re-attempting this over in #4322 -- thanks for chiming in, everyone!

rmosolgo avatar Feb 22 '23 19:02 rmosolgo