Merge branch 'stable-3.4' into stable-3.5
* stable-3.4:
Bump bazel version to 4.2.2
Update git submodules
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
Also fix references to Builder of InMemoryRepository flagged by
errorprone on RepoRefCacheTest.
Release-Notes: skip
Change-Id: I9a83cf3a14e1df7c0d80a48d06ada138d305f42c
diff --git a/.bazelversion b/.bazelversion
index 6aba2b2..af8c8ec 100644
--- a/.bazelversion
+++ b/.bazelversion
@@ -1 +1 @@
-4.2.0
+4.2.2
diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index 57dc9f3..d981a45 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -361,7 +361,7 @@
try (TraceContext traceContext = enableTracing(req, res)) {
String requestUri = requestUri(req);
- try (PerThreadCache ignored = PerThreadCache.create(req)) {
+ try (PerThreadCache ignored = PerThreadCache.create()) {
List<IdString> path = splitPath(req);
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 4270d1e..ef00b80 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.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 @@
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 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 @@
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 @@
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 5a5cae9..2dee427 100644
--- a/java/com/google/gerrit/server/git/RefCache.java
+++ b/java/com/google/gerrit/server/git/RefCache.java
@@ -37,4 +37,7 @@
* 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 7f22111..d2b3c32 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 @@
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 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 3ecedfa..a1d6b29 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -58,7 +58,6 @@
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;
@@ -702,10 +701,6 @@
@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 99c72f2..5ff8d33 100644
--- a/java/com/google/gerrit/server/update/ChainedReceiveCommands.java
+++ b/java/com/google/gerrit/server/update/ChainedReceiveCommands.java
@@ -39,13 +39,19 @@
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 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 c04deb4..1bb9784 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.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 @@
@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 @@
@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 @@
// 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 @@
@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 0000000..2bc6b92
--- /dev/null
+++ b/javatests/com/google/gerrit/server/git/RepoRefCacheTest.java
@@ -0,0 +1,274 @@
+// 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.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 {
+ InMemoryRepository.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";
+ }
+ }
+}
