From 45cf64c827270d66a88d483bb3f9043a90301255 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Thu, 20 Sep 2018 11:49:58 -0700 Subject: Use a null object with RequestStore Makes it easier and safer to use RequestStore because you don't need to check `RequestStore.active?` before using it. You just have to use `Gitlab::SafeRequestStore` instead. --- lib/gitlab/null_request_store.rb | 41 +++++ lib/gitlab/safe_request_store.rb | 23 +++ spec/lib/gitlab/null_request_store_spec.rb | 75 +++++++++ spec/lib/gitlab/safe_request_store_spec.rb | 245 +++++++++++++++++++++++++++++ 4 files changed, 384 insertions(+) create mode 100644 lib/gitlab/null_request_store.rb create mode 100644 lib/gitlab/safe_request_store.rb create mode 100644 spec/lib/gitlab/null_request_store_spec.rb create mode 100644 spec/lib/gitlab/safe_request_store_spec.rb diff --git a/lib/gitlab/null_request_store.rb b/lib/gitlab/null_request_store.rb new file mode 100644 index 00000000000..8db331dcb9f --- /dev/null +++ b/lib/gitlab/null_request_store.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +# Used by Gitlab::SafeRequestStore +module Gitlab + # The methods `begin!`, `clear!`, and `end!` are not defined because they + # should only be called directly on `RequestStore`. + class NullRequestStore + def store + {} + end + + def active? + end + + def read(key) + end + + def [](key) + end + + def write(key, value) + value + end + + def []=(key, value) + value + end + + def exist?(key) + false + end + + def fetch(key, &block) + yield + end + + def delete(key, &block) + yield(key) if block_given? + end + end +end diff --git a/lib/gitlab/safe_request_store.rb b/lib/gitlab/safe_request_store.rb new file mode 100644 index 00000000000..4e82353adb6 --- /dev/null +++ b/lib/gitlab/safe_request_store.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Gitlab + module SafeRequestStore + NULL_STORE = Gitlab::NullRequestStore.new + + class << self + # These methods should always run directly against RequestStore + delegate :clear!, :begin!, :end!, :active?, to: :RequestStore + + # These methods will run against NullRequestStore if RequestStore is disabled + delegate :read, :[], :write, :[]=, :exist?, :fetch, :delete, to: :store + end + + def self.store + if RequestStore.active? + RequestStore + else + NULL_STORE + end + end + end +end diff --git a/spec/lib/gitlab/null_request_store_spec.rb b/spec/lib/gitlab/null_request_store_spec.rb new file mode 100644 index 00000000000..c023dac53ad --- /dev/null +++ b/spec/lib/gitlab/null_request_store_spec.rb @@ -0,0 +1,75 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::NullRequestStore do + let(:null_store) { described_class.new } + + describe '#store' do + it 'returns an empty hash' do + expect(null_store.store).to eq({}) + end + end + + describe '#active?' do + it 'returns falsey' do + expect(null_store.active?).to be_falsey + end + end + + describe '#read' do + it 'returns nil' do + expect(null_store.read('foo')).to be nil + end + end + + describe '#[]' do + it 'returns nil' do + expect(null_store['foo']).to be nil + end + end + + describe '#write' do + it 'returns the same value' do + expect(null_store.write('key', 'value')).to eq('value') + end + end + + describe '#[]=' do + it 'returns the same value' do + expect(null_store['key'] = 'value').to eq('value') + end + end + + describe '#exist?' do + it 'returns falsey' do + expect(null_store.exist?('foo')).to be_falsey + end + end + + describe '#fetch' do + it 'returns the block result' do + expect(null_store.fetch('key') { 'block result' }).to eq('block result') + end + end + + describe '#delete' do + context 'when a block is given' do + it 'yields the key to the block' do + expect do |b| + null_store.delete('foo', &b) + end.to yield_with_args('foo') + end + + it 'returns the block result' do + expect(null_store.delete('foo') { |key| 'block result' }).to eq('block result') + end + end + + context 'when a block is not given' do + it 'returns nil' do + expect(null_store.delete('foo')).to be nil + end + end + end +end diff --git a/spec/lib/gitlab/safe_request_store_spec.rb b/spec/lib/gitlab/safe_request_store_spec.rb new file mode 100644 index 00000000000..27766fa0eda --- /dev/null +++ b/spec/lib/gitlab/safe_request_store_spec.rb @@ -0,0 +1,245 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::SafeRequestStore do + describe '.store' do + context 'when RequestStore is active', :request_store do + it 'uses RequestStore' do + expect(described_class.store).to eq(RequestStore) + end + end + + context 'when RequestStore is NOT active' do + it 'does not use RequestStore' do + expect(described_class.store).to be_a(Gitlab::NullRequestStore) + end + end + end + + describe '.begin!' do + context 'when RequestStore is active', :request_store do + it 'uses RequestStore' do + expect(RequestStore).to receive(:begin!) + + described_class.begin! + end + end + + context 'when RequestStore is NOT active' do + it 'uses RequestStore' do + expect(RequestStore).to receive(:begin!) + + described_class.begin! + end + end + end + + describe '.clear!' do + context 'when RequestStore is active', :request_store do + it 'uses RequestStore' do + expect(RequestStore).to receive(:clear!).twice.and_call_original + + described_class.clear! + end + end + + context 'when RequestStore is NOT active' do + it 'uses RequestStore' do + expect(RequestStore).to receive(:clear!).and_call_original + + described_class.clear! + end + end + end + + describe '.end!' do + context 'when RequestStore is active', :request_store do + it 'uses RequestStore' do + expect(RequestStore).to receive(:end!).twice.and_call_original + + described_class.end! + end + end + + context 'when RequestStore is NOT active' do + it 'uses RequestStore' do + expect(RequestStore).to receive(:end!).and_call_original + + described_class.end! + end + end + end + + describe '.write' do + context 'when RequestStore is active', :request_store do + it 'uses RequestStore' do + expect do + described_class.write('foo', true) + end.to change { described_class.read('foo') }.from(nil).to(true) + end + end + + context 'when RequestStore is NOT active' do + it 'does not use RequestStore' do + expect do + described_class.write('foo', true) + end.not_to change { described_class.read('foo') }.from(nil) + end + end + end + + describe '.[]=' do + context 'when RequestStore is active', :request_store do + it 'uses RequestStore' do + expect do + described_class['foo'] = true + end.to change { described_class.read('foo') }.from(nil).to(true) + end + end + + context 'when RequestStore is NOT active' do + it 'does not use RequestStore' do + expect do + described_class['foo'] = true + end.not_to change { described_class.read('foo') }.from(nil) + end + end + end + + describe '.read' do + context 'when RequestStore is active', :request_store do + it 'uses RequestStore' do + expect do + RequestStore.write('foo', true) + end.to change { described_class.read('foo') }.from(nil).to(true) + end + end + + context 'when RequestStore is NOT active' do + it 'does not use RequestStore' do + expect do + RequestStore.write('foo', true) + end.not_to change { described_class.read('foo') }.from(nil) + + RequestStore.clear! # Clean up + end + end + end + + describe '.[]' do + context 'when RequestStore is active', :request_store do + it 'uses RequestStore' do + expect do + RequestStore.write('foo', true) + end.to change { described_class['foo'] }.from(nil).to(true) + end + end + + context 'when RequestStore is NOT active' do + it 'does not use RequestStore' do + expect do + RequestStore.write('foo', true) + end.not_to change { described_class['foo'] }.from(nil) + + RequestStore.clear! # Clean up + end + end + end + + describe '.exist?' do + context 'when RequestStore is active', :request_store do + it 'uses RequestStore' do + expect do + RequestStore.write('foo', 'not nil') + end.to change { described_class.exist?('foo') }.from(false).to(true) + end + end + + context 'when RequestStore is NOT active' do + it 'does not use RequestStore' do + expect do + RequestStore.write('foo', 'not nil') + end.not_to change { described_class.exist?('foo') }.from(false) + + RequestStore.clear! # Clean up + end + end + end + + describe '.fetch' do + context 'when RequestStore is active', :request_store do + it 'uses RequestStore' do + expect do + described_class.fetch('foo') { 'block result' } + end.to change { described_class.read('foo') }.from(nil).to('block result') + end + end + + context 'when RequestStore is NOT active' do + it 'does not use RequestStore' do + RequestStore.clear! # Ensure clean + + expect do + described_class.fetch('foo') { 'block result' } + end.not_to change { described_class.read('foo') }.from(nil) + + RequestStore.clear! # Clean up + end + end + end + + describe '.delete' do + context 'when RequestStore is active', :request_store do + it 'uses RequestStore' do + described_class.write('foo', true) + + expect do + described_class.delete('foo') + end.to change { described_class.read('foo') }.from(true).to(nil) + end + + context 'when given a block and the key exists' do + it 'does not execute the block' do + described_class.write('foo', true) + + expect do |b| + described_class.delete('foo', &b) + end.not_to yield_control + end + end + + context 'when given a block and the key does not exist' do + it 'yields the key and returns the block result' do + result = described_class.delete('foo') { |key| "#{key} block result" } + + expect(result).to eq('foo block result') + end + end + end + + context 'when RequestStore is NOT active' do + around do |example| + RequestStore.write('foo', true) + + example.run + + RequestStore.clear! # Clean up + end + + it 'does not use RequestStore' do + expect do + described_class.delete('foo') + end.not_to change { RequestStore.read('foo') }.from(true) + end + + context 'when given a block' do + it 'yields the key and returns the block result' do + result = described_class.delete('foo') { |key| "#{key} block result" } + + expect(result).to eq('foo block result') + end + end + end + end +end -- cgit v1.2.1 From f107bc69e34f79ea8faaa154caefe56948b8dd68 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Thu, 20 Sep 2018 12:32:54 -0700 Subject: Simplify by using Gitlab::SafeRequestStore These are clear wins. --- app/controllers/concerns/with_performance_bar.rb | 6 +----- app/models/concerns/cacheable_attributes.rb | 6 +----- app/models/legacy_diff_note.rb | 6 +----- app/models/project.rb | 6 +----- lib/banzai/filter/external_issue_reference_filter.rb | 4 +--- lib/feature.rb | 10 +++------- lib/gitlab/current_settings.rb | 6 +----- lib/gitlab/diff/position.rb | 20 ++++++++------------ lib/gitlab/git/wiki.rb | 6 +----- lib/gitlab/issuables_count_for_state.rb | 9 ++------- lib/gitlab/performance_bar/peek_query_tracker.rb | 2 +- 11 files changed, 21 insertions(+), 60 deletions(-) diff --git a/app/controllers/concerns/with_performance_bar.rb b/app/controllers/concerns/with_performance_bar.rb index c12839c7bbd..77c3d476ac6 100644 --- a/app/controllers/concerns/with_performance_bar.rb +++ b/app/controllers/concerns/with_performance_bar.rb @@ -10,11 +10,7 @@ module WithPerformanceBar def peek_enabled? return false unless Gitlab::PerformanceBar.enabled?(current_user) - if RequestStore.active? - RequestStore.fetch(:peek_enabled) { cookie_or_default_value } - else - cookie_or_default_value - end + Gitlab::SafeRequestStore.fetch(:peek_enabled) { cookie_or_default_value } end private diff --git a/app/models/concerns/cacheable_attributes.rb b/app/models/concerns/cacheable_attributes.rb index 62b78c3611c..f8034be8376 100644 --- a/app/models/concerns/cacheable_attributes.rb +++ b/app/models/concerns/cacheable_attributes.rb @@ -27,11 +27,7 @@ module CacheableAttributes end def cached - if RequestStore.active? - RequestStore[:"#{name}_cached_attributes"] ||= retrieve_from_cache - else - retrieve_from_cache - end + Gitlab::SafeRequestStore[:"#{name}_cached_attributes"] ||= retrieve_from_cache end def retrieve_from_cache diff --git a/app/models/legacy_diff_note.rb b/app/models/legacy_diff_note.rb index 20f9b18e4ca..00dec6bb92b 100644 --- a/app/models/legacy_diff_note.rb +++ b/app/models/legacy_diff_note.rb @@ -20,11 +20,7 @@ class LegacyDiffNote < Note end def project_repository - if RequestStore.active? - RequestStore.fetch("project:#{project_id}:repository") { self.project.repository } - else - self.project.repository - end + Gitlab::SafeRequestStore.fetch("project:#{project_id}:repository") { self.project.repository } end def diff_file_hash diff --git a/app/models/project.rb b/app/models/project.rb index 3e14064a556..b18efa41a33 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -2272,11 +2272,7 @@ class Project < ActiveRecord::Base end end - if RequestStore.active? - RequestStore.fetch("project-#{id}:branch-#{branch_name}:user-#{user.id}:branch_allows_collaboration") do - check_access.call - end - else + Gitlab::SafeRequestStore.fetch("project-#{id}:branch-#{branch_name}:user-#{user.id}:branch_allows_collaboration") do check_access.call end end diff --git a/lib/banzai/filter/external_issue_reference_filter.rb b/lib/banzai/filter/external_issue_reference_filter.rb index b4a7a44e109..8159dcfed72 100644 --- a/lib/banzai/filter/external_issue_reference_filter.rb +++ b/lib/banzai/filter/external_issue_reference_filter.rb @@ -97,9 +97,7 @@ module Banzai private def external_issues_cached(attribute) - return project.public_send(attribute) unless RequestStore.active? # rubocop:disable GitlabSecurity/PublicSend - - cached_attributes = RequestStore[:banzai_external_issues_tracker_attributes] ||= Hash.new { |h, k| h[k] = {} } + cached_attributes = Gitlab::SafeRequestStore[:banzai_external_issues_tracker_attributes] ||= Hash.new { |h, k| h[k] = {} } cached_attributes[project.id][attribute] = project.public_send(attribute) if cached_attributes[project.id][attribute].nil? # rubocop:disable GitlabSecurity/PublicSend cached_attributes[project.id][attribute] end diff --git a/lib/feature.rb b/lib/feature.rb index f4b57376313..a8324d99c10 100644 --- a/lib/feature.rb +++ b/lib/feature.rb @@ -28,11 +28,7 @@ class Feature end def persisted_names - if RequestStore.active? - RequestStore[:flipper_persisted_names] ||= FlipperFeature.feature_names - else - FlipperFeature.feature_names - end + Gitlab::SafeRequestStore[:flipper_persisted_names] ||= FlipperFeature.feature_names end def persisted?(feature) @@ -76,8 +72,8 @@ class Feature end def flipper - if RequestStore.active? - RequestStore[:flipper] ||= build_flipper_instance + if Gitlab::SafeRequestStore.active? + Gitlab::SafeRequestStore[:flipper] ||= build_flipper_instance else @flipper ||= build_flipper_instance end diff --git a/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb index c9dbd2d2e5f..de7c959e706 100644 --- a/lib/gitlab/current_settings.rb +++ b/lib/gitlab/current_settings.rb @@ -2,11 +2,7 @@ module Gitlab module CurrentSettings class << self def current_application_settings - if RequestStore.active? - RequestStore.fetch(:current_application_settings) { ensure_application_settings! } - else - ensure_application_settings! - end + Gitlab::SafeRequestStore.fetch(:current_application_settings) { ensure_application_settings! } end def fake_application_settings(attributes = {}) diff --git a/lib/gitlab/diff/position.rb b/lib/gitlab/diff/position.rb index 4b6016e00bc..fc280f96ec1 100644 --- a/lib/gitlab/diff/position.rb +++ b/lib/gitlab/diff/position.rb @@ -101,18 +101,14 @@ module Gitlab return @diff_file if defined?(@diff_file) @diff_file = begin - if RequestStore.active? - key = { - project_id: repository.project.id, - start_sha: start_sha, - head_sha: head_sha, - path: file_path - } - - RequestStore.fetch(key) { find_diff_file(repository) } - else - find_diff_file(repository) - end + key = { + project_id: repository.project.id, + start_sha: start_sha, + head_sha: head_sha, + path: file_path + } + + Gitlab::SafeRequestStore.fetch(key) { find_diff_file(repository) } end end diff --git a/lib/gitlab/git/wiki.rb b/lib/gitlab/git/wiki.rb index ae92a624e05..d2dc4f2e688 100644 --- a/lib/gitlab/git/wiki.rb +++ b/lib/gitlab/git/wiki.rb @@ -115,11 +115,7 @@ module Gitlab def version(commit_id) commit_find_proc = -> { Gitlab::Git::Commit.find(@repository, commit_id) } - if RequestStore.active? - RequestStore.fetch([:wiki_version_commit, commit_id]) { commit_find_proc.call } - else - commit_find_proc.call - end + Gitlab::SafeRequestStore.fetch([:wiki_version_commit, commit_id]) { commit_find_proc.call } end def assert_type!(object, klass) diff --git a/lib/gitlab/issuables_count_for_state.rb b/lib/gitlab/issuables_count_for_state.rb index 505810964bc..b5657a36998 100644 --- a/lib/gitlab/issuables_count_for_state.rb +++ b/lib/gitlab/issuables_count_for_state.rb @@ -1,7 +1,7 @@ module Gitlab # Class for counting and caching the number of issuables per state. class IssuablesCountForState - # The name of the RequestStore cache key. + # The name of the Gitlab::SafeRequestStore cache key. CACHE_KEY = :issuables_count_for_state # The state values that can be safely casted to a Symbol. @@ -10,12 +10,7 @@ module Gitlab # finder - The finder class to use for retrieving the issuables. def initialize(finder) @finder = finder - @cache = - if RequestStore.active? - RequestStore[CACHE_KEY] ||= initialize_cache - else - initialize_cache - end + @cache = Gitlab::SafeRequestStore[CACHE_KEY] ||= initialize_cache end def for_state_or_opened(state = nil) diff --git a/lib/gitlab/performance_bar/peek_query_tracker.rb b/lib/gitlab/performance_bar/peek_query_tracker.rb index f2825db59ae..37ff32b1296 100644 --- a/lib/gitlab/performance_bar/peek_query_tracker.rb +++ b/lib/gitlab/performance_bar/peek_query_tracker.rb @@ -23,7 +23,7 @@ module Gitlab end subscribe('sql.active_record') do |_, start, finish, _, data| - if RequestStore.active? && RequestStore.store[:peek_enabled] + if Gitlab::SafeRequestStore.store[:peek_enabled] # data[:cached] is only available starting from Rails 5.1.0 # https://github.com/rails/rails/blob/v5.1.0/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb#L113 # Before that, data[:name] was set to 'CACHE' -- cgit v1.2.1 From 22bf3848ef0e59fb7689bfeab3ba0d8079f1597e Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Thu, 20 Sep 2018 12:38:00 -0700 Subject: Add note to docs about `Gitlab::SafeRequestStore` --- doc/development/merge_request_performance_guidelines.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/doc/development/merge_request_performance_guidelines.md b/doc/development/merge_request_performance_guidelines.md index 12badbe39b2..ee01c89e0ed 100644 --- a/doc/development/merge_request_performance_guidelines.md +++ b/doc/development/merge_request_performance_guidelines.md @@ -168,6 +168,7 @@ user objects for every username we can remove the need for running the same query for every mention of `@alice`. Caching data per transaction can be done using -[RequestStore](https://github.com/steveklabnik/request_store). Caching data in -Redis can be done using [Rails' caching +[RequestStore](https://github.com/steveklabnik/request_store) (use +`Gitlab::SafeRequestStore` to avoid having to remember to check +`RequestStore.active?`). Caching data in Redis can be done using [Rails' caching system](http://guides.rubyonrails.org/caching_with_rails.html). -- cgit v1.2.1 From a54a5d9f39df505fe7c68e14c693553bd29bd725 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Thu, 20 Sep 2018 15:40:15 -0700 Subject: Use `Gitlab::SafeRequestStore` in more places MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Even if it doesn’t save lines of code, since people will tend to use code they’ve seen. And `SafeRequestStore` is safer since you don’t have to remember to check `RequestStore.active?`. --- app/models/ability.rb | 2 +- app/models/concerns/bulk_member_access_load.rb | 6 +-- app/models/namespace.rb | 4 +- lib/banzai/filter/abstract_reference_filter.rb | 4 +- lib/banzai/reference_parser/base_parser.rb | 4 +- lib/banzai/request_store_reference_cache.rb | 4 +- lib/gitlab/cache/request_cache.rb | 4 +- lib/gitlab/favicon.rb | 2 +- lib/gitlab/git/hook_env.rb | 10 ++--- lib/gitlab/git/storage/circuit_breaker.rb | 2 +- lib/gitlab/git/storage/failure_info.rb | 2 +- lib/gitlab/gitaly_client.rb | 48 +++++++++++----------- lib/gitlab/gitaly_client/commit_service.rb | 13 +++--- lib/gitlab/logger.rb | 2 +- lib/gitlab/request_context.rb | 4 +- lib/gitlab/temporarily_allow.rb | 6 +-- .../filter/external_issue_reference_filter_spec.rb | 6 +-- .../banzai/reference_parser/base_parser_spec.rb | 3 +- spec/lib/gitlab/git/hook_env_spec.rb | 20 +++------ spec/models/commit_spec.rb | 2 +- 20 files changed, 67 insertions(+), 81 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index a853106e5bd..1466407d0d1 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -74,7 +74,7 @@ class Ability end def policy_for(user, subject = :global) - cache = RequestStore.active? ? RequestStore : {} + cache = Gitlab::SafeRequestStore.active? ? Gitlab::SafeRequestStore : {} DeclarativePolicy.policy_for(user, subject, cache: cache) end diff --git a/app/models/concerns/bulk_member_access_load.rb b/app/models/concerns/bulk_member_access_load.rb index c4346d5dd17..041ed3755e0 100644 --- a/app/models/concerns/bulk_member_access_load.rb +++ b/app/models/concerns/bulk_member_access_load.rb @@ -16,9 +16,9 @@ module BulkMemberAccessLoad key = max_member_access_for_resource_key(resource_klass, memoization_index) access = {} - if RequestStore.active? - RequestStore.store[key] ||= {} - access = RequestStore.store[key] + if Gitlab::SafeRequestStore.active? + Gitlab::SafeRequestStore[key] ||= {} + access = Gitlab::SafeRequestStore[key] end # Look up only the IDs we need diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 0289f29211d..c54be778d1b 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -148,8 +148,8 @@ class Namespace < ActiveRecord::Base def find_fork_of(project) return nil unless project.fork_network - if RequestStore.active? - forks_in_namespace = RequestStore.fetch("namespaces:#{id}:forked_projects") do + if Gitlab::SafeRequestStore.active? + forks_in_namespace = Gitlab::SafeRequestStore.fetch("namespaces:#{id}:forked_projects") do Hash.new do |found_forks, project| found_forks[project] = project.fork_network.find_forks_in(projects).first end diff --git a/lib/banzai/filter/abstract_reference_filter.rb b/lib/banzai/filter/abstract_reference_filter.rb index ad0806df8e6..4764f8e1e19 100644 --- a/lib/banzai/filter/abstract_reference_filter.rb +++ b/lib/banzai/filter/abstract_reference_filter.rb @@ -296,7 +296,7 @@ module Banzai # Returns projects for the given paths. def find_for_paths(paths) - if RequestStore.active? + if Gitlab::SafeRequestStore.active? cache = refs_cache to_query = paths - cache.keys @@ -340,7 +340,7 @@ module Banzai end def refs_cache - RequestStore["banzai_#{parent_type}_refs".to_sym] ||= {} + Gitlab::SafeRequestStore["banzai_#{parent_type}_refs".to_sym] ||= {} end def parent_type diff --git a/lib/banzai/reference_parser/base_parser.rb b/lib/banzai/reference_parser/base_parser.rb index 68752f5bb5a..3ab154a7b1c 100644 --- a/lib/banzai/reference_parser/base_parser.rb +++ b/lib/banzai/reference_parser/base_parser.rb @@ -166,7 +166,7 @@ module Banzai # objects that have not yet been queried. For objects that have already # been queried the object is returned from the cache. def collection_objects_for_ids(collection, ids) - if RequestStore.active? + if Gitlab::SafeRequestStore.active? ids = ids.map(&:to_i) cache = collection_cache[collection_cache_key(collection)] to_query = ids - cache.keys @@ -248,7 +248,7 @@ module Banzai end def collection_cache - RequestStore[:banzai_collection_cache] ||= Hash.new do |hash, key| + Gitlab::SafeRequestStore[:banzai_collection_cache] ||= Hash.new do |hash, key| hash[key] = {} end end diff --git a/lib/banzai/request_store_reference_cache.rb b/lib/banzai/request_store_reference_cache.rb index 426131442a2..9a9704f9837 100644 --- a/lib/banzai/request_store_reference_cache.rb +++ b/lib/banzai/request_store_reference_cache.rb @@ -1,8 +1,8 @@ module Banzai module RequestStoreReferenceCache def cached_call(request_store_key, cache_key, path: []) - if RequestStore.active? - cache = RequestStore[request_store_key] ||= Hash.new do |hash, key| + if Gitlab::SafeRequestStore.active? + cache = Gitlab::SafeRequestStore[request_store_key] ||= Hash.new do |hash, key| hash[key] = Hash.new { |h, k| h[k] = {} } end diff --git a/lib/gitlab/cache/request_cache.rb b/lib/gitlab/cache/request_cache.rb index 671b8e7e1b1..b96e161a5b6 100644 --- a/lib/gitlab/cache/request_cache.rb +++ b/lib/gitlab/cache/request_cache.rb @@ -26,8 +26,8 @@ module Gitlab define_method(method_name) do |*args| store = - if RequestStore.active? - RequestStore.store + if Gitlab::SafeRequestStore.active? + Gitlab::SafeRequestStore.store else ivar_name = # ! and ? cannot be used as ivar name "@cache_#{method_name.to_s.tr('!?', "\u2605\u2606")}" diff --git a/lib/gitlab/favicon.rb b/lib/gitlab/favicon.rb index 4850a6c0430..050a1ad3a0b 100644 --- a/lib/gitlab/favicon.rb +++ b/lib/gitlab/favicon.rb @@ -47,7 +47,7 @@ module Gitlab end def appearance - RequestStore.store[:appearance] ||= (Appearance.current || Appearance.new) + Gitlab::SafeRequestStore[:appearance] ||= (Appearance.current || Appearance.new) end def appearance_favicon diff --git a/lib/gitlab/git/hook_env.rb b/lib/gitlab/git/hook_env.rb index 455e8451c10..620568d8817 100644 --- a/lib/gitlab/git/hook_env.rb +++ b/lib/gitlab/git/hook_env.rb @@ -17,18 +17,18 @@ module Gitlab ].freeze def self.set(gl_repository, env) - return unless RequestStore.active? + return unless Gitlab::SafeRequestStore.active? raise "missing gl_repository" if gl_repository.blank? - RequestStore.store[:gitlab_git_env] ||= {} - RequestStore.store[:gitlab_git_env][gl_repository] = whitelist_git_env(env) + Gitlab::SafeRequestStore[:gitlab_git_env] ||= {} + Gitlab::SafeRequestStore[:gitlab_git_env][gl_repository] = whitelist_git_env(env) end def self.all(gl_repository) - return {} unless RequestStore.active? + return {} unless Gitlab::SafeRequestStore.active? - h = RequestStore.fetch(:gitlab_git_env) { {} } + h = Gitlab::SafeRequestStore.fetch(:gitlab_git_env) { {} } h.fetch(gl_repository, {}) end diff --git a/lib/gitlab/git/storage/circuit_breaker.rb b/lib/gitlab/git/storage/circuit_breaker.rb index 62427ac9cc4..fcee9ae566c 100644 --- a/lib/gitlab/git/storage/circuit_breaker.rb +++ b/lib/gitlab/git/storage/circuit_breaker.rb @@ -11,7 +11,7 @@ module Gitlab to: :failure_info def self.for_storage(storage) - cached_circuitbreakers = RequestStore.fetch(:circuitbreaker_cache) do + cached_circuitbreakers = Gitlab::SafeRequestStore.fetch(:circuitbreaker_cache) do Hash.new do |hash, storage_name| hash[storage_name] = build(storage_name) end diff --git a/lib/gitlab/git/storage/failure_info.rb b/lib/gitlab/git/storage/failure_info.rb index 387279c110d..1d28a850049 100644 --- a/lib/gitlab/git/storage/failure_info.rb +++ b/lib/gitlab/git/storage/failure_info.rb @@ -10,7 +10,7 @@ module Gitlab redis.del(*all_storage_keys) unless all_storage_keys.empty? end - RequestStore.delete(:circuitbreaker_cache) + Gitlab::SafeRequestStore.delete(:circuitbreaker_cache) end def self.load(cache_key) diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb index 12307338972..a7b67b2786d 100644 --- a/lib/gitlab/gitaly_client.rb +++ b/lib/gitlab/gitaly_client.rb @@ -302,7 +302,7 @@ module Gitlab # Ensures that Gitaly is not being abuse through n+1 misuse etc def self.enforce_gitaly_request_limits(call_site) # Only count limits in request-response environments (not sidekiq for example) - return unless RequestStore.active? + return unless Gitlab::SafeRequestStore.active? # This is this actual number of times this call was made. Used for information purposes only actual_call_count = increment_call_count("gitaly_#{call_site}_actual") @@ -326,7 +326,7 @@ module Gitlab end def self.allow_n_plus_1_calls - return yield unless RequestStore.active? + return yield unless Gitlab::SafeRequestStore.active? begin increment_call_count(:gitaly_call_count_exception_block_depth) @@ -337,25 +337,25 @@ module Gitlab end def self.get_call_count(key) - RequestStore.store[key] || 0 + Gitlab::SafeRequestStore[key] || 0 end private_class_method :get_call_count def self.increment_call_count(key) - RequestStore.store[key] ||= 0 - RequestStore.store[key] += 1 + Gitlab::SafeRequestStore[key] ||= 0 + Gitlab::SafeRequestStore[key] += 1 end private_class_method :increment_call_count def self.decrement_call_count(key) - RequestStore.store[key] -= 1 + Gitlab::SafeRequestStore[key] -= 1 end private_class_method :decrement_call_count # Returns an estimate of the number of Gitaly calls made for this # request def self.get_request_count - return 0 unless RequestStore.active? + return 0 unless Gitlab::SafeRequestStore.active? gitaly_migrate_count = get_call_count("gitaly_migrate_actual") gitaly_call_count = get_call_count("gitaly_call_actual") @@ -372,28 +372,28 @@ module Gitlab end def self.reset_counts - return unless RequestStore.active? + return unless Gitlab::SafeRequestStore.active? %w[migrate call].each do |call_site| - RequestStore.store["gitaly_#{call_site}_actual"] = 0 - RequestStore.store["gitaly_#{call_site}_permitted"] = 0 + Gitlab::SafeRequestStore["gitaly_#{call_site}_actual"] = 0 + Gitlab::SafeRequestStore["gitaly_#{call_site}_permitted"] = 0 end end def self.add_call_details(details) id = details.delete(:id) - return unless id && RequestStore.active? && RequestStore.store[:peek_enabled] + return unless id && Gitlab::SafeRequestStore[:peek_enabled] - RequestStore.store['gitaly_call_details'] ||= {} - RequestStore.store['gitaly_call_details'][id] ||= {} - RequestStore.store['gitaly_call_details'][id].merge!(details) + Gitlab::SafeRequestStore['gitaly_call_details'] ||= {} + Gitlab::SafeRequestStore['gitaly_call_details'][id] ||= {} + Gitlab::SafeRequestStore['gitaly_call_details'][id].merge!(details) end def self.list_call_details - return {} unless RequestStore.active? && RequestStore.store[:peek_enabled] + return {} unless Gitlab::SafeRequestStore[:peek_enabled] - RequestStore.store['gitaly_call_details'] || {} + Gitlab::SafeRequestStore['gitaly_call_details'] || {} end def self.expected_server_version @@ -431,22 +431,22 @@ module Gitlab # Count a stack. Used for n+1 detection def self.count_stack - return unless RequestStore.active? + return unless Gitlab::SafeRequestStore.active? stack_string = Gitlab::Profiler.clean_backtrace(caller).drop(1).join("\n") - RequestStore.store[:stack_counter] ||= Hash.new + Gitlab::SafeRequestStore[:stack_counter] ||= Hash.new - count = RequestStore.store[:stack_counter][stack_string] || 0 - RequestStore.store[:stack_counter][stack_string] = count + 1 + count = Gitlab::SafeRequestStore[:stack_counter][stack_string] || 0 + Gitlab::SafeRequestStore[:stack_counter][stack_string] = count + 1 end private_class_method :count_stack # Returns a count for the stack which called Gitaly the most times. Used for n+1 detection def self.max_call_count - return 0 unless RequestStore.active? + return 0 unless Gitlab::SafeRequestStore.active? - stack_counter = RequestStore.store[:stack_counter] + stack_counter = Gitlab::SafeRequestStore[:stack_counter] return 0 unless stack_counter stack_counter.values.max @@ -455,9 +455,9 @@ module Gitlab # Returns the stacks that calls Gitaly the most times. Used for n+1 detection def self.max_stacks - return nil unless RequestStore.active? + return nil unless Gitlab::SafeRequestStore.active? - stack_counter = RequestStore.store[:stack_counter] + stack_counter = Gitlab::SafeRequestStore[:stack_counter] return nil unless stack_counter max = max_call_count diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb index 6c95abdcb4b..07e5e204b68 100644 --- a/lib/gitlab/gitaly_client/commit_service.rb +++ b/lib/gitlab/gitaly_client/commit_service.rb @@ -240,22 +240,23 @@ module Gitlab end def find_commit(revision) - if RequestStore.active? - # We don't use RequeStstore.fetch(key) { ... } directly because `revision` - # can be a branch name, so we can't use it as a key as it could point - # to another commit later on (happens a lot in tests). + if Gitlab::SafeRequestStore.active? + # We don't use Gitlab::SafeRequestStore.fetch(key) { ... } directly + # because `revision` can be a branch name, so we can't use it as a key + # as it could point to another commit later on (happens a lot in + # tests). key = { storage: @gitaly_repo.storage_name, relative_path: @gitaly_repo.relative_path, commit_id: revision } - return RequestStore[key] if RequestStore.exist?(key) + return Gitlab::SafeRequestStore[key] if Gitlab::SafeRequestStore.exist?(key) commit = call_find_commit(revision) return unless commit key[:commit_id] = commit.id - RequestStore[key] = commit + Gitlab::SafeRequestStore[key] = commit else call_find_commit(revision) end diff --git a/lib/gitlab/logger.rb b/lib/gitlab/logger.rb index e58927a40b9..3d7c049c17f 100644 --- a/lib/gitlab/logger.rb +++ b/lib/gitlab/logger.rb @@ -30,7 +30,7 @@ module Gitlab end def self.build - RequestStore[self.cache_key] ||= new(self.full_log_path) + Gitlab::SafeRequestStore[self.cache_key] ||= new(self.full_log_path) end def self.full_log_path diff --git a/lib/gitlab/request_context.rb b/lib/gitlab/request_context.rb index fef536ecb0b..8562d4515aa 100644 --- a/lib/gitlab/request_context.rb +++ b/lib/gitlab/request_context.rb @@ -2,7 +2,7 @@ module Gitlab class RequestContext class << self def client_ip - RequestStore[:client_ip] + Gitlab::SafeRequestStore[:client_ip] end end @@ -13,7 +13,7 @@ module Gitlab def call(env) req = Rack::Request.new(env) - RequestStore[:client_ip] = req.ip + Gitlab::SafeRequestStore[:client_ip] = req.ip @app.call(env) end diff --git a/lib/gitlab/temporarily_allow.rb b/lib/gitlab/temporarily_allow.rb index 880e55f71df..8d7dacc6c54 100644 --- a/lib/gitlab/temporarily_allow.rb +++ b/lib/gitlab/temporarily_allow.rb @@ -10,7 +10,7 @@ module Gitlab end def temporarily_allowed?(key) - if RequestStore.active? + if Gitlab::SafeRequestStore.active? temporarily_allow_request_store[key] > 0 else TEMPORARILY_ALLOW_MUTEX.synchronize do @@ -26,11 +26,11 @@ module Gitlab end def temporarily_allow_request_store - RequestStore[:temporarily_allow] ||= Hash.new(0) + Gitlab::SafeRequestStore[:temporarily_allow] ||= Hash.new(0) end def temporarily_allow_add(key, value) - if RequestStore.active? + if Gitlab::SafeRequestStore.active? temporarily_allow_request_store[key] += value else TEMPORARILY_ALLOW_MUTEX.synchronize do diff --git a/spec/lib/banzai/filter/external_issue_reference_filter_spec.rb b/spec/lib/banzai/filter/external_issue_reference_filter_spec.rb index d9018a7e4fe..0d0554a2259 100644 --- a/spec/lib/banzai/filter/external_issue_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/external_issue_reference_filter_spec.rb @@ -79,13 +79,9 @@ describe Banzai::Filter::ExternalIssueReferenceFilter do expect(link).to eq helper.url_for_issue(issue_id, project, only_path: true) end - context 'with RequestStore enabled' do + context 'with RequestStore enabled', :request_store do let(:reference_filter) { HTML::Pipeline.new([described_class]) } - before do - allow(RequestStore).to receive(:active?).and_return(true) - end - it 'queries the collection on the first call' do expect_any_instance_of(Project).to receive(:default_issues_tracker?).once.and_call_original expect_any_instance_of(Project).to receive(:external_issue_reference_pattern).once.and_call_original diff --git a/spec/lib/banzai/reference_parser/base_parser_spec.rb b/spec/lib/banzai/reference_parser/base_parser_spec.rb index 4e6e8eca38a..c6e9fc414a1 100644 --- a/spec/lib/banzai/reference_parser/base_parser_spec.rb +++ b/spec/lib/banzai/reference_parser/base_parser_spec.rb @@ -263,11 +263,10 @@ describe Banzai::ReferenceParser::BaseParser do end end - context 'with RequestStore enabled' do + context 'with RequestStore enabled', :request_store do before do cache = Hash.new { |hash, key| hash[key] = {} } - allow(RequestStore).to receive(:active?).and_return(true) allow(subject).to receive(:collection_cache).and_return(cache) end diff --git a/spec/lib/gitlab/git/hook_env_spec.rb b/spec/lib/gitlab/git/hook_env_spec.rb index e6aa5ad8c90..5e49ea6da7a 100644 --- a/spec/lib/gitlab/git/hook_env_spec.rb +++ b/spec/lib/gitlab/git/hook_env_spec.rb @@ -4,11 +4,7 @@ describe Gitlab::Git::HookEnv do let(:gl_repository) { 'project-123' } describe ".set" do - context 'with RequestStore.store disabled' do - before do - allow(RequestStore).to receive(:active?).and_return(false) - end - + context 'with RequestStore disabled' do it 'does not store anything' do described_class.set(gl_repository, GIT_OBJECT_DIRECTORY_RELATIVE: 'foo') @@ -16,11 +12,7 @@ describe Gitlab::Git::HookEnv do end end - context 'with RequestStore.store enabled' do - before do - allow(RequestStore).to receive(:active?).and_return(true) - end - + context 'with RequestStore enabled', :request_store do it 'whitelist some `GIT_*` variables and stores them using RequestStore' do described_class.set( gl_repository, @@ -41,9 +33,8 @@ describe Gitlab::Git::HookEnv do end describe ".all" do - context 'with RequestStore.store enabled' do + context 'with RequestStore enabled', :request_store do before do - allow(RequestStore).to receive(:active?).and_return(true) described_class.set( gl_repository, GIT_OBJECT_DIRECTORY_RELATIVE: 'foo', @@ -60,7 +51,7 @@ describe Gitlab::Git::HookEnv do end describe ".to_env_hash" do - context 'with RequestStore.store enabled' do + context 'with RequestStore enabled', :request_store do using RSpec::Parameterized::TableSyntax let(:key) { 'GIT_OBJECT_DIRECTORY_RELATIVE' } @@ -76,7 +67,6 @@ describe Gitlab::Git::HookEnv do with_them do before do - allow(RequestStore).to receive(:active?).and_return(true) described_class.set(gl_repository, key.to_sym => input) end @@ -92,7 +82,7 @@ describe Gitlab::Git::HookEnv do end describe 'thread-safety' do - context 'with RequestStore.store enabled' do + context 'with RequestStore enabled', :request_store do before do allow(RequestStore).to receive(:active?).and_return(true) described_class.set(gl_repository, GIT_OBJECT_DIRECTORY_RELATIVE: 'foo') diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index d5f88e930d4..a6957095166 100644 --- a/spec/models/commit_spec.rb +++ b/spec/models/commit_spec.rb @@ -65,7 +65,7 @@ describe Commit do key = "Commit:author:#{commit.author_email.downcase}" - expect(RequestStore.store[key]).to eq(user) + expect(Gitlab::SafeRequestStore[key]).to eq(user) expect(commit.author).to eq(user) end -- cgit v1.2.1 From 74ae135888f55c17f6c53bcba9c1ca979a33724e Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Mon, 24 Sep 2018 12:29:22 -0700 Subject: Refactor Feature.flipper method * Fix typo in context 'when request store is active' * Rearrange test since the instance variable always gets set now, even if RequestStore is active --- lib/feature.rb | 6 +----- spec/lib/feature_spec.rb | 20 +++++++++++--------- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/lib/feature.rb b/lib/feature.rb index a8324d99c10..0e90ad9a333 100644 --- a/lib/feature.rb +++ b/lib/feature.rb @@ -72,11 +72,7 @@ class Feature end def flipper - if Gitlab::SafeRequestStore.active? - Gitlab::SafeRequestStore[:flipper] ||= build_flipper_instance - else - @flipper ||= build_flipper_instance - end + @flipper ||= (Gitlab::SafeRequestStore[:flipper] ||= build_flipper_instance) end def build_flipper_instance diff --git a/spec/lib/feature_spec.rb b/spec/lib/feature_spec.rb index 48c0ba8a653..9d56c62ae57 100644 --- a/spec/lib/feature_spec.rb +++ b/spec/lib/feature_spec.rb @@ -91,7 +91,11 @@ describe Feature do end describe '.flipper' do - shared_examples 'a memoized Flipper instance' do + before do + described_class.instance_variable_set(:@flipper, nil) + end + + context 'when request store is inactive' do it 'memoizes the Flipper instance' do expect(Flipper).to receive(:new).once.and_call_original @@ -101,16 +105,14 @@ describe Feature do end end - context 'when request store is inactive' do - before do + context 'when request store is active', :request_store do + it 'memoizes the Flipper instance' do + expect(Flipper).to receive(:new).once.and_call_original + + described_class.flipper described_class.instance_variable_set(:@flipper, nil) + described_class.flipper end - - it_behaves_like 'a memoized Flipper instance' - end - - context 'when request store is inactive', :request_store do - it_behaves_like 'a memoized Flipper instance' end end -- cgit v1.2.1