Merge branch 'bvl-remove-sidekiq-request-store-env' into 'master'

Remove SIDEKIQ_REQUEST_STORE env variable

See merge request gitlab-org/gitlab!29955

Gemfile

@@ -287,7 +287,7 @@ gem 'addressable', '~> 2.7'
 gem 'font-awesome-rails', '~> 4.7'
 gem 'gemojione', '~> 3.3'
 gem 'gon', '~> 6.2'
-gem 'request_store', '~> 1.3'
+gem 'request_store', '~> 1.5'
 gem 'base32', '~> 0.3.0'
 
 gem "gitlab-license", "~> 1.0"

Gemfile.lock

@@ -883,7 +883,8 @@ GEM
       declarative (< 0.1.0)
       declarative-option (< 0.2.0)
       uber (< 0.2.0)
-    request_store (1.3.1)
+    request_store (1.5.0)
+      rack (>= 1.4)
     responders (3.0.0)
       actionpack (>= 5.0)
       railties (>= 5.0)
@@ -1350,7 +1351,7 @@ DEPENDENCIES
   redis (~> 4.0)
   redis-namespace (~> 1.6.0)
   redis-rails (~> 5.0.2)
-  request_store (~> 1.3)
+  request_store (~> 1.5)
   responders (~> 3.0)
   retriable (~> 3.1.2)
   rouge (~> 3.18.0)

changelogs/unreleased/bvl-remove-sidekiq-request-store-env.yml

+---
+title: Remove the SIDEKIQ_REQUEST_STORE configuration
+merge_request: 29955
+author:
+type: other

config/initializers/sidekiq.rb

@@ -33,7 +33,6 @@ enable_json_logs = Gitlab.config.sidekiq.log_format == 'json'
 enable_sidekiq_memory_killer = ENV['SIDEKIQ_MEMORY_KILLER_MAX_RSS'].to_i.nonzero?
 use_sidekiq_daemon_memory_killer = ENV["SIDEKIQ_DAEMON_MEMORY_KILLER"].to_i.nonzero?
 use_sidekiq_legacy_memory_killer = !use_sidekiq_daemon_memory_killer
-use_request_store = ENV.fetch('SIDEKIQ_REQUEST_STORE', 1).to_i.nonzero?
 
 Sidekiq.configure_server do |config|
   if enable_json_logs
@@ -50,8 +49,7 @@ Sidekiq.configure_server do |config|
   config.server_middleware(&Gitlab::SidekiqMiddleware.server_configurator({
     metrics: Settings.monitoring.sidekiq_exporter,
     arguments_logger: ENV['SIDEKIQ_LOG_ARGUMENTS'] && !enable_json_logs,
-    memory_killer: enable_sidekiq_memory_killer && use_sidekiq_legacy_memory_killer,
-    request_store: use_request_store
+    memory_killer: enable_sidekiq_memory_killer && use_sidekiq_legacy_memory_killer
   }))
 
   config.client_middleware(&Gitlab::SidekiqMiddleware.client_configurator)

ee/spec/services/design_management/save_designs_service_spec.rb

@@ -272,7 +272,7 @@ describe DesignManagement::SaveDesignsService do
           expect { run_service }.to change { commit_count.call }.by(1)
         end
 
-        it 'only does 5 gitaly calls', :request_store, :sidekiq_might_not_need_inline do
+        it 'only does 4 gitaly calls', :request_store, :sidekiq_might_not_need_inline do
           service = described_class.new(project, user, issue: issue, files: files)
           # Some unrelated calls that are usually cached or happen only once
           service.__send__(:repository).create_if_not_exists
@@ -281,8 +281,8 @@ describe DesignManagement::SaveDesignsService do
           request_count = -> { Gitlab::GitalyClient.get_request_count }
 
           # An exists?, a check for existing blobs, default branch, an after_commit
-          # callback on LfsObjectsProject, and the creation of commits
-          expect { service.execute }.to change(&request_count).by(5)
+          # callback on LfsObjectsProject
+          expect { service.execute }.to change(&request_count).by(4)
         end
 
         context 'when uploading too many files' do

lib/gitlab/sidekiq_middleware.rb

@@ -7,13 +7,13 @@ module Gitlab
     # The result of this method should be passed to
     # Sidekiq's `config.server_middleware` method
     # eg: `config.server_middleware(&Gitlab::SidekiqMiddleware.server_configurator)`
-    def self.server_configurator(metrics: true, arguments_logger: true, memory_killer: true, request_store: true)
+    def self.server_configurator(metrics: true, arguments_logger: true, memory_killer: true)
       lambda do |chain|
         chain.add ::Gitlab::SidekiqMiddleware::Monitor
         chain.add ::Gitlab::SidekiqMiddleware::ServerMetrics if metrics
         chain.add ::Gitlab::SidekiqMiddleware::ArgumentsLogger if arguments_logger
         chain.add ::Gitlab::SidekiqMiddleware::MemoryKiller if memory_killer
