Set PerThreadCache as readonly after posting review comments

Once the review comments mutations have been completed, it is safe to
cache the results of the /meta refs lookups, avoiding the slowdown of
the repeated refs lookups performed during the creation of the event
with the added review and the dispatching of the stream event associated.

The refs lookups could be as slow as 400 ms per lookup for a 500k refs
repository on a shared NFS volume: using a thread-local cache for
/meta ref lookup has a significant improvement (50% less refs lookups)
on the overall user-experience.

Introduce also the safety-net of checking for stale cached refs
against the underlying repository, so that errors can arise
during testing. The staleness checker can be enabled via system
property.

NOTE: The execution of the listeners logic associated with the review
is left outside the read-only request window because of the risk of
a plugin or hook updating the underlying /meta ref and causing a
stale read.

Also rename the current hasReadonlyRequest method to isReadonlyRequest,
which is consistent with the new setter introduced in the PerThreadCache
class.

P.S. DO REVERT this change from stable-3.2 onwards because the
refs lookups have been optimised already with the introduction of the
cached-refdb libModule [1] which is enough to cover all use-cases
introduced in the PerThreadCache for refs lookups.

[1] https://gerrit.googlesource.com/modules/cached-refdb/

Bug: Issue 15798
Release-Notes: enable the read-only cache of /meta refs after review
Change-Id: I1b71bfcff081e66eed897d41539d2be675f34af4

8 files changed, 468 insertions, 41 deletions

diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 2fbc3e988e..8e9c64263b 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -104,6 +104,7 @@ import com.google.gerrit.server.config.CanonicalWebUrl;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.config.PluginConfigFactory;
 import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.git.RepoRefCache;
 import com.google.gerrit.server.git.meta.MetaDataUpdate;
 import com.google.gerrit.server.group.InternalGroup;
 import com.google.gerrit.server.group.SystemGroupBackend;
@@ -181,6 +182,7 @@ import org.eclipse.jgit.transport.URIish;
 import org.junit.After;
 import org.junit.AfterClass;
 import org.junit.Before;
+import org.junit.BeforeClass;
 import org.junit.Rule;
 import org.junit.rules.ExpectedException;
 import org.junit.rules.TestRule;
