Closed igorbelo closed 4 months ago
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.
Fixed by #4994 -- thanks again!
Instead of Thread.current[:arbitrary_context]
you should try using Fiber[:arbitrary_context]
.
Instead of
Thread.current[:arbitrary_context]
you should try usingFiber[: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.
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 https://github.com/rmosolgo/graphql-ruby/pull/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.5async
: 2.12.0Steps to reproduce
Expected behavior
Expected the output of above script to be:
Which is the same behavior as if the script above was using
GraphQL::Dataloader
instead ofGraphQL::Dataloader::AsyncDataloader
.Actual behavior
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 https://github.com/rmosolgo/graphql-ruby/pull/3461, https://github.com/DataDog/dd-trace-rb/issues/1389 and https://github.com/steveklabnik/request_store/issues/86#issuecomment-2091235285, 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.