diff options
author | Luca Milanesio <luca.milanesio@gmail.com> | 2022-05-20 17:29:16 +0100 |
---|---|---|
committer | Luca Milanesio <luca.milanesio@gmail.com> | 2022-05-20 19:21:22 +0100 |
commit | 6bf056a620a531643dd56b34ad81ba03c39a11f1 (patch) | |
tree | 6d5d90e6e0d08d0d2116eed660fc75f05c609fa7 | |
parent | 5086635007c72a8956ade8def9a120302f282e58 (diff) | |
parent | 71c5cae07aaedccb561d5a8be53d79b8761fc36f (diff) |
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
9 files changed, 311 insertions, 88 deletions
diff --git a/.bazelversion b/.bazelversion index 6aba2b245a..af8c8ec7c1 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 57dc9f3a8e..d981a45e6a 100644 --- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java +++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java @@ -361,7 +361,7 @@ public class RestApiServlet extends HttpServlet { 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 4270d1e961..ef00b80492 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 7f221110f9..d2b3c32bcb 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 3ecedfaa0b..a1d6b29d14 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.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; @@ -702,10 +701,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 99c72f2307..5ff8d33815 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..2bc6b92b6b --- /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"; + } + } +} |