@@ -192,6 +194,7 @@ import org.junit.runners.model.Statement;
 public abstract class AbstractDaemonTest {
   private static GerritServer commonServer;
   private static Description firstTest;
+  private static String perThreadCacheCheckStaleEntry;
 
   @ConfigSuite.Parameter public Config baseConfig;
   @ConfigSuite.Name private String configName;
@@ -291,6 +294,12 @@ public abstract class AbstractDaemonTest {
   private ProjectResetter resetter;
   private List<Repository> toClose;
 
+  @BeforeClass
+  public static void enablePerThreadCacheStalenessCheck() {
+    perThreadCacheCheckStaleEntry =
+        System.setProperty(RepoRefCache.REPO_REF_CACHE_CHECK_STALE_ENTRIES_PROPERTY, "true");
+  }
+
   @Before
   public void clearSender() {
     sender.clear();
@@ -333,6 +342,16 @@ public abstract class AbstractDaemonTest {
     TempFileUtil.cleanup();
   }
 
+  @AfterClass
+  public static void disablePerThreadCacheStalenessCheck() {
+    if (perThreadCacheCheckStaleEntry == null) {
+      System.clearProperty(RepoRefCache.REPO_REF_CACHE_CHECK_STALE_ENTRIES_PROPERTY);
+    } else {
+      System.setProperty(
+          RepoRefCache.REPO_REF_CACHE_CHECK_STALE_ENTRIES_PROPERTY, perThreadCacheCheckStaleEntry);
+    }
+  }
+
   /** Controls which project and branches should be reset after each test case. */
   protected ProjectResetter.Config resetProjects() {
     return null;
diff --git a/java/com/google/gerrit/server/cache/PerThreadCache.java b/java/com/google/gerrit/server/cache/PerThreadCache.java
index 459650d90c..9b04561dfd 100644
--- a/java/com/google/gerrit/server/cache/PerThreadCache.java
+++ b/java/com/google/gerrit/server/cache/PerThreadCache.java
@@ -17,15 +17,19 @@ package com.google.gerrit.server.cache;
 import static com.google.common.base.Preconditions.checkState;
 
 import com.google.common.base.Objects;
+import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Maps;
 import com.google.gerrit.common.Nullable;
+import com.google.gerrit.server.git.RefCache;
+import java.util.Collection;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Optional;
 import java.util.function.Consumer;
 import java.util.function.Supplier;
-import java.util.stream.Stream;
+import java.util.stream.Collectors;
 import javax.servlet.http.HttpServletRequest;
 
 /**
@@ -63,10 +67,48 @@ public class PerThreadCache implements AutoCloseable {
   private static final int PER_THREAD_CACHE_SIZE = 25;
 
   /**
+   * System property for disabling caching specific key types. TODO: DO NOT MERGE into stable-3.2
+   * onwards.
+   */
+  public static final String PER_THREAD_CACHE_DISABLED_TYPES_PROPERTY =
+      "PerThreadCache_disabledTypes";
+
+  /**
    * True when the current thread is associated with an incoming API request that is not changing
-   * any state.
+   * any repository /meta refs and therefore caching repo refs is safe. TODO: DO NOT MERGE into
+   * stable-3.2 onwards.
+   */
+  private boolean allowRefCache;
+
+  /**
+   * Sets the request status flag to read-only temporarily. TODO: DO NOT MERGE into stable-3.2
+   * onwards.
    */
-  private final boolean readOnlyRequest;
+  public interface ReadonlyRequestWindow extends AutoCloseable {
+
+    /**
+     * Close the request read-only status, restoring the previous value.
+     *
+     * <p>NOTE: If the previous status was not read-only, the cache is getting cleared for making
+     * sure that all potential stale entries coming from a read-only windows are cleared.
+     */
+    @Override
+    default void close() {}
+  }
+
+  private class ReadonlyRequestWindowImpl implements ReadonlyRequestWindow {
+    private final boolean oldAllowRepoRefsCache;
+
+    private ReadonlyRequestWindowImpl() {
+      oldAllowRepoRefsCache = allowRefCache();
+      allowRefCache(true);
+    }
+
+    @Override
+    public void close() {
+      allowRefCache(oldAllowRepoRefsCache);
+    }
+  }
 
   private final Map<Key<?>, Consumer<Object>> unloaders =
       Maps.newHashMapWithExpectedSize(PER_THREAD_CACHE_SIZE);
@@ -186,10 +228,16 @@ public class PerThreadCache implements AutoCloseable {
   }
 
   private final Map<Key<?>, Object> cache = Maps.newHashMapWithExpectedSize(PER_THREAD_CACHE_SIZE);
+  private final ImmutableSet<String> disabledTypes;
 
-  private PerThreadCache(@Nullable HttpServletRequest req, boolean readOnly) {
-    readOnlyRequest =
-        readOnly
+  private PerThreadCache(@Nullable HttpServletRequest req, boolean alwaysCacheRepoRefs) {
+    disabledTypes =
+        ImmutableSet.copyOf(
+            Splitter.on(',')
+                .split(System.getProperty(PER_THREAD_CACHE_DISABLED_TYPES_PROPERTY, "")));
+
+    allowRefCache =
+        alwaysCacheRepoRefs
             || (req != null
                 && (req.getMethod().equalsIgnoreCase("GET")
                     || req.getMethod().equalsIgnoreCase("HEAD")));
@@ -220,6 +268,10 @@ public class PerThreadCache implements AutoCloseable {
   @SuppressWarnings("unchecked")
   public <T> Optional<T> getWithLoader(
       Key<T> key, Supplier<T> loader, @Nullable Consumer<T> unloader) {
+    if (disabledTypes.contains(key.clazz.getCanonicalName())) {
+      return Optional.empty();
+    }
+
     T value = (T) cache.get(key);
     if (value == null && cache.size() < PER_THREAD_CACHE_SIZE) {
       value = loader.get();
@@ -231,22 +283,66 @@ public class PerThreadCache implements AutoCloseable {
     return Optional.ofNullable(value);
   }
 
-  /** Returns true if the associated request is read-only */
-  public boolean hasReadonlyRequest() {
-    return readOnlyRequest;
+  /** Returns an instance of {@code T} that is already loaded from the cache or null otherwise. */
+  @SuppressWarnings("unchecked")
+  public <T> T get(Key<T> key) {
+    return (T) cache.get(key);
+  }
+
+  /**
+   * Returns true if the associated request is read-only and therefore the repo refs are safe to be
+   * cached
+   */
+  public boolean allowRefCache() {
+    return allowRefCache;
+  }
+
+  /**
+   * Set the cache read-only request status temporarily, for enabling caching of all entries.
+   *
+   * @return {@link ReadonlyRequestWindow} associated with the incoming request
+   */
+  public static ReadonlyRequestWindow openReadonlyRequestWindow() {
+    PerThreadCache perThreadCache = CACHE.get();
+    return perThreadCache == null
+        ? new ReadonlyRequestWindow() {}
+        : perThreadCache.new ReadonlyRequestWindowImpl();
   }
 
   @Override
   public void close() {
-    Optional.of(CACHE.get())
-        .map(v -> v.unloaders.entrySet().stream())
-        .orElse(Stream.empty())
-        .forEach(this::unload);
-
+    unload(unloaders.entrySet());
     CACHE.remove();
   }
 
+  private void unload(Collection<Entry<Key<?>, Consumer<Object>>> entriesToUnload) {
+    ImmutableSet<Entry<Key<?>, Consumer<Object>>> toUnload = ImmutableSet.copyOf(entriesToUnload);
+    try {
+      toUnload.stream().forEach(this::unload);
+    } finally {
+      toUnload.stream()
+          .forEach(
+              e -> {
+                cache.remove(e.getKey());
+                unloaders.remove(e.getKey());
+              });
+    }
+  }
+
   private <T> void unload(Entry<Key<?>, Consumer<Object>> unloaderEntry) {
-    unloaderEntry.getValue().accept(cache.get(unloaderEntry.getKey()));
+    Object valueToUnload = cache.get(unloaderEntry.getKey());
+    unloaderEntry.getValue().accept(valueToUnload);
+    cache.remove(unloaderEntry.getKey());
+  }
+
+  private void allowRefCache(boolean allowed) {
+    allowRefCache = allowed;
+
+    if (!allowRefCache) {
+      unload(
+          unloaders.entrySet().stream()
+              .filter(e -> RefCache.class.isAssignableFrom(e.getKey().clazz))
+              .collect(Collectors.toSet()));
+    }
   }
 }
diff --git a/java/com/google/gerrit/server/events/StreamEventsApiListener.java b/java/com/google/gerrit/server/events/StreamEventsApiListener.java
index 4f43fe60ad..272c835c0b 100644
--- a/java/com/google/gerrit/server/events/StreamEventsApiListener.java
+++ b/java/com/google/gerrit/server/events/StreamEventsApiListener.java
@@ -47,6 +47,8 @@ import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.PatchSetUtil;
+import com.google.gerrit.server.cache.PerThreadCache;
+import com.google.gerrit.server.cache.PerThreadCache.ReadonlyRequestWindow;
 import com.google.gerrit.server.data.AccountAttribute;
 import com.google.gerrit.server.data.ApprovalAttribute;
 import com.google.gerrit.server.data.ChangeAttribute;
@@ -393,17 +395,24 @@ public class StreamEventsApiListener
 
   @Override
   public void onCommentAdded(CommentAddedListener.Event ev) {
-    try {
-      ChangeNotes notes = getNotes(ev.getChange());
-      Change change = notes.getChange();
-      PatchSet ps = getPatchSet(notes, ev.getRevision());
-      CommentAddedEvent event = new CommentAddedEvent(change);
+    Change change;
 
-      event.change = changeAttributeSupplier(change, notes);
-      event.author = accountAttributeSupplier(ev.getWho());
-      event.patchSet = patchSetAttributeSupplier(change, ps);
-      event.comment = ev.getComment();
-      event.approvals = approvalsAttributeSupplier(change, ev.getApprovals(), ev.getOldApprovals());
+    try {
+      CommentAddedEvent event;
+
+      try (ReadonlyRequestWindow window = PerThreadCache.openReadonlyRequestWindow()) {
+        ChangeNotes notes = getNotes(ev.getChange());
+        change = notes.getChange();
+        PatchSet ps = getPatchSet(notes, ev.getRevision());
+        event = new CommentAddedEvent(change);
+
+        event.change = changeAttributeSupplier(change, notes);
+        event.author = accountAttributeSupplier(ev.getWho());
+        event.patchSet = patchSetAttributeSupplier(change, ps);
+        event.comment = ev.getComment();
+        event.approvals =
+            approvalsAttributeSupplier(change, ev.getApprovals(), ev.getOldApprovals());
+      }
 
       dispatcher.run(d -> d.postEvent(change, event));
     } catch (OrmException e) {
diff --git a/java/com/google/gerrit/server/extensions/events/CommentAdded.java b/java/com/google/gerrit/server/extensions/events/CommentAdded.java
index e224540dab..c941393a96 100644
--- a/java/com/google/gerrit/server/extensions/events/CommentAdded.java
+++ b/java/com/google/gerrit/server/extensions/events/CommentAdded.java
@@ -25,6 +25,8 @@ import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.server.GpgException;
 import com.google.gerrit.server.account.AccountState;
+import com.google.gerrit.server.cache.PerThreadCache;
+import com.google.gerrit.server.cache.PerThreadCache.ReadonlyRequestWindow;
 import com.google.gerrit.server.patch.PatchListNotAvailableException;
 import com.google.gerrit.server.patch.PatchListObjectTooLargeException;
 import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -61,15 +63,18 @@ public class CommentAdded {
       return;
     }
     try {
-      Event event =
-          new Event(
-              util.changeInfo(change),
-              util.revisionInfo(change.getProject(), ps),
-              util.accountInfo(author),
-              comment,
-              util.approvals(author, approvals, when),
-              util.approvals(author, oldApprovals, when),
-              when);
+      Event event;
+      try (ReadonlyRequestWindow window = PerThreadCache.openReadonlyRequestWindow()) {
+        event =
+            new Event(
+                util.changeInfo(change),
+                util.revisionInfo(change.getProject(), ps),
+                util.accountInfo(author),
+                comment,
+                util.approvals(author, approvals, when),
+                util.approvals(author, oldApprovals, when),
+                when);
+      }
       listeners.runEach(l -> l.onCommentAdded(event));
     } catch (PatchListObjectTooLargeException e) {
       logger.atWarning().log("Couldn't fire event: %s", e.getMessage());
diff --git a/java/com/google/gerrit/server/git/RepoRefCache.java b/java/com/google/gerrit/server/git/RepoRefCache.java
index 2f60f580e0..c813b8375b 100644
--- a/java/com/google/gerrit/server/git/RepoRefCache.java
+++ b/java/com/google/gerrit/server/git/RepoRefCache.java
@@ -14,26 +14,46 @@
 
 package com.google.gerrit.server.git;
 
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.server.cache.PerThreadCache;
 import java.io.IOException;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.Optional;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.stream.Collectors;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.RefDatabase;
 import org.eclipse.jgit.lib.Repository;
 
-/** {@link RefCache} backed directly by a repository. */
+/**
+ * {@link RefCache} backed directly by a repository. TODO: DO NOT MERGE
+ * PerThreadCache.CacheStalenessCheck into stable-3.2 onwards.
+ */
 public class RepoRefCache implements RefCache {
+
+  /**
+   * System property for enabling the check for stale cache entries. TODO: DO NOT MERGE into
+   * stable-3.2 onwards.
+   */
+  public static final String REPO_REF_CACHE_CHECK_STALE_ENTRIES_PROPERTY =
+      "RepoRefCache_checkStaleEntries";
+
+  private static FluentLogger log = FluentLogger.forEnclosingClass();
+
   private final RefDatabase refdb;
   private final Map<String, Optional<ObjectId>> ids;
   private final Repository repo;
+  private final boolean checkStaleEntries;
+  private final AtomicBoolean closed;
 
   public static Optional<RefCache> getOptional(Repository repo) {
     PerThreadCache cache = PerThreadCache.get();
-    if (cache != null && cache.hasReadonlyRequest()) {
+    if (cache != null && cache.allowRefCache()) {
       return cache
           .getWithLoader(
               PerThreadCache.Key.create(RepoRefCache.class, repo),
@@ -46,10 +66,14 @@ public class RepoRefCache implements RefCache {
   }
 
   public RepoRefCache(Repository repo) {
+    checkStaleEntries =
+        Boolean.valueOf(System.getProperty(REPO_REF_CACHE_CHECK_STALE_ENTRIES_PROPERTY, "false"));
+
     repo.incrementOpen();
     this.repo = repo;
     this.refdb = repo.getRefDatabase();
     this.ids = new HashMap<>();
+    this.closed = new AtomicBoolean();
   }
 
   @Override
@@ -71,6 +95,66 @@ public class RepoRefCache implements RefCache {
 
   @Override
   public void close() {
+    if (closed.getAndSet(true)) {
+      log.atWarning().log("RepoRefCache of {} closed more than once", repo.getDirectory());
+      return;
+    }
+
+    if (checkStaleEntries) {
+      checkStaleness();
+    }
+
     repo.close();
   }
+
+  /** TODO: DO NOT MERGE into stable-3.2 onwards. */
+  @VisibleForTesting
+  void checkStaleness() {
+    List<String> staleRefs = staleRefs();
+    if (staleRefs.size() > 0) {
+      throw new IllegalStateException(
+          "Repository "
+              + repo
+              + " had modifications on refs "
+              + staleRefs
+              + " during a readonly window");
+    }
+  }
+
+  private List<String> staleRefs() {
+    return ids.entrySet().stream()
+        .filter(this::isStale)
+        .map(Map.Entry::getKey)
+        .collect(Collectors.toList());
+  }
+
+  private boolean isStale(Map.Entry<String, Optional<ObjectId>> refEntry) {
+    String refName = refEntry.getKey();
+    Optional<ObjectId> id = ids.get(refName);
+    if (id == null) {
+      return false;
+    }
+
+    try {
+      ObjectId diskId = refdb.exactRef(refName).getObjectId();
+      boolean isStale = !Optional.ofNullable(diskId).equals(id);
+      if (isStale) {
+        log.atSevere().log(
+            "Repository "
+                + repo
+                + " has a stale ref "
+                + refName
+                + " (cache="
+                + id
+                + ", disk="
+                + diskId
+                + ")");
+      }
+      return isStale;
+    } catch (IOException e) {
+      log.atSevere().withCause(e).log(
+          "Unable to check if ref={} from repository={} is stale", refName, repo);
+      return true;
+    }
+  }
 }
diff --git a/javatests/com/google/gerrit/server/cache/BUILD b/javatests/com/google/gerrit/server/cache/BUILD
index 84c1f18024..7ef48fc20f 100644
--- a/javatests/com/google/gerrit/server/cache/BUILD
+++ b/javatests/com/google/gerrit/server/cache/BUILD
@@ -5,10 +5,12 @@ junit_tests(
     srcs = glob(["*.java"]),
     deps = [
         "//java/com/google/gerrit/server",
+        "//java/com/google/gerrit/testing:gerrit-test-util",
         "//javatests/com/google/gerrit/util/http/testutil",
         "//lib:junit",
         "//lib/truth",
         "//lib/truth:truth-java8-extension",
+        "@jgit-lib//jar",
         "@servlet-api-3_1//jar",
     ],
 )
diff --git a/javatests/com/google/gerrit/server/cache/PerThreadCacheTest.java b/javatests/com/google/gerrit/server/cache/PerThreadCacheTest.java
index 2905e570ac..f1d074f087 100644
--- a/javatests/com/google/gerrit/server/cache/PerThreadCacheTest.java
+++ b/javatests/com/google/gerrit/server/cache/PerThreadCacheTest.java
@@ -16,12 +16,19 @@ package com.google.gerrit.server.cache;
 
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth8.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
 
+import com.google.gerrit.server.cache.PerThreadCache.Key;
+import com.google.gerrit.server.cache.PerThreadCache.ReadonlyRequestWindow;
+import com.google.gerrit.server.git.RefCache;
 import com.google.gerrit.util.http.testutil.FakeHttpServletRequest;
+import java.io.IOException;
+import java.util.Optional;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.function.Supplier;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletRequestWrapper;
+import org.eclipse.jgit.lib.ObjectId;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
@@ -102,7 +109,7 @@ public class PerThreadCacheTest {
   public void isAssociatedWithHttpReadonlyRequest() {
     HttpServletRequest getRequest = new FakeHttpServletRequest();
     try (PerThreadCache cache = PerThreadCache.create(getRequest)) {
-      assertThat(cache.hasReadonlyRequest()).isTrue();
+      assertThat(cache.allowRefCache()).isTrue();
     }
   }
 
@@ -116,21 +123,167 @@ public class PerThreadCacheTest {
           }
         };
     try (PerThreadCache cache = PerThreadCache.create(putRequest)) {
-      assertThat(cache.hasReadonlyRequest()).isFalse();
+      assertThat(cache.allowRefCache()).isFalse();
     }
   }
 
   @Test
   public void isNotAssociatedWithHttpRequest() {
     try (PerThreadCache cache = PerThreadCache.create(null)) {
-      assertThat(cache.hasReadonlyRequest()).isFalse();
+      assertThat(cache.allowRefCache()).isFalse();
     }
   }
 
   @Test
   public void isAssociatedWithReadonlyRequest() {
     try (PerThreadCache cache = PerThreadCache.createReadOnly()) {
-      assertThat(cache.hasReadonlyRequest()).isTrue();
+      assertThat(cache.allowRefCache()).isTrue();
+    }
+  }
+
+  @Test
+  public void openNestedTemporaryReadonlyWindows() throws Exception {
+
+    try (PerThreadCache cache = PerThreadCache.create(null)) {
+      assertThat(cache.allowRefCache()).isFalse();
+
+      try (ReadonlyRequestWindow outerWindow = PerThreadCache.openReadonlyRequestWindow()) {
+        assertThat(cache.allowRefCache()).isTrue();
+
+        try (ReadonlyRequestWindow innerWindow = PerThreadCache.openReadonlyRequestWindow()) {
+          assertThat(cache.allowRefCache()).isTrue();
+        }
+
+        assertThat(cache.allowRefCache()).isTrue();
+      }
+
+      assertThat(cache.allowRefCache()).isFalse();
+    }
+  }
+
+  static class TestCacheValue implements RefCache {
+    public static final String STALENESS_FAILED_MESSAGE = "Staleness check failed";
+
+    private final String value;
+
+    public TestCacheValue(String value) {
+      this.value = value;
+    }
+
+    @Override
+    public String toString() {
+      return value;
+    }
+
+    @Override
+    public boolean equals(Object obj) {
+      return obj != null
+          && (obj instanceof TestCacheValue)
+          && ((TestCacheValue) obj).value.equals(value);
+    }
+
+    @Override
+    public int hashCode() {
+      return value.hashCode();
+    }
+
+    public void checkStaleness() throws IllegalStateException {
+      throw new IllegalStateException(STALENESS_FAILED_MESSAGE);
+    }
+
+    @Override
+    public Optional<ObjectId> get(String refName) throws IOException {
+      return Optional.empty();
+    }
+
+    @Override
+    public void close() {}
+  }
+
+  @Test
+  public void clearOutStaleEntriesAfterReadonlyWindow() throws Exception {
+    Key<TestCacheValue> key = PerThreadCache.Key.create(TestCacheValue.class, "key");
+    TestCacheValue cachedValue = new TestCacheValue("cached value");
+    TestCacheValue updatedValue = new TestCacheValue("updated value");
+
+    try (PerThreadCache cache = PerThreadCache.create(null)) {
+      try (ReadonlyRequestWindow outerWindow = PerThreadCache.openReadonlyRequestWindow()) {
+        assertThat(PerThreadCache.get(key, () -> cachedValue, v -> {})).hasValue(cachedValue);
+        assertThat(PerThreadCache.get(key, () -> updatedValue, v -> {})).hasValue(cachedValue);
+      }
+
+      assertThat(PerThreadCache.get(key, () -> updatedValue, v -> {})).hasValue(updatedValue);
+    }
+  }
+
+  @Test
+  public void checkForStaleEntriesAfterReadonlyWindow() {
+    Key<TestCacheValue> key = PerThreadCache.Key.create(TestCacheValue.class, "key");
+
+    try (PerThreadCache cache = PerThreadCache.create(null)) {
+      IllegalStateException thrown =
+          assertThrows(
+              IllegalStateException.class,
+              () -> {
+                try (ReadonlyRequestWindow outerWindow =
+                    PerThreadCache.openReadonlyRequestWindow()) {
+                  PerThreadCache.get(
+                      key, () -> new TestCacheValue(""), TestCacheValue::checkStaleness);
+                }
+              });
+      assertThat(thrown).hasMessageThat().isEqualTo(TestCacheValue.STALENESS_FAILED_MESSAGE);
+    }
+  }
+
+  @Test
+  public void allowStaleEntriesAfterReadonlyWindow() {
+    Key<TestCacheValue> key = PerThreadCache.Key.create(TestCacheValue.class, "key");
+    TestCacheValue value = new TestCacheValue("");
+    Optional<TestCacheValue> cachedValue;
+
+    try (PerThreadCache cache = PerThreadCache.create(null)) {
+      try (ReadonlyRequestWindow window = PerThreadCache.openReadonlyRequestWindow()) {
+
+        cachedValue = PerThreadCache.get(key, () -> value, null);
+      }
+    }
+
+    assertThat(cachedValue).hasValue(value);
+  }
+
+  @SuppressWarnings("deprecation")
+  @Test
+  public void disableCachingForSpecificTypes() {
+    System.setProperty(
+        PerThreadCache.PER_THREAD_CACHE_DISABLED_TYPES_PROPERTY,
+        String.class.getCanonicalName() + "," + Integer.class.getCanonicalName());
+    try {
+      try (PerThreadCache cache = PerThreadCache.createReadOnly()) {
+        assertThat(
+                PerThreadCache.get(
+                    PerThreadCache.Key.create(String.class, "key"), () -> "oldValue", null))
+            .isEmpty();
+        assertThat(
+                PerThreadCache.getOrCompute(
+                    PerThreadCache.Key.create(String.class, "key"), () -> "newValue"))
+            .isEqualTo("newValue");
+
+        assertThat(
+                PerThreadCache.get(PerThreadCache.Key.create(Integer.class, "key"), () -> 1, null))
+            .isEmpty();
+        assertThat(
+                PerThreadCache.getOrCompute(
+                    PerThreadCache.Key.create(Integer.class, "key"), () -> 2))
+            .isEqualTo(2);
+
+        assertThat(PerThreadCache.get(PerThreadCache.Key.create(Long.class, "key"), () -> 1L, null))
+            .hasValue(1L);
+        assertThat(
+                PerThreadCache.getOrCompute(PerThreadCache.Key.create(Long.class, "key"), () -> 2L))
+            .isEqualTo(1L);
+      }
+    } finally {
+      System.setProperty(PerThreadCache.PER_THREAD_CACHE_DISABLED_TYPES_PROPERTY, "");
     }
   }
 
@@ -142,7 +295,7 @@ public class PerThreadCacheTest {
 
       // Assert that the value was not persisted
       PerThreadCache.Key<String> key = PerThreadCache.Key.create(String.class, 1000);
-      cache.get(key, () -> "new value");
+      assertThat(cache.getWithLoader(key, () -> "new value", null)).isEmpty();
       String value = cache.get(key, () -> "directly served");
       assertThat(value).isEqualTo("directly served");
     }
diff --git a/javatests/com/google/gerrit/server/git/RepoRefCacheTest.java b/javatests/com/google/gerrit/server/git/RepoRefCacheTest.java
index 1f422c1fe1..4063495a51 100644
--- a/javatests/com/google/gerrit/server/git/RepoRefCacheTest.java
+++ b/javatests/com/google/gerrit/server/git/RepoRefCacheTest.java
@@ -15,11 +15,14 @@
 package com.google.gerrit.server.git;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
 
+import com.google.gerrit.server.cache.PerThreadCache;
 import java.io.IOException;
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import org.eclipse.jgit.attributes.AttributesNodeProvider;
 import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
 import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
@@ -28,10 +31,12 @@ import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.BatchRefUpdate;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectDatabase;
+import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.RefDatabase;
 import org.eclipse.jgit.lib.RefRename;
 import org.eclipse.jgit.lib.RefUpdate;
+import org.eclipse.jgit.lib.RefUpdate.Result;
 import org.eclipse.jgit.lib.ReflogReader;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.lib.StoredConfig;
@@ -58,6 +63,60 @@ public class RepoRefCacheTest {
     assertThat(cache.get(Constants.R_HEADS + TEST_BRANCH)).isNotNull();
   }
 
+  @Test
+  @SuppressWarnings("resource")
+  public void shouldNotKeepReferenceToReposWhenCacheIsFull() throws Exception {
+    TestRepositoryWithRefCounting repoPointedFromCache;
+
+    try (PerThreadCache threadCache = PerThreadCache.createReadOnly()) {
+      fillUpAllThreadCache(threadCache);
+
+      try (TestRepositoryWithRefCounting repo =
+          TestRepositoryWithRefCounting.createWithBranch(TEST_BRANCH)) {
+        repoPointedFromCache = repo;
+        assertThat(repo.refCounter()).isEqualTo(1);
+        Optional<RefCache> refCache = RepoRefCache.getOptional(repo);
+        assertThat(refCache).isEqualTo(Optional.empty());
+        assertThat(repo.refCounter()).isEqualTo(1);
+      }
+
+      assertThat(repoPointedFromCache.refCounter()).isEqualTo(0);
+    }
+  }
+
+  @Test
+  public void shouldCheckForStaleness() throws Exception {
+    String refName = "refs/heads/foo";
+
+    try (TestRepositoryWithRefCounting repo =
+        TestRepositoryWithRefCounting.createWithBranch(TEST_BRANCH)) {
+      RepoRefCache refCache = new RepoRefCache(repo);
+      TestRepository<Repository> testRepo = new TestRepository<>(repo);
+
+      Optional<ObjectId> cachedObjId = refCache.get(refName);
+
+      assertThat(cachedObjId).isEqualTo(Optional.empty());
+
+      RefUpdate refUpdate = repo.getRefDatabase().newUpdate(refName, true);
+      refUpdate.setNewObjectId(testRepo.commit().create().getId());
+
+      assertThat(refUpdate.forceUpdate()).isEqualTo(Result.NEW);
+
+      IllegalStateException thrown =
+          assertThrows(IllegalStateException.class, () -> refCache.checkStaleness());
+      assertThat(thrown).hasMessageThat().contains(refName);
+    }
+  }
+
+  private void fillUpAllThreadCache(PerThreadCache cache) {
+
+    // Fill the cache
+    for (int i = 0; i < 50; i++) {
+      PerThreadCache.Key<String> key = PerThreadCache.Key.create(String.class, i);
+      cache.getWithLoader(key, () -> "cached value", null);
+    }
+  }
+
   private static class TestRepositoryWithRefCounting extends Repository {
     private int refCounter;