Merge branch 'stable-3.2' into stable-3.3

* stable-3.2:
  Set PerThreadCache as readonly after creating a new patch-set
  Set PerThreadCache as readonly when formatting change e-mails
  Set PerThreadCache as readonly when formatting change JSON
  Set PerThreadCache as readonly after deleting a change
  Set PerThreadCache as readonly after abandoning a change
  Set PerThreadCache as readonly after merging a change
  Set PerThreadCache as readonly after posting review comments
  Introduce unloaders on PerThreadCache entries
  RepoRefCache: Hold a reference to the refDatabase with ref counting
  Remove use of RefCache in ChangeNotes
  Revert "Cache change /meta ref SHA1 for each REST API request"
  Cache change /meta ref SHA1 for each change indexing task

Release-Notes: skip
Change-Id: Ic3981c1fad0a1f8c232a72d858b8521d2407c0ff

8 files changed, 311 insertions, 87 deletions

diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index fb1f2353ff..d427caa0c6 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -326,7 +326,7 @@ public class RestApiServlet extends HttpServlet {
     try (TraceContext traceContext = enableTracing(req, res)) {
       List<IdString> path = splitPath(req);
 
-      try (PerThreadCache ignored = PerThreadCache.create(req)) {
+      try (PerThreadCache ignored = PerThreadCache.create()) {
         RequestInfo requestInfo = createRequestInfo(traceContext, requestUri(req), path);
         globals.requestListeners.runEach(l -> l.onRequest(requestInfo));
 
diff --git a/java/com/google/gerrit/server/cache/PerThreadCache.java b/java/com/google/gerrit/server/cache/PerThreadCache.java
index 609fce7b25..b4f79d10ab 100644
--- a/java/com/google/gerrit/server/cache/PerThreadCache.java
+++ b/java/com/google/gerrit/server/cache/PerThreadCache.java
@@ -21,9 +21,7 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Maps;
 import com.google.gerrit.common.Nullable;
 import java.util.Map;
-import java.util.Optional;
 import java.util.function.Supplier;
-import javax.servlet.http.HttpServletRequest;
 
 /**
  * Caches object instances for a request as {@link ThreadLocal} in the serving thread.
@@ -60,12 +58,6 @@ public class PerThreadCache implements AutoCloseable {
   private static final int PER_THREAD_CACHE_SIZE = 25;
 
   /**
-   * Optional HTTP request associated with the per-thread cache, should the thread be associated
-   * with the incoming HTTP thread pool.
-   */
-  private final Optional<HttpServletRequest> httpRequest;
-
-  /**
    * Unique key for key-value mappings stored in PerThreadCache. The key is based on the value's
    * class and a list of identifiers that in combination uniquely set the object apart form others
    * of the same class.
@@ -110,9 +102,9 @@ public class PerThreadCache implements AutoCloseable {
     }
   }
 
-  public static PerThreadCache create(@Nullable HttpServletRequest httpRequest) {
+  public static PerThreadCache create() {
     checkState(CACHE.get() == null, "called create() twice on the same request");
-    PerThreadCache cache = new PerThreadCache(httpRequest);
+    PerThreadCache cache = new PerThreadCache();
     CACHE.set(cache);
     return cache;
   }
@@ -129,9 +121,7 @@ public class PerThreadCache implements AutoCloseable {
 
   private final Map<Key<?>, Object> cache = Maps.newHashMapWithExpectedSize(PER_THREAD_CACHE_SIZE);
 
-  private PerThreadCache(@Nullable HttpServletRequest req) {
-    httpRequest = Optional.ofNullable(req);
-  }
+  private PerThreadCache() {}
 
   /**
    * Returns an instance of {@code T} that was either loaded from the cache or obtained from the
@@ -149,19 +139,6 @@ public class PerThreadCache implements AutoCloseable {
     return value;
   }
 
-  /** Returns the optional HTTP request associated with the local thread cache. */
-  public Optional<HttpServletRequest> getHttpRequest() {
-    return httpRequest;
-  }
-
-  /** Returns true if there is an HTTP request associated and is a GET or HEAD */
-  public boolean hasReadonlyRequest() {
-    return httpRequest
-        .map(HttpServletRequest::getMethod)
-        .filter(m -> m.equalsIgnoreCase("GET") || m.equalsIgnoreCase("HEAD"))
-        .isPresent();
-  }
-
   @Override
   public void close() {
     CACHE.remove();
diff --git a/java/com/google/gerrit/server/git/RefCache.java b/java/com/google/gerrit/server/git/RefCache.java
index 5a5cae9332..2dee427bf7 100644
--- a/java/com/google/gerrit/server/git/RefCache.java
+++ b/java/com/google/gerrit/server/git/RefCache.java
@@ -37,4 +37,7 @@ public interface RefCache {
    *     present with a value of {@link ObjectId#zeroId()}.
    */
   Optional<ObjectId> get(String refName) throws IOException;
+
+  /** Closes this cache, releasing the references to any underlying resources. */
+  void close();
 }
diff --git a/java/com/google/gerrit/server/git/RepoRefCache.java b/java/com/google/gerrit/server/git/RepoRefCache.java
index 9086da7fb5..7a61e67982 100644
--- a/java/com/google/gerrit/server/git/RepoRefCache.java
+++ b/java/com/google/gerrit/server/git/RepoRefCache.java
@@ -14,7 +14,6 @@
 
 package com.google.gerrit.server.git;
 
-import com.google.gerrit.server.cache.PerThreadCache;
 import java.io.IOException;
 import java.util.Collections;
 import java.util.HashMap;
@@ -29,19 +28,11 @@ import org.eclipse.jgit.lib.Repository;
 public class RepoRefCache implements RefCache {
   private final RefDatabase refdb;
   private final Map<String, Optional<ObjectId>> ids;
-
-  public static Optional<RefCache> getOptional(Repository repo) {
-    PerThreadCache cache = PerThreadCache.get();
-    if (cache != null && cache.hasReadonlyRequest()) {
-      return Optional.of(
-          cache.get(
-              PerThreadCache.Key.create(RepoRefCache.class, repo), () -> new RepoRefCache(repo)));
-    }
-
-    return Optional.empty();
-  }
+  private final Repository repo;
 
   public RepoRefCache(Repository repo) {
+    repo.incrementOpen();
+    this.repo = repo;
     this.refdb = repo.getRefDatabase();
     this.ids = new HashMap<>();
   }
@@ -62,4 +53,9 @@ public class RepoRefCache implements RefCache {
   public Map<String, Optional<ObjectId>> getCachedRefs() {
     return Collections.unmodifiableMap(ids);
   }
+
+  @Override
+  public void close() {
+    repo.close();
+  }
 }
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotes.java b/java/com/google/gerrit/server/notedb/ChangeNotes.java
index 60954eb9d2..28f25ec5e4 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -56,7 +56,6 @@ import com.google.gerrit.server.ReviewerByEmailSet;
 import com.google.gerrit.server.ReviewerSet;
 import com.google.gerrit.server.ReviewerStatusUpdate;
 import com.google.gerrit.server.git.RefCache;
-import com.google.gerrit.server.git.RepoRefCache;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.query.change.ChangeData;
@@ -639,10 +638,6 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
 
   @Override
   protected ObjectId readRef(Repository repo) throws IOException {
-    Optional<RefCache> refsCache =
-        Optional.ofNullable(refs).map(Optional::of).orElse(RepoRefCache.getOptional(repo));
-    return refsCache.isPresent()
-        ? refsCache.get().get(getRefName()).orElse(null)
-        : super.readRef(repo);
+    return refs != null ? refs.get(getRefName()).orElse(null) : super.readRef(repo);
   }
 }
diff --git a/java/com/google/gerrit/server/update/ChainedReceiveCommands.java b/java/com/google/gerrit/server/update/ChainedReceiveCommands.java
index c223aec637..9a395759bf 100644
--- a/java/com/google/gerrit/server/update/ChainedReceiveCommands.java
+++ b/java/com/google/gerrit/server/update/ChainedReceiveCommands.java
@@ -39,13 +39,19 @@ import org.eclipse.jgit.transport.ReceiveCommand;
 public class ChainedReceiveCommands implements RefCache {
   private final Map<String, ReceiveCommand> commands = new LinkedHashMap<>();
   private final RepoRefCache refCache;
+  private final boolean closeRefCache;
 
   public ChainedReceiveCommands(Repository repo) {
-    this(new RepoRefCache(repo));
+    this(new RepoRefCache(repo), true);
   }
 
   public ChainedReceiveCommands(RepoRefCache refCache) {
+    this(refCache, false);
+  }
+
+  private ChainedReceiveCommands(RepoRefCache refCache, boolean closeRefCache) {
     this.refCache = requireNonNull(refCache);
+    this.closeRefCache = closeRefCache;
   }
 
   public RepoRefCache getRepoRefCache() {
@@ -122,4 +128,11 @@ public class ChainedReceiveCommands implements RefCache {
   public Map<String, ReceiveCommand> getCommands() {
     return Collections.unmodifiableMap(commands);
   }
+
+  @Override
+  public void close() {
+    if (closeRefCache) {
+      refCache.close();
+    }
+  }
 }
diff --git a/javatests/com/google/gerrit/server/cache/PerThreadCacheTest.java b/javatests/com/google/gerrit/server/cache/PerThreadCacheTest.java
index c04deb4e8e..1bb97842b5 100644
--- a/javatests/com/google/gerrit/server/cache/PerThreadCacheTest.java
+++ b/javatests/com/google/gerrit/server/cache/PerThreadCacheTest.java
@@ -18,10 +18,7 @@ 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.util.http.testutil.FakeHttpServletRequest;
 import java.util.function.Supplier;
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletRequestWrapper;
 import org.junit.Test;
 
 public class PerThreadCacheTest {
@@ -47,7 +44,7 @@ public class PerThreadCacheTest {
 
   @Test
   public void endToEndCache() {
-    try (PerThreadCache ignored = PerThreadCache.create(null)) {
+    try (PerThreadCache ignored = PerThreadCache.create()) {
       PerThreadCache cache = PerThreadCache.get();
       PerThreadCache.Key<String> key1 = PerThreadCache.Key.create(String.class);
 
@@ -65,7 +62,7 @@ public class PerThreadCacheTest {
   @Test
   public void cleanUp() {
     PerThreadCache.Key<String> key = PerThreadCache.Key.create(String.class);
-    try (PerThreadCache ignored = PerThreadCache.create(null)) {
+    try (PerThreadCache ignored = PerThreadCache.create()) {
       PerThreadCache cache = PerThreadCache.get();
       String value1 = cache.get(key, () -> "value1");
       assertThat(value1).isEqualTo("value1");
@@ -73,7 +70,7 @@ public class PerThreadCacheTest {
 
     // Create a second cache and assert that it is not connected to the first one.
     // This ensures that the cleanup is actually working.
-    try (PerThreadCache ignored = PerThreadCache.create(null)) {
+    try (PerThreadCache ignored = PerThreadCache.create()) {
       PerThreadCache cache = PerThreadCache.get();
       String value1 = cache.get(key, () -> "value2");
       assertThat(value1).isEqualTo("value2");
@@ -82,48 +79,16 @@ public class PerThreadCacheTest {
 
   @Test
   public void doubleInstantiationFails() {
-    try (PerThreadCache ignored = PerThreadCache.create(null)) {
+    try (PerThreadCache ignored = PerThreadCache.create()) {
       IllegalStateException thrown =
-          assertThrows(IllegalStateException.class, () -> PerThreadCache.create(null));
+          assertThrows(IllegalStateException.class, () -> PerThreadCache.create());
       assertThat(thrown).hasMessageThat().contains("called create() twice on the same request");
     }
   }
 
   @Test
-  public void isAssociatedWithHttpReadonlyRequest() {
-    HttpServletRequest getRequest = new FakeHttpServletRequest();
-    try (PerThreadCache cache = PerThreadCache.create(getRequest)) {
-      assertThat(cache.getHttpRequest()).hasValue(getRequest);
-      assertThat(cache.hasReadonlyRequest()).isTrue();
-    }
-  }
-
-  @Test
-  public void isAssociatedWithHttpWriteRequest() {
-    HttpServletRequest putRequest =
-        new HttpServletRequestWrapper(new FakeHttpServletRequest()) {
-          @Override
-          public String getMethod() {
-            return "PUT";
-          }
-        };
-    try (PerThreadCache cache = PerThreadCache.create(putRequest)) {
-      assertThat(cache.getHttpRequest()).hasValue(putRequest);
-      assertThat(cache.hasReadonlyRequest()).isFalse();
-    }
-  }
-
-  @Test
-  public void isNotAssociatedWithHttpRequest() {
-    try (PerThreadCache cache = PerThreadCache.create(null)) {
-      assertThat(cache.getHttpRequest()).isEmpty();
-      assertThat(cache.hasReadonlyRequest()).isFalse();
-    }
-  }
-
-  @Test
   public void enforceMaxSize() {
-    try (PerThreadCache cache = PerThreadCache.create(null)) {
+    try (PerThreadCache cache = PerThreadCache.create()) {
       // Fill the cache
       for (int i = 0; i < 50; i++) {
         PerThreadCache.Key<String> key = PerThreadCache.Key.create(String.class, i);
diff --git a/javatests/com/google/gerrit/server/git/RepoRefCacheTest.java b/javatests/com/google/gerrit/server/git/RepoRefCacheTest.java
new file mode 100644
index 0000000000..81d68147ea
--- /dev/null
+++ b/javatests/com/google/gerrit/server/git/RepoRefCacheTest.java
@@ -0,0 +1,275 @@
+// Copyright (C) 2022 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.git;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import org.eclipse.jgit.attributes.AttributesNodeProvider;
+import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository.Builder;
+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.Ref;
+import org.eclipse.jgit.lib.RefDatabase;
+import org.eclipse.jgit.lib.RefRename;
+import org.eclipse.jgit.lib.RefUpdate;
+import org.eclipse.jgit.lib.ReflogReader;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.lib.StoredConfig;
+import org.eclipse.jgit.util.FS;
+import org.junit.Test;
+
+public class RepoRefCacheTest {
+  private static final String TEST_BRANCH = "main";
+
+  @Test
+  @SuppressWarnings("resource")
+  public void repositoryUseShouldBeTrackedByRepoRefCache() throws Exception {
+    RefCache cache;
+    TestRepositoryWithRefCounting repoWithRefCounting;
+
+    try (TestRepositoryWithRefCounting repo =
+        TestRepositoryWithRefCounting.createWithBranch(TEST_BRANCH)) {
+      assertThat(repo.refCounter()).isEqualTo(1);
+      repoWithRefCounting = repo;
+      cache = new RepoRefCache(repo);
+    }
+
+    assertThat(repoWithRefCounting.refCounter()).isEqualTo(1);
+    assertThat(cache.get(Constants.R_HEADS + TEST_BRANCH)).isNotNull();
+  }
+
+  private static class TestRepositoryWithRefCounting extends Repository {
+    private int refCounter;
+
+    static TestRepositoryWithRefCounting createWithBranch(String branchName) throws Exception {
+      Builder builder =
+          new InMemoryRepository.Builder()
+              .setRepositoryDescription(new DfsRepositoryDescription(""))
+              .setFS(FS.detect().setUserHome(null));
+      TestRepositoryWithRefCounting testRepo = new TestRepositoryWithRefCounting(builder);
+      new TestRepository<>(testRepo).branch(branchName).commit().message("").create();
+      return testRepo;
+    }
+
+    private final Repository repo;
+
+    private TestRepositoryWithRefCounting(InMemoryRepository.Builder builder) throws IOException {
+      super(builder);
+
+      repo = builder.build();
+      refCounter = 1;
+    }
+
+    public int refCounter() {
+      return refCounter;
+    }
+
+    @Override
+    public void incrementOpen() {
+      repo.incrementOpen();
+      refCounter++;
+    }
+
+    @Override
+    public void close() {
+      repo.close();
+      refCounter--;
+    }
+
+    @Override
+    public void create(boolean bare) throws IOException {}
+
+    @Override
+    public ObjectDatabase getObjectDatabase() {
+      checkIsOpen();
+      return repo.getObjectDatabase();
+    }
+
+    @Override
+    public RefDatabase getRefDatabase() {
+      RefDatabase refDatabase = repo.getRefDatabase();
+      return new RefDatabase() {
+
+        @Override
+        public int hashCode() {
+          return refDatabase.hashCode();
+        }
+
+        @Override
+        public void create() throws IOException {
+          refDatabase.create();
+        }
+
+        @Override
+        public void close() {
+          checkIsOpen();
+          refDatabase.close();
+        }
+
+        @Override
+        public boolean isNameConflicting(String name) throws IOException {
+          checkIsOpen();
+          return refDatabase.isNameConflicting(name);
+        }
+
+        @Override
+        public boolean equals(Object obj) {
+          return refDatabase.equals(obj);
+        }
+
+        @Override
+        public Collection<String> getConflictingNames(String name) throws IOException {
+          checkIsOpen();
+          return refDatabase.getConflictingNames(name);
+        }
+
+        @Override
+        public RefUpdate newUpdate(String name, boolean detach) throws IOException {
+          checkIsOpen();
+          return refDatabase.newUpdate(name, detach);
+        }
+
+        @Override
+        public RefRename newRename(String fromName, String toName) throws IOException {
+          checkIsOpen();
+          return refDatabase.newRename(fromName, toName);
+        }
+
+        @Override
+        public BatchRefUpdate newBatchUpdate() {
+          checkIsOpen();
+          return refDatabase.newBatchUpdate();
+        }
+
+        @Override
+        public boolean performsAtomicTransactions() {
+          checkIsOpen();
+          return refDatabase.performsAtomicTransactions();
+        }
+
+        @Override
+        public Ref exactRef(String name) throws IOException {
+          checkIsOpen();
+          return refDatabase.exactRef(name);
+        }
+
+        @Override
+        public String toString() {
+          return refDatabase.toString();
+        }
+
+        @Override
+        public Map<String, Ref> exactRef(String... refs) throws IOException {
+          checkIsOpen();
+          return refDatabase.exactRef(refs);
+        }
+
+        @Override
+        public Ref firstExactRef(String... refs) throws IOException {
+          checkIsOpen();
+          return refDatabase.firstExactRef(refs);
+        }
+
+        @Override
+        public List<Ref> getRefs() throws IOException {
+          checkIsOpen();
+          return refDatabase.getRefs();
+        }
+
+        @Override
+        public Map<String, Ref> getRefs(String prefix) throws IOException {
+          checkIsOpen();
+          return refDatabase.getRefs(prefix);
+        }
+
+        @Override
+        public List<Ref> getRefsByPrefix(String prefix) throws IOException {
+          checkIsOpen();
+          return refDatabase.getRefsByPrefix(prefix);
+        }
+
+        @Override
+        public boolean hasRefs() throws IOException {
+          checkIsOpen();
+          return refDatabase.hasRefs();
+        }
+
+        @Override
+        public List<Ref> getAdditionalRefs() throws IOException {
+          checkIsOpen();
+          return refDatabase.getAdditionalRefs();
+        }
+
+        @Override
+        public Ref peel(Ref ref) throws IOException {
+          checkIsOpen();
+          return refDatabase.peel(ref);
+        }
+
+        @Override
+        public void refresh() {
+          checkIsOpen();
+          refDatabase.refresh();
+        }
+      };
+    }
+
+    @Override
+    public StoredConfig getConfig() {
+      return repo.getConfig();
+    }
+
+    @Override
+    public AttributesNodeProvider createAttributesNodeProvider() {
+      checkIsOpen();
+      return repo.createAttributesNodeProvider();
+    }
+
+    @Override
+    public void scanForRepoChanges() throws IOException {
+      checkIsOpen();
+    }
+
+    @Override
+    public void notifyIndexChanged(boolean internal) {
+      checkIsOpen();
+    }
+
+    @Override
+    public ReflogReader getReflogReader(String refName) throws IOException {
+      checkIsOpen();
+      return repo.getReflogReader(refName);
+    }
+
+    private void checkIsOpen() {
+      if (refCounter <= 0) {
+        throw new IllegalStateException("Repository is not open (refCounter=" + refCounter + ")");
+      }
+    }
+
+    @Override
+    public String getIdentifier() {
+      return "foo";
+    }
+  }
+}