diff options
author | Michael Kozono <mkozono@gmail.com> | 2018-09-25 10:12:51 -0700 |
---|---|---|
committer | Michael Kozono <mkozono@gmail.com> | 2018-09-27 18:22:37 -0700 |
commit | 3640292bf2ef822c8e2394fa2397b1b7d04e9891 (patch) | |
tree | e6083dca766e7acfb94b613329a43c98a8aa526b | |
parent | d9c4ebc5a0b2e911f17865e482de1dfcc2189ac3 (diff) | |
download | gitlab-ce-3640292bf2ef822c8e2394fa2397b1b7d04e9891.tar.gz |
Cache `Repository#exists?` false in RequestStore
* Only truthy values are cached in Redis.
* All values are cached in RequestStore and in an instance variable.
-rw-r--r-- | app/models/repository.rb | 6 | ||||
-rw-r--r-- | lib/gitlab/repository_cache.rb | 16 | ||||
-rw-r--r-- | lib/gitlab/repository_cache_adapter.rb | 36 | ||||
-rw-r--r-- | spec/controllers/projects/pipelines_controller_spec.rb | 5 | ||||
-rw-r--r-- | spec/lib/gitlab/repository_cache_adapter_spec.rb | 80 | ||||
-rw-r--r-- | spec/lib/gitlab/repository_cache_spec.rb | 85 | ||||
-rw-r--r-- | spec/models/repository_spec.rb | 43 |
7 files changed, 269 insertions, 2 deletions
diff --git a/app/models/repository.rb b/app/models/repository.rb index 7a6dfada917..4fecdb3c1ad 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -510,7 +510,7 @@ class Repository raw_repository.exists? end - cache_method :exists? + cache_method_asymmetrically :exists? # We don't need to cache the output of this method because both exists? and # has_visible_content? are already memoized and cached. There's no guarantee @@ -1029,6 +1029,10 @@ class Repository @cache ||= Gitlab::RepositoryCache.new(self) end + def request_store_cache + @request_store_cache ||= Gitlab::RepositoryCache.new(self, backend: Gitlab::SafeRequestStore) + end + def tags_sorted_by_committed_date tags.sort_by do |tag| # Annotated tags can point to any object (e.g. a blob), but generally diff --git a/lib/gitlab/repository_cache.rb b/lib/gitlab/repository_cache.rb index b1bf3ca4143..a03ce07b6a1 100644 --- a/lib/gitlab/repository_cache.rb +++ b/lib/gitlab/repository_cache.rb @@ -29,5 +29,21 @@ module Gitlab def read(key) backend.read(cache_key(key)) end + + def write(key, value) + backend.write(cache_key(key), value) + end + + def fetch_without_caching_false(key, &block) + value = read(key) + return value if value + + value = yield + + # Don't cache false values + write(key, value) if value + + value + end end end diff --git a/lib/gitlab/repository_cache_adapter.rb b/lib/gitlab/repository_cache_adapter.rb index f7318f81613..bd0e51cbfe5 100644 --- a/lib/gitlab/repository_cache_adapter.rb +++ b/lib/gitlab/repository_cache_adapter.rb @@ -15,6 +15,20 @@ module Gitlab wrap_method(name, :cache_method_output, fallback: fallback) end + # Caches truthy values from the method. All values are strongly memoized, + # and cached in RequestStore. + # + # Currently only used to cache `exists?` since stale false values are + # particularly troublesome. This can occur, for example, when an NFS mount + # is temporarily down. + # + # This only works for methods that do not take any arguments. + # + # name - The name of the method to be cached. + def cache_method_asymmetrically(name) + wrap_method(name, :cache_method_output_asymmetrically) + end + # Strongly memoizes the method. # # This only works for methods that do not take any arguments. @@ -42,6 +56,12 @@ module Gitlab end end + # RequestStore-backed RepositoryCache to be used. Should be overridden by + # the including class + def request_store_cache + raise NotImplementedError + end + # RepositoryCache to be used. Should be overridden by the including class def cache raise NotImplementedError @@ -63,6 +83,22 @@ module Gitlab end end + # Caches truthy values from the supplied block. All values are strongly + # memoized, and cached in RequestStore. + # + # Currently only used to cache `exists?` since stale false values are + # particularly troublesome. This can occur, for example, when an NFS mount + # is temporarily down. + # + # name - The name of the method to be cached. + def cache_method_output_asymmetrically(name, &block) + memoize_method_output(name) do + request_store_cache.fetch(name) do + cache.fetch_without_caching_false(name, &block) + end + end + end + # Strongly memoizes the supplied block. # # name - The name of the method to be memoized. diff --git a/spec/controllers/projects/pipelines_controller_spec.rb b/spec/controllers/projects/pipelines_controller_spec.rb index 0d49033c691..5c7415a318d 100644 --- a/spec/controllers/projects/pipelines_controller_spec.rb +++ b/spec/controllers/projects/pipelines_controller_spec.rb @@ -90,6 +90,11 @@ describe Projects::PipelinesController do context 'when performing gitaly calls', :request_store do it 'limits the Gitaly requests' do + # Isolate from test preparation (Repository#exists? is also cached in RequestStore) + RequestStore.end! + RequestStore.clear! + RequestStore.begin! + expect { get_pipelines_index_json } .to change { Gitlab::GitalyClient.get_request_count }.by(2) end diff --git a/spec/lib/gitlab/repository_cache_adapter_spec.rb b/spec/lib/gitlab/repository_cache_adapter_spec.rb index 703e73f1a9b..0295138fc3a 100644 --- a/spec/lib/gitlab/repository_cache_adapter_spec.rb +++ b/spec/lib/gitlab/repository_cache_adapter_spec.rb @@ -65,6 +65,86 @@ describe Gitlab::RepositoryCacheAdapter do end end + describe '#cache_method_output_asymmetrically', :use_clean_rails_memory_store_caching, :request_store do + let(:request_store_cache) { repository.send(:request_store_cache) } + + context 'with a non-existing repository' do + let(:project) { create(:project) } # No repository + let(:object) { double } + + subject do + repository.cache_method_output_asymmetrically(:cats) do + object.cats_call_stub + end + end + + it 'returns the output of the original method' do + expect(object).to receive(:cats_call_stub).and_return('output') + + expect(subject).to eq('output') + end + end + + context 'with a method throwing a non-existing-repository error' do + subject do + repository.cache_method_output_asymmetrically(:cats) do + raise Gitlab::Git::Repository::NoRepository + end + end + + it 'returns nil' do + expect(subject).to eq(nil) + end + + it 'does not cache the data' do + subject + + expect(repository.instance_variable_defined?(:@cats)).to eq(false) + expect(cache.exist?(:cats)).to eq(false) + end + end + + context 'with an existing repository' do + let(:object) { double } + + context 'when it returns truthy' do + before do + expect(object).to receive(:cats).once.and_return('truthy output') + end + + it 'caches the output in RequestStore' do + expect do + repository.cache_method_output_asymmetrically(:cats) { object.cats } + end.to change { request_store_cache.read(:cats) }.from(nil).to('truthy output') + end + + it 'caches the output in RepositoryCache' do + expect do + repository.cache_method_output_asymmetrically(:cats) { object.cats } + end.to change { cache.read(:cats) }.from(nil).to('truthy output') + end + end + + context 'when it returns false' do + before do + expect(object).to receive(:cats).once.and_return(false) + end + + it 'caches the output in RequestStore' do + expect do + repository.cache_method_output_asymmetrically(:cats) { object.cats } + end.to change { request_store_cache.read(:cats) }.from(nil).to(false) + end + + it 'does NOT cache the output in RepositoryCache' do + expect do + repository.cache_method_output_asymmetrically(:cats) { object.cats } + end.not_to change { cache.read(:cats) }.from(nil) + end + end + end + end + describe '#memoize_method_output' do let(:fallback) { 10 } diff --git a/spec/lib/gitlab/repository_cache_spec.rb b/spec/lib/gitlab/repository_cache_spec.rb index fc259cf1208..741ee12633f 100644 --- a/spec/lib/gitlab/repository_cache_spec.rb +++ b/spec/lib/gitlab/repository_cache_spec.rb @@ -47,4 +47,89 @@ describe Gitlab::RepositoryCache do expect(backend).to have_received(:fetch).with("baz:#{namespace}", &p) end end + + describe '#fetch_without_caching_false', :use_clean_rails_memory_store_caching do + let(:key) { :foo } + let(:backend) { Rails.cache } + + it 'requires a block' do + expect do + cache.fetch_without_caching_false(key) + end.to raise_error(LocalJumpError) + end + + context 'when the key does not exist in the cache' do + context 'when the result of the block is truthy' do + it 'returns the result of the block' do + result = cache.fetch_without_caching_false(key) { true } + + expect(result).to be true + end + + it 'caches the value' do + expect(backend).to receive(:write).with("#{key}:#{namespace}", true) + + cache.fetch_without_caching_false(key) { true } + end + end + + context 'when the result of the block is falsey' do + let(:p) { -> { false } } + + it 'returns the result of the block' do + result = cache.fetch_without_caching_false(key, &p) + + expect(result).to be false + end + + it 'does not cache the value' do + expect(backend).not_to receive(:write).with("#{key}:#{namespace}", true) + + cache.fetch_without_caching_false(key, &p) + end + end + end + + context 'when the cached value is truthy' do + before do + backend.write("#{key}:#{namespace}", true) + end + + it 'returns the cached value' do + result = cache.fetch_without_caching_false(key) { 'block result' } + + expect(result).to be true + end + + it 'does not execute the block' do + expect do |b| + cache.fetch_without_caching_false(key, &b) + end.not_to yield_control + end + + it 'does not write to the cache' do + expect(backend).not_to receive(:write) + + cache.fetch_without_caching_false(key) { 'block result' } + end + end + + context 'when the cached value is falsey' do + before do + backend.write("#{key}:#{namespace}", false) + end + + it 'returns the result of the block' do + result = cache.fetch_without_caching_false(key) { 'block result' } + + expect(result).to eq 'block result' + end + + it 'writes the truthy value to the cache' do + expect(backend).to receive(:write).with("#{key}:#{namespace}", 'block result') + + cache.fetch_without_caching_false(key) { 'block result' } + end + end + end end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index dffac05152b..b2869104e02 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1044,6 +1044,47 @@ describe Repository do expect_to_raise_storage_error { broken_repository.exists? } end end + + context 'asymmetric caching', :use_clean_rails_memory_store_caching, :request_store do + let(:cache) { repository.send(:cache) } + let(:request_store_cache) { repository.send(:request_store_cache) } + + context 'when it returns true' do + before do + expect(repository.raw_repository).to receive(:exists?).once.and_return(true) + end + + it 'caches the output in RequestStore' do + expect do + repository.exists? + end.to change { request_store_cache.read(:exists?) }.from(nil).to(true) + end + + it 'caches the output in RepositoryCache' do + expect do + repository.exists? + end.to change { cache.read(:exists?) }.from(nil).to(true) + end + end + + context 'when it returns false' do + before do + expect(repository.raw_repository).to receive(:exists?).once.and_return(false) + end + + it 'caches the output in RequestStore' do + expect do + repository.exists? + end.to change { request_store_cache.read(:exists?) }.from(nil).to(false) + end + + it 'does NOT cache the output in RepositoryCache' do + expect do + repository.exists? + end.not_to change { cache.read(:exists?) }.from(nil) + end + end + end end describe '#has_visible_content?' do @@ -1892,7 +1933,7 @@ describe Repository do match[1].to_sym if match end.compact - expect(methods).to match_array(Repository::CACHED_METHODS + Repository::MEMOIZED_CACHED_METHODS) + expect(Repository::CACHED_METHODS + Repository::MEMOIZED_CACHED_METHODS).to include(*methods) end end |