From d9c4ebc5a0b2e911f17865e482de1dfcc2189ac3 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Tue, 25 Sep 2018 10:07:59 -0700 Subject: Extract `Repository.memoize_method` method And reuse `Gitlab::Utils::StrongMemoize`. There is a subtle behavior change required to reuse StrongMemoize in this case. The early fallback check now occurs *before* reading the memoized value instead of after. I think this is fine since a memoized value should only exist if `exists?` is also already memoized as `true`. --- lib/gitlab/repository_cache_adapter.rb | 120 ++++++++++++++++++++------------- 1 file changed, 73 insertions(+), 47 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/repository_cache_adapter.rb b/lib/gitlab/repository_cache_adapter.rb index 2ec871f0754..f7318f81613 100644 --- a/lib/gitlab/repository_cache_adapter.rb +++ b/lib/gitlab/repository_cache_adapter.rb @@ -1,19 +1,41 @@ 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) + # + # 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) + wrap_method(name, :cache_method_output, fallback: fallback) + 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) + wrap_method(name, :memoize_method_output, fallback: fallback) + end + + # Prepends "_uncached_" to the target method name, and redefines the method + # but wraps it in the `wrapper` method. + def wrap_method(name, wrapper, *options) original = :"_uncached_#{name}" alias_method(original, name) define_method(name) do - cache_method_output(name, fallback: fallback, memoize_only: memoize_only) do + __send__(wrapper, name, *options) do # rubocop:disable GitlabSecurity/PublicSend __send__(original) # rubocop:disable GitlabSecurity/PublicSend end end @@ -30,65 +52,69 @@ 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. - # - # 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`. + # 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 + + # Strongly memoizes the supplied block. # - # 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 - end + # 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) - - ivar = cache_instance_variable_name(key) + cache.expire(name) - remove_instance_variable(ivar) if instance_variable_defined?(ivar) + clear_memoization(memoizable_name(name)) end end private - def cache_instance_variable_name(key) - :"@#{key.to_s.tr('?!', '')}" + def memoizable_name(name) + "#{name.to_s.tr('?!', '')}" + 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 -- cgit v1.2.3 From 3640292bf2ef822c8e2394fa2397b1b7d04e9891 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Tue, 25 Sep 2018 10:12:51 -0700 Subject: Cache `Repository#exists?` false in RequestStore * Only truthy values are cached in Redis. * All values are cached in RequestStore and in an instance variable. --- lib/gitlab/repository_cache.rb | 16 +++++++++++++++ lib/gitlab/repository_cache_adapter.rb | 36 ++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+) (limited to 'lib') 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. -- cgit v1.2.3 From 87a1ba1488a6591569ecaf1131094fefd2163fd0 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Tue, 25 Sep 2018 10:13:05 -0700 Subject: Expire RequestStore cache properly --- lib/gitlab/repository_cache_adapter.rb | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'lib') diff --git a/lib/gitlab/repository_cache_adapter.rb b/lib/gitlab/repository_cache_adapter.rb index bd0e51cbfe5..160e3d13b43 100644 --- a/lib/gitlab/repository_cache_adapter.rb +++ b/lib/gitlab/repository_cache_adapter.rb @@ -136,6 +136,8 @@ module Gitlab clear_memoization(memoizable_name(name)) end + + expire_request_store_method_caches(methods) end private @@ -144,6 +146,12 @@ module Gitlab "#{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) -- cgit v1.2.3 From f2fa7c10c8490a31863c9bd21bbfd66675e3c909 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Thu, 27 Sep 2018 15:03:42 -0700 Subject: Remove send-in-send for safety and readability I attempted to refactor so that the caller of `wrap_method` passes in a block, rather than a method name, but I was unsuccessful. I kept getting the following error: NoMethodError: undefined method `cache_method_output' for Repository:Class If you can figure this out, then feel free to dry up these class methods again without doing a send-within-a-send. --- lib/gitlab/repository_cache_adapter.rb | 41 +++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 13 deletions(-) (limited to 'lib') diff --git a/lib/gitlab/repository_cache_adapter.rb b/lib/gitlab/repository_cache_adapter.rb index 160e3d13b43..d95024fccf7 100644 --- a/lib/gitlab/repository_cache_adapter.rb +++ b/lib/gitlab/repository_cache_adapter.rb @@ -12,7 +12,13 @@ module Gitlab # 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) - wrap_method(name, :cache_method_output, fallback: fallback) + 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 # Caches truthy values from the method. All values are strongly memoized, @@ -26,7 +32,13 @@ module Gitlab # # name - The name of the method to be cached. def cache_method_asymmetrically(name) - wrap_method(name, :cache_method_output_asymmetrically) + uncached_name = alias_uncached_method(name) + + define_method(name) do + cache_method_output_asymmetrically(name) do + __send__(uncached_name) # rubocop:disable GitlabSecurity/PublicSend + end + end end # Strongly memoizes the method. @@ -38,22 +50,25 @@ module Gitlab # in case of a Git error. Defaults to nil. The fallback value # is not memoized. def memoize_method(name, fallback: nil) - wrap_method(name, :memoize_method_output, fallback: fallback) - end - - # Prepends "_uncached_" to the target method name, and redefines the method - # but wraps it in the `wrapper` method. - def wrap_method(name, wrapper, *options) - original = :"_uncached_#{name}" - - alias_method(original, name) + uncached_name = alias_uncached_method(name) define_method(name) do - __send__(wrapper, name, *options) do # rubocop:disable GitlabSecurity/PublicSend - __send__(original) # rubocop:disable GitlabSecurity/PublicSend + 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 -- cgit v1.2.3