diff options
author | Stan Hu <stanhu@gmail.com> | 2018-09-28 02:24:16 +0000 |
---|---|---|
committer | Stan Hu <stanhu@gmail.com> | 2018-09-28 02:24:16 +0000 |
commit | b0b5346d65b24bad188f3ef4a8f282ac07bf1136 (patch) | |
tree | a91ff1d8bc97f3723c95623f8b2aeba9480c21f7 | |
parent | c02057b4c877e664f16708628723509ca0b5e506 (diff) | |
parent | f2fa7c10c8490a31863c9bd21bbfd66675e3c909 (diff) | |
download | gitlab-ce-b0b5346d65b24bad188f3ef4a8f282ac07bf1136.tar.gz |
Merge branch 'mk/asymmetric-exists-cache' into 'master'
Resolve "Geo: Does not mark repositories as missing on primary due to stale cache"
Closes #50211 and #44044
See merge request gitlab-org/gitlab-ce!21789
-rw-r--r-- | app/models/repository.rb | 8 | ||||
-rw-r--r-- | changelogs/unreleased/mk-asymmetric-exists-cache.yml | 6 | ||||
-rw-r--r-- | lib/gitlab/repository_cache.rb | 16 | ||||
-rw-r--r-- | lib/gitlab/repository_cache_adapter.rb | 181 | ||||
-rw-r--r-- | spec/controllers/projects/pipelines_controller_spec.rb | 5 | ||||
-rw-r--r-- | spec/lib/gitlab/repository_cache_adapter_spec.rb | 138 | ||||
-rw-r--r-- | spec/lib/gitlab/repository_cache_spec.rb | 85 | ||||
-rw-r--r-- | spec/models/repository_spec.rb | 50 |
8 files changed, 438 insertions, 51 deletions
diff --git a/app/models/repository.rb b/app/models/repository.rb index 12fbf7d5d1d..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 @@ -612,7 +612,7 @@ class Repository Licensee::License.new(license_key) end - cache_method :license, memoize_only: true + memoize_method :license def gitignore file_on_head(:gitignore) @@ -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/changelogs/unreleased/mk-asymmetric-exists-cache.yml b/changelogs/unreleased/mk-asymmetric-exists-cache.yml new file mode 100644 index 00000000000..b6eec7d1fc6 --- /dev/null +++ b/changelogs/unreleased/mk-asymmetric-exists-cache.yml @@ -0,0 +1,6 @@ +--- +title: 'Resolve "Geo: Does not mark repositories as missing on primary due to stale + cache"' +merge_request: 21789 +author: +type: fixed 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 2ec871f0754..d95024fccf7 100644 --- a/lib/gitlab/repository_cache_adapter.rb +++ b/lib/gitlab/repository_cache_adapter.rb @@ -1,23 +1,80 @@ module Gitlab module RepositoryCacheAdapter extend ActiveSupport::Concern + include Gitlab::Utils::StrongMemoize class_methods do - # Wraps around the given method and caches its output in Redis and an instance - # variable. + # Caches and strongly memoizes the method. # # This only works for methods that do not take any arguments. - def cache_method(name, fallback: nil, memoize_only: false) - original = :"_uncached_#{name}" + # + # name - The name of the method to be cached. + # fallback - A value to fall back to if the repository does not exist, or + # in case of a Git error. Defaults to nil. + def cache_method(name, fallback: nil) + uncached_name = alias_uncached_method(name) + + define_method(name) do + cache_method_output(name, fallback: fallback) do + __send__(uncached_name) # rubocop:disable GitlabSecurity/PublicSend + end + end + end - alias_method(original, name) + # 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) + uncached_name = alias_uncached_method(name) define_method(name) do - cache_method_output(name, fallback: fallback, memoize_only: memoize_only) do - __send__(original) # rubocop:disable GitlabSecurity/PublicSend + cache_method_output_asymmetrically(name) do + __send__(uncached_name) # rubocop:disable GitlabSecurity/PublicSend end end end + + # Strongly memoizes the method. + # + # This only works for methods that do not take any arguments. + # + # name - The name of the method to be memoized. + # fallback - A value to fall back to if the repository does not exist, or + # in case of a Git error. Defaults to nil. The fallback value + # is not memoized. + def memoize_method(name, fallback: nil) + uncached_name = alias_uncached_method(name) + + define_method(name) do + memoize_method_output(name, fallback: fallback) do + __send__(uncached_name) # rubocop:disable GitlabSecurity/PublicSend + end + end + end + + # Prepends "_uncached_" to the target method name + # + # Returns the uncached method name + def alias_uncached_method(name) + uncached_name = :"_uncached_#{name}" + + alias_method(uncached_name, name) + + uncached_name + 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 @@ -30,65 +87,93 @@ module Gitlab raise NotImplementedError end - # Caches the supplied block both in a cache and in an instance variable. + # Caches and strongly memoizes the supplied block. # - # The cache key and instance variable are named the same way as the value of - # the `key` argument. + # name - The name of the method to be cached. + # fallback - A value to fall back to if the repository does not exist, or + # in case of a Git error. Defaults to nil. + def cache_method_output(name, fallback: nil, &block) + memoize_method_output(name, fallback: fallback) do + cache.fetch(name, &block) + end + end + + # Caches truthy values from the supplied block. All values are strongly + # memoized, and cached in RequestStore. # - # This method will return `nil` if the corresponding instance variable is also - # set to `nil`. This ensures we don't keep yielding the block when it returns - # `nil`. + # 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. # - # key - The name of the key to cache the data in. - # fallback - A value to fall back to in the event of a Git error. - def cache_method_output(key, fallback: nil, memoize_only: false, &block) - ivar = cache_instance_variable_name(key) - - if instance_variable_defined?(ivar) - instance_variable_get(ivar) - else - # If the repository doesn't exist and a fallback was specified we return - # that value inmediately. This saves us Rugged/gRPC invocations. - return fallback unless fallback.nil? || cache.repository.exists? - - begin - value = - if memoize_only - yield - else - cache.fetch(key, &block) - end - - instance_variable_set(ivar, value) - rescue Gitlab::Git::Repository::NoRepository - # Even if the above `#exists?` check passes these errors might still - # occur (for example because of a non-existing HEAD). We want to - # gracefully handle this and not cache anything - fallback + # 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. + # fallback - A value to fall back to if the repository does not exist, or + # in case of a Git error. Defaults to nil. The fallback value is + # not memoized. + def memoize_method_output(name, fallback: nil, &block) + no_repository_fallback(name, fallback: fallback) do + strong_memoize(memoizable_name(name), &block) + end + end + + # Returns the fallback value if the repository does not exist + def no_repository_fallback(name, fallback: nil, &block) + # Avoid unnecessary gRPC invocations + return fallback if fallback && fallback_early?(name) + + yield + rescue Gitlab::Git::Repository::NoRepository + # Even if the `#exists?` check in `fallback_early?` passes, these errors + # might still occur (for example because of a non-existing HEAD). We + # want to gracefully handle this and not memoize anything. + fallback + end + # Expires the caches of a specific set of methods def expire_method_caches(methods) - methods.each do |key| - unless cached_methods.include?(key.to_sym) - Rails.logger.error "Requested to expire non-existent method '#{key}' for Repository" + methods.each do |name| + unless cached_methods.include?(name.to_sym) + Rails.logger.error "Requested to expire non-existent method '#{name}' for Repository" next end - cache.expire(key) + cache.expire(name) - ivar = cache_instance_variable_name(key) - - remove_instance_variable(ivar) if instance_variable_defined?(ivar) + clear_memoization(memoizable_name(name)) end + + expire_request_store_method_caches(methods) end private - def cache_instance_variable_name(key) - :"@#{key.to_s.tr('?!', '')}" + def memoizable_name(name) + "#{name.to_s.tr('?!', '')}" + end + + def expire_request_store_method_caches(methods) + methods.each do |name| + request_store_cache.expire(name) + end + end + + # All cached repository methods depend on the existence of a Git repository, + # so if the repository doesn't exist, we already know not to call it. + def fallback_early?(method_name) + # Avoid infinite loop + return false if method_name == :exists? + + !exists? end end end 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 5bd4d6c6a48..0295138fc3a 100644 --- a/spec/lib/gitlab/repository_cache_adapter_spec.rb +++ b/spec/lib/gitlab/repository_cache_adapter_spec.rb @@ -65,6 +65,144 @@ 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 } + + context 'with a non-existing repository' do + let(:project) { create(:project) } # No repository + + subject do + repository.memoize_method_output(:cats, fallback: fallback) do + repository.cats_call_stub + end + end + + it 'returns the fallback value' do + expect(subject).to eq(fallback) + end + + it 'avoids calling the original method' do + expect(repository).not_to receive(:cats_call_stub) + + subject + end + + it 'does not set the instance variable' do + subject + + expect(repository.instance_variable_defined?(:@cats)).to eq(false) + end + end + + context 'with a method throwing a non-existing-repository error' do + subject do + repository.memoize_method_output(:cats, fallback: fallback) do + raise Gitlab::Git::Repository::NoRepository + end + end + + it 'returns the fallback value' do + expect(subject).to eq(fallback) + end + + it 'does not set the instance variable' do + subject + + expect(repository.instance_variable_defined?(:@cats)).to eq(false) + end + end + + context 'with an existing repository' do + it 'sets the instance variable' do + repository.memoize_method_output(:cats, fallback: fallback) do + 'block output' + end + + expect(repository.instance_variable_get(:@cats)).to eq('block output') + end + end + end + describe '#expire_method_caches' do it 'expires the caches of the given methods' do expect(cache).to receive(:expire).with(:rendered_readme) 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..7e1b7c35517 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 @@ -1716,12 +1757,19 @@ describe Repository do describe '#expire_exists_cache' do let(:cache) { repository.send(:cache) } + let(:request_store_cache) { repository.send(:request_store_cache) } it 'expires the cache' do expect(cache).to receive(:expire).with(:exists?) repository.expire_exists_cache end + + it 'expires the request store cache', :request_store do + expect(request_store_cache).to receive(:expire).with(:exists?) + + repository.expire_exists_cache + end end describe '#xcode_project?' do @@ -1892,7 +1940,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 |