-        chain.add ::Gitlab::SidekiqMiddleware::RequestStoreMiddleware if request_store
+        chain.add ::Gitlab::SidekiqMiddleware::RequestStoreMiddleware
         chain.add ::Gitlab::SidekiqMiddleware::BatchLoader
         chain.add ::Labkit::Middleware::Sidekiq::Server
         chain.add ::Gitlab::SidekiqMiddleware::InstrumentationLogger

lib/gitlab/with_request_store.rb

@@ -2,12 +2,24 @@
 
 module Gitlab
   module WithRequestStore
-    def with_request_store
+    def with_request_store(&block)
+      # Skip enabling the request store if it was already active. Whatever
+      # instantiated the request store first is responsible for clearing it
+      return yield if RequestStore.active?
+
+      enabling_request_store(&block)
+    end
+
+    private
+
+    def enabling_request_store
       RequestStore.begin!
       yield
     ensure
       RequestStore.end!
       RequestStore.clear!
     end
+
+    extend self
   end
 end

spec/lib/gitlab/sidekiq_middleware_spec.rb

@@ -32,8 +32,7 @@ describe Gitlab::SidekiqMiddleware do
         described_class.server_configurator(
           metrics: metrics,
           arguments_logger: arguments_logger,
-          memory_killer: memory_killer,
-          request_store: request_store
+          memory_killer: memory_killer
         ).call(chain)
 
         example.run
@@ -77,13 +76,11 @@ describe Gitlab::SidekiqMiddleware do
       let(:metrics) { false }
       let(:arguments_logger) { false }
       let(:memory_killer) { false }
-      let(:request_store) { false }
       let(:disabled_sidekiq_middlewares) do
         [
           Gitlab::SidekiqMiddleware::ServerMetrics,
           Gitlab::SidekiqMiddleware::ArgumentsLogger,
-          Gitlab::SidekiqMiddleware::MemoryKiller,
-          Gitlab::SidekiqMiddleware::RequestStoreMiddleware
+          Gitlab::SidekiqMiddleware::MemoryKiller
         ]
       end
 
@@ -94,7 +91,6 @@ describe Gitlab::SidekiqMiddleware do
       let(:metrics) { true }
       let(:arguments_logger) { true }
       let(:memory_killer) { true }
-      let(:request_store) { true }
       let(:disabled_sidekiq_middlewares) { [] }
 
       it_behaves_like "a server middleware chain"

spec/lib/gitlab/with_request_store_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require 'request_store'
+
+describe Gitlab::WithRequestStore do
+  let(:fake_class) { Class.new { include Gitlab::WithRequestStore } }
+
+  subject(:object) { fake_class.new }
+
+  describe "#with_request_store" do
+    it 'starts a request store and yields control' do
+      expect(RequestStore).to receive(:begin!).ordered
+      expect(RequestStore).to receive(:end!).ordered
+      expect(RequestStore).to receive(:clear!).ordered
+
+      expect { |b| object.with_request_store(&b) }.to yield_control
+    end
+
+    it 'only starts a request store once when nested' do
+      expect(RequestStore).to receive(:begin!).ordered.once.and_call_original
+      expect(RequestStore).to receive(:end!).ordered.once.and_call_original
+      expect(RequestStore).to receive(:clear!).ordered.once.and_call_original
+
+      object.with_request_store do
+        expect { |b| object.with_request_store(&b) }.to yield_control
+      end
+    end
+  end
+end

spec/spec_helper.rb

@@ -286,12 +286,7 @@ RSpec.configure do |config|
   end
 
   config.around(:example, :request_store) do |example|
-    RequestStore.begin!
-
-    example.run
-
-    RequestStore.end!
-    RequestStore.clear!
+    Gitlab::WithRequestStore.with_request_store { example.run }
   end
 
   config.around do |example|
@@ -305,12 +300,10 @@ RSpec.configure do |config|
       Gitlab::SidekiqMiddleware.server_configurator(
         metrics: false, # The metrics don't go anywhere in tests
         arguments_logger: false, # We're not logging the regular messages for inline jobs
-        memory_killer: false, # This is not a thing we want to do inline in tests
-        # Don't enable this if the request store is active in the spec itself
-        # This needs to run within the `request_store` around block defined above
-        request_store: !RequestStore.active?
+        memory_killer: false # This is not a thing we want to do inline in tests
       ).call(chain)
       chain.add DisableQueryLimit
+      chain.insert_after ::Gitlab::SidekiqMiddleware::RequestStoreMiddleware, IsolatedRequestStore
 
       example.run
     end

spec/support/sidekiq_middleware.rb

@@ -31,3 +31,16 @@ class DisableQueryLimit
     end
   end
 end
+
+# When running `Sidekiq::Testing.inline!` each job is using a request-store.
+# This middleware makes sure the values don't leak into eachother.
+class IsolatedRequestStore
+  def call(_worker, msg, queue)
+    old_store = RequestStore.store.dup
+    RequestStore.clear!
+
+    yield
+
+    RequestStore.store = old_store
+  end
+end