Merge pull request #3461 from ianks/fiber-thread-locals

Copy parent thread local vars to dataloader fiber local vars

Author: Robert Mosolgo

guides/dataloader/overview.md

@@ -33,7 +33,7 @@ At a high level, `GraphQL::Dataloader`'s usage of `Fiber` looks like this:
 - `GraphQL::Dataloader` takes the first paused Fiber and resumes it, causing the `GraphQL::Dataloader::Source` to execute its `#fetch(...)` call. That Fiber continues execution as far as it can.
 - Likewise, paused Fibers are resumed, causing GraphQL execution to continue, until all paused Fibers are evaluated completely.
 
-Since _all_ GraphQL execution is inside a Fiber, __`Thread.current[...]` won't be set__. Those assignments are _Fiber-local_, so each new Fiber has an _empty_ `Thread.current`. For `GraphQL::Dataloader`, use GraphQL-Ruby's `context` object to provide "current" values to a GraphQL query. Inside `#fetch(...)` methods, those values can be re-assigned to `Thread.current` if application code requires it.
+Whenever `GraphQL::Dataloader` creates a new `Fiber`, it copies each pair from `Thread.current[...]` and reassigns them inside the new `Fiber`.
 
 ## Getting Started
 

lib/graphql/dataloader.rb

@@ -116,7 +116,7 @@ def run
         while @pending_jobs.any?
           # Create a Fiber to consume jobs until one of the jobs yields
           # or jobs run out
-          f = Fiber.new {
+          f = spawn_fiber {
             while (job = @pending_jobs.shift)
               job.call
             end
@@ -203,7 +203,7 @@ def create_source_fiber
         #
         # This design could probably be improved by maintaining a `@pending_sources` queue which is shared by the fibers,
         # similar to `@pending_jobs`. That way, when a fiber is resumed, it would never pick up work that was finished by a different fiber.
-        source_fiber = Fiber.new do
+        source_fiber = spawn_fiber do
           pending_sources.each(&:run_pending_keys)
         end
       end
@@ -216,5 +216,24 @@ def resume(fiber)
     rescue UncaughtThrowError => e
       throw e.tag, e.value
     end
+
+    # Copies the thread local vars into the fiber thread local vars. Many
+    # gems (such as RequestStore, MiniRacer, etc.) rely on thread local vars
+    # to keep track of execution context, and without this they do not
+    # behave as expected.
+    #
+    # @see https://github.com/rmosolgo/graphql-ruby/issues/3449
+    def spawn_fiber
+      fiber_locals = {} 
+
+      Thread.current.keys.each do |fiber_var_key|
+        fiber_locals[fiber_var_key] = Thread.current[fiber_var_key]
+      end 
+
+      Fiber.new do 
+        fiber_locals.each { |k, v| Thread.current[k] = v }
+        yield
+      end 
+    end
   end
 end

spec/graphql/dataloader_spec.rb

@@ -733,4 +733,55 @@ def request_all
 
     assert :world, value
   end
+
+  describe "thread local variables" do
+    module ThreadVariable
+      class Type < GraphQL::Schema::Object
+        field :key, String, null: false
+        field :value, String, null: false
+      end
+
+      class Source < GraphQL::Dataloader::Source
+        def fetch(keys)
+          keys.map { |key| OpenStruct.new(key: key, value: Thread.current[key.to_sym]) }
+        end
+      end
+
+      class QueryType < GraphQL::Schema::Object
+        field :thread_var, ThreadVariable::Type, null: true do
+          argument :key, GraphQL::Types::String, required: true
+        end
+
+        def thread_var(key:)
+          dataloader.with(ThreadVariable::Source).load(key)
+        end
+      end
+
+      class Schema < GraphQL::Schema
+        query ThreadVariable::QueryType
+        use GraphQL::Dataloader
+      end
+    end
+
+    it "sets the parent thread locals in the execution fiber" do
+      Thread.current[:test_thread_var] = 'foobarbaz'
+
+      result = ThreadVariable::Schema.execute(<<-GRAPHQL)
+      {
+        threadVar(key: "test_thread_var") {
+          key
+          value
+        }
+      }
+      GRAPHQL
+
+      expected_result = {
+        "data" => {
+          "threadVar" => { "key" => "test_thread_var", "value" => "foobarbaz" }
+        }
+      }
+
+      assert_equal expected_result, result.to_h
+    end
+  end
 end