diff options
| author | Stan Hu <stanhu@gmail.com> | 2018-03-06 17:01:12 -0800 |
|---|---|---|
| committer | Stan Hu <stanhu@gmail.com> | 2018-03-07 06:25:21 -0800 |
| commit | 611929eb96c0ee9f07d98163d0610325d8a2a322 (patch) | |
| tree | 6dda6341c6a44c1b94d0db4d5fd915035c590c2a | |
| parent | ff00cfe45c744393de5bfcafdcfbd12108cc664f (diff) | |
| download | gitlab-ce-611929eb96c0ee9f07d98163d0610325d8a2a322.tar.gz | |
Memoize Repository#empty? instead of double caching the valuesh-remove-double-caching-repo-empty
We saw that in a customer instance, `empty?` was cached to be `true` even
though `has_visible_content?` and `exists?` were `true`. This double caching
can run into edge cases because there's no guarantee that the inner values
will properly expire the outer one, especially if there is Redis replication lag.
Consider this scenario:
1. `exists?` and `has_visible_content?` are false
2. `empty?` is expired
3. A subsequent call to `empty?` returns `true` because `exists?` is false even though `empty` is true
4. `exists?` and `has_visible_content?` are then expired
5. `exists?` and `has_visible_content?` are set to true
6. `empty?` is still stuck in the wrong value as `true`
Closes #43882
| -rw-r--r-- | app/models/repository.rb | 8 | ||||
| -rw-r--r-- | changelogs/unreleased/sh-remove-double-caching-repo-empty.yml | 5 | ||||
| -rw-r--r-- | spec/models/repository_spec.rb | 2 |
3 files changed, 10 insertions, 5 deletions
diff --git a/app/models/repository.rb b/app/models/repository.rb index 1a14afb951a..e2e2f82ccea 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -35,7 +35,7 @@ class Repository CACHED_METHODS = %i(size commit_count rendered_readme contribution_guide changelog license_blob license_key gitignore koding_yml gitlab_ci_yml branch_names tag_names branch_count - tag_count avatar exists? empty? root_ref has_visible_content? + tag_count avatar exists? root_ref has_visible_content? issue_template_names merge_request_template_names).freeze # Methods that use cache_method but only memoize the value @@ -360,7 +360,7 @@ class Repository def expire_emptiness_caches return unless empty? - expire_method_caches(%i(empty? has_visible_content?)) + expire_method_caches(%i(has_visible_content?)) end def lookup_cache @@ -506,12 +506,14 @@ class Repository end cache_method :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 + # that the values are expired and loaded atomically. def empty? return true unless exists? !has_visible_content? end - cache_method :empty? # The size of this repository in megabytes. def size diff --git a/changelogs/unreleased/sh-remove-double-caching-repo-empty.yml b/changelogs/unreleased/sh-remove-double-caching-repo-empty.yml new file mode 100644 index 00000000000..1684be4e5e3 --- /dev/null +++ b/changelogs/unreleased/sh-remove-double-caching-repo-empty.yml @@ -0,0 +1,5 @@ +--- +title: Remove double caching of Repository#empty? +merge_request: +author: +type: fixed diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 38653e18306..81fbd15b2b1 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1447,7 +1447,6 @@ describe Repository do it 'expires the caches for an empty repository' do allow(repository).to receive(:empty?).and_return(true) - expect(cache).to receive(:expire).with(:empty?) expect(cache).to receive(:expire).with(:has_visible_content?) repository.expire_emptiness_caches @@ -1456,7 +1455,6 @@ describe Repository do it 'does not expire the cache for a non-empty repository' do allow(repository).to receive(:empty?).and_return(false) - expect(cache).not_to receive(:expire).with(:empty?) expect(cache).not_to receive(:expire).with(:has_visible_content?) repository.expire_emptiness_caches |