Always generate a random id for each request, and use a different thr…

…ead-local variable. Fixes #9
ElMassimo · May 20, 2019 · da2e349 · ElMassimo · May 20, 2019 · da2e349
1 parent 19ce692
commit da2e349
Show file tree

Hide file tree

Showing 8 changed files with 81 additions and 33 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,7 @@
+## RequestLocals 2.0.0 (2019-05-20) ##
+
+*   Always use a random uuid to identify the request in the store. Thanks [@gtmax](https://github.com/gtmax)!
+
 ## RequestLocals 1.0.3 (2017-06-06) ##
 
 *   Improve API compatibility with `request_store` by adding `key?` and `exist?`. Thanks [@abrisse](https://github.com/abrisse)!

diff --git a/Gemfile b/Gemfile
@@ -5,7 +5,7 @@ gemspec
 group :test do
   gem 'pry'
   gem 'minitest-reporters'
-  gem 'codeclimate-test-reporter', '~> 1.0.0'
+  gem 'codeclimate-test-reporter'
 end
 
 group :doc do

diff --git a/README.md b/README.md
@@ -77,11 +77,11 @@ use RequestStoreRails::Middleware
 ```
 
 ## Multi-Threading
-The middleware in the gem sets a thread-local variable `:request_id` in
+The middleware in the gem sets a thread-local variable `:request_store_id` in
 `Thread.current` for the main thread that is executing the request.
 
 If you need to spawn threads within a server that is already using thread-based
-concurrency, all you need to do is to make sure that the `:request_id`
+concurrency, all you need to do is to make sure that the `:request_store_id`
 variable is set for your threads, and you will be able to access the
 `RequestLocals` as usual.
 
@@ -93,9 +93,9 @@ class ThreadWithContext
 
   # Public: Returns a new Thread that preserves the context of the current request.
   def ThreadWithContext.new(*args)
-    request_id = Thread.current[:request_id]
+    store_id = RequestLocals.current_store_id
     Thread.new {
-      Thread.current[:request_id] = request_id
+      RequestLocals.set_current_store_id(store_id)
       yield *args
     }
   end
@@ -133,6 +133,35 @@ if RequestStore != RequestLocals
 end
 ```
 
+## Usage in Sidekiq
+If your code depends on these global variables, it's likely that you'll need
+to avoid collisions in Sidekiq workers (which would happen if the current store
+id is `nil`).
+
+You can use the following middleware, using the job id to identify the store:
+
+```ruby
+class Sidekiq::Middleware::Server::RequestStoreRails
+  def call(_worker, job, _queue)
+    RequestLocals.set_current_store_id(job['jid'])
+    yield
+  ensure
+    RequestLocals.clear!
+    RequestLocals.set_current_store_id(nil)
+  end
+end
+```
+
+Make sure to configure it as server middleware:
+
+```ruby
+Sidekiq.configure_server do |config|
+  config.server_middleware do |chain|
+    chain.add Sidekiq::Middleware::Server::RequestStoreRails
+  end
+end
+```
+
 ## Special Thanks
 The inspiration for this gem, tests, and a big part of the readme were borrowed
 from the really cool [`request_store`](https://github.com/steveklabnik/request_store) gem.

diff --git a/lib/request_locals.rb b/lib/request_locals.rb
@@ -5,20 +5,24 @@
 # Public: Provides per-request global storage, by offering an interface that is
 # very similar to Rails.cache, or Hash.
 #
-# The store may be shared between threads, as long as the :request_id
-# thread-local variable is set.
+# The store may be shared between threads, as long as the current store id is
+# set as a thread-local variable.
 #
 # Intended to work in collaboration with the RequestStoreRails middleware, that
 # will clear the request local variables after each request.
 class RequestLocals
   include Singleton
   extend Forwardable
 
+  # Internal: The key of the thread-local variable the library uses to store the
+  # identifier of the current store, used during the request lifecycle.
+  REQUEST_STORE_ID = :request_store_id
+
   class << self
     extend Forwardable
 
     # Public: Public methods of RequestLocals, they are delegated to the Singleton instance.
-    def_delegators :instance, :clear!, :clear_all!, :[], :[]=, :fetch, :delete, :exist?, :key?, :empty?
+    def_delegators :instance, :clear!, :clear_all!, :current_store_id, :[], :[]=, :fetch, :delete, :exist?, :key?, :empty?
 
     # Public: There is no accounting for taste, RequestLocals[:foo] vs RequestLocals.store[:foo]
     alias_method :store, :instance
@@ -46,7 +50,7 @@ def initialize
   #
   # Returns nothing.
   def clear!
-    @cache.delete(current_request_id)
+    @cache.delete(current_store_id)
   end
 
   # Public: Clears all the request-local variable stores.
@@ -75,21 +79,27 @@ def fetch(key, &block)
     store.compute_if_absent(key, &block)
   end
 
+  # Public: The current request is inferred from the current thread.
+  #
+  # NOTE: It's very important to set the current store id when spawning new
+  # threads within a single request, using `RequestLocals.set_current_store_id`.
+  def current_store_id
+    Thread.current[REQUEST_STORE_ID]
+  end
+
+  # Public: Changes the store RequestLocals will read from in the current thread.
+  def self.set_current_store_id(id)
+    Thread.current[REQUEST_STORE_ID] = id
+  end
+
 protected
 
   # Internal: Returns the structure that holds the request-local variables for
   # the current request.
   #
   # Returns a ThreadSafe::Cache.
   def store
-    @cache.compute_if_absent(current_request_id) { new_store }
-  end
-
-  # Internal: The current request is inferred from the current thread. It's very
-  # important to pass on the :request_id thread local variable when spawning new
-  # threads within a single request.
-  def current_request_id
-    Thread.current[:request_id]
+    @cache.compute_if_absent(current_store_id) { new_store }
   end
 
   # Internal: Returns a new empty structure where the request-local variables

diff --git a/lib/request_store_rails/middleware.rb b/lib/request_store_rails/middleware.rb
@@ -2,29 +2,33 @@
 
 module RequestStoreRails
 
-  # Public: Middleware that takes care of setting the thread-local variable
-  # :request_id, which enables RequestLocals to associate threads and requests.
+  # Public: Middleware that takes care of setting a thread-local variable, which
+  # enables RequestLocals to associate threads with the store for a request.
   class Middleware
 
     def initialize(app)
       @app = app
     end
 
-    # Internal: Assigns the :request_id thread-local variable, and cleans up all
-    # the request-local variables after the request.
+    # Internal: Assigns a thread-local variable to identify the current store,
+    # and cleans up all the variables stored for the request once it finishes.
     def call(env)
-      Thread.current[:request_id] = extract_request_id(env)
+      RequestLocals.set_current_store_id(extract_request_id(env))
       @app.call(env)
     ensure
       RequestLocals.clear!
-      Thread.current[:request_id] = nil
+      RequestLocals.set_current_store_id(nil)
     end
 
   protected
 
     # Internal: Extracts the request id from the environment, or generates one.
+    #
+    # NOTE: We always generate an id to prevent accidental conflicts from an
+    # externally provided one, but subclasses of this middleware might override
+    # it.
     def extract_request_id(env)
-      env['action_dispatch.request_id'] || env['HTTP_X_REQUEST_ID'] || SecureRandom.hex(16)
+      SecureRandom.uuid
     end
   end
 end
diff --git a/lib/request_store_rails/version.rb b/lib/request_store_rails/version.rb
@@ -1,4 +1,4 @@
 module RequestStoreRails
 
-  VERSION = '1.0.3'
+  VERSION = '2.0.0'
 end
diff --git a/test/middleware_test.rb b/test/middleware_test.rb
@@ -9,7 +9,7 @@ def setup
     @app = RackApp.new
     @middleware = RequestStoreRails::Middleware.new(@app)
 
-    Thread.current[:request_id] = nil
+    RequestLocals.set_current_store_id(nil)
     RequestLocals.clear_all!
   end
 

diff --git a/test/request_locals_test.rb b/test/request_locals_test.rb
@@ -7,8 +7,9 @@
 class RequestLocalsTest < Minitest::Unit::TestCase
 
   def test_initial_state
-    Thread.current[:request_id] = :random_id
+    RequestLocals.set_current_store_id(:random_id)
     assert_empty RequestLocals.store
+    assert RequestLocals.current_store_id
   end
 
   def test_exist_and_delete
@@ -68,17 +69,17 @@ def test_store_per_request
     RequestLocals.clear_all!
     assert_empty global_store
 
-    Thread.current[:request_id] = :awesome_id
+    RequestLocals.set_current_store_id(:awesome_id)
     RequestLocals[:foo] = :bar
 
     Thread.new {
       assert_empty RequestLocals.store
       RequestLocals[:foo] = :mar
 
-      Thread.current[:request_id] = :awesome_id
+      RequestLocals.set_current_store_id(:awesome_id)
       assert_equal :bar, RequestLocals.store[:foo]
 
-      Thread.current[:request_id] = :different_id
+      RequestLocals.set_current_store_id(:different_id)
       assert_empty RequestLocals.store
 
       RequestLocals.fetch(:foo) { :beer }
@@ -93,14 +94,14 @@ def test_clear_per_request
     RequestLocals.clear_all!
     assert_empty global_store
 
-    Thread.current[:request_id] = :awesome_id
+    RequestLocals.set_current_store_id(:awesome_id)
     RequestLocals.fetch(:foo) { :bar }
 
     Thread.new {
-      Thread.current[:request_id] = :awesome_id
+      RequestLocals.set_current_store_id(:awesome_id)
       RequestLocals[:foo] = :beer
 
-      Thread.current[:request_id] = :different_id
+      RequestLocals.set_current_store_id(:different_id)
       RequestLocals[:foo] = :mar
       RequestLocals.clear!
     }.join