rmosolgo/graphql-ruby

Fiber-local variables are not copied when using AsyncDataloader

igorbelo opened this issue · 4 comments

igorbelo commented

Describe the bug

When using the GraphQL::Dataloader::AsyncDataloader, Fiber-local variables (Thread#[]) are missing in the context of the source execution.

I was on the fence about it being an actual bug, but since the patch at #3461 copied the Fiber-local variables over, I expected AsyncDataloader's behavior to be the same.

I believe the issue is when spawning the source tasks here. At that point, get_fiber_variables will be referencing the root task's Fiber-local storage.

Versions

graphql: 2.3.5
async: 2.12.0

Steps to reproduce

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'

  ruby '3.3.3'

  gem 'async', '2.12.0'
  gem 'graphql', '2.3.5'
end

class OneSource < GraphQL::Dataloader::Source
  def fetch(_ids)
    [Thread.current[:arbitrary_context]]
  end
end

class Query < GraphQL::Schema::Object
  field :one, String, null: true

  def one
    dataloader.with(OneSource).load(1)
  end
end

class Schema < GraphQL::Schema
  query Query

  use GraphQL::Dataloader::AsyncDataloader
end

Thread.current[:arbitrary_context] = 'a'
result = Schema.execute("query { one }")
puts result.to_h

Expected behavior

Expected the output of above script to be:

{"data"=>{"one"=>"a"}}

Which is the same behavior as if the script above was using GraphQL::Dataloader instead of GraphQL::Dataloader::AsyncDataloader.

Actual behavior

{"data"=>{"one"=>nil}}

Additional context

I noticed the issue when the request trace's flamegraph in Datadog looked weird. Basically any span created within the source execution was getting detached from the original trace. I stumbled upon #3461, DataDog/dd-trace-rb#1389 and steveklabnik/request_store#86 (comment), which are related to some extent.

As I said, I was on the fence about opening this as a bug, mainly because I believe the original solution of copying the variables over solves a problem that is not created by graphql-ruby itself. Nevertheless would be nice if dataloaders (sync and async) would behave the same way when it comes to Fiber-local variables, or at least having that difference between both mentioned in the docs. Unless there's a limitation I'm overlooking, I have a patch that fixes the issue for me, but felt like bringing this up as an issue first before proposing the patch in a PR.

rmosolgo commented

Hey, thank you for the very detailed report! I agree, it'd be nice to keep these two working the same way. I'd love to see the patch that you worked up if you don't mind opening a PR.

  • 👍1
rmosolgo commented

Fixed by #4994 -- thanks again!

  • 👍1
ioquatix commented

Instead of Thread.current[:arbitrary_context] you should try using Fiber[:arbitrary_context].

igorbelo commented

Instead of Thread.current[:arbitrary_context] you should try using Fiber[:arbitrary_context].

I'm not explicitly referencing Thread.current#[]; my example was just to illustrate how the Datadog gem sets the trace context.

However, I wasn't aware of Fiber::[] and Fiber::[]=, so thanks for sharing! I experimented with it a bit and initially didn't understand how variables were passed around fibers. Referring to the docs I could grasp it:

If the storage is unspecified, the default is to inherit a copy of the storage from the current fiber. This is the same as specifying storage: true.

Thanks for chiming in and for all the work you're doing for the Ruby ecosystem.

  • 👍1