Merge branch 'stable-3.4' into stable-3.5

* stable-3.4:
  Introduce cache.threads option to enable a custom cache executor
  GitwebServlet: Fix project root computation

Release-Notes: skip
Change-Id: If80bdcf0833713dfbcd5f37ba2e0209bb021cc2f

7 files changed, 289 insertions, 9 deletions

diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 2685d3fae1..706a4608bc 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -743,6 +743,19 @@ By default, true.
 [[cache]]
 === Section cache
 
+[[cache.threads]]cache.threads::
++
+Number of threads to use when running asynchronous cache tasks.
+The threads executor is delegated to when sending removal notifications to listeners,
+when asynchronous computations like refresh, refreshAfterWrite are performed, or when
+performing periodic maintenance.
++
+**NOTE**: Setting it to 0 disables the dedicated thread pool and indexing will be done in the
+same thread as the operation. This may result in evictions taking longer because the
+listeners are executed in the caller's thread.
++
+By default, the JVM common ForkJoinPool is used.
+
 [[cache.directory]]cache.directory::
 +
 Path to a local directory where Gerrit can write cached entities for
diff --git a/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java b/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java
index 0875317542..4c18afda4f 100644
--- a/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java
+++ b/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java
@@ -32,6 +32,7 @@ package com.google.gerrit.httpd.gitweb;
 import static java.nio.charset.StandardCharsets.ISO_8859_1;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.CharMatcher;
 import com.google.common.base.Splitter;
 import com.google.common.flogger.FluentLogger;
@@ -640,20 +641,39 @@ class GitwebServlet extends HttpServlet {
     return env.getEnvArray();
   }
 
-  private String getProjectRoot(Project.NameKey nameKey)
-      throws RepositoryNotFoundException, IOException {
+  /**
+   * Return the project root under which the specified project is stored.
+   *
+   * @param nameKey the name of the project
+   * @return base directory
+   */
+  @VisibleForTesting
+  String getProjectRoot(Project.NameKey nameKey) throws RepositoryNotFoundException, IOException {
     try (Repository repo = repoManager.openRepository(nameKey)) {
-      return getProjectRoot(repo);
+      return getRepositoryRoot(repo, nameKey).toString();
     }
   }
 
-  private String getProjectRoot(Repository repo) {
+  /**
+   * Return the repository root under which the specified repository is stored.
+   *
+   * @param repo the name of the repository
+   * @param nameKey project name
+   * @return base path
+   * @throws ProvisionException if the repo is not DelegateRepository or FileRepository.
+   */
+  private static Path getRepositoryRoot(Repository repo, Project.NameKey nameKey) {
     if (repo instanceof DelegateRepository) {
-      return getProjectRoot(((DelegateRepository) repo).delegate());
+      return getRepositoryRoot(((DelegateRepository) repo).delegate(), nameKey);
     }
 
     if (repo instanceof FileRepository) {
-      return repo.getDirectory().getAbsolutePath();
+      String name = nameKey.get();
+      Path current = repo.getDirectory().toPath();
+      for (int i = 0; i <= CharMatcher.is('/').countIn(name); i++) {
+        current = current.getParent();
+      }
+      return current;
     }
 
     throw new ProvisionException("Gitweb can only be used with FileRepository");
diff --git a/java/com/google/gerrit/server/cache/ForwardingRemovalListener.java b/java/com/google/gerrit/server/cache/ForwardingRemovalListener.java
index 28d57e62d2..852d8a3faf 100644
--- a/java/com/google/gerrit/server/cache/ForwardingRemovalListener.java
+++ b/java/com/google/gerrit/server/cache/ForwardingRemovalListener.java
@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.cache;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Strings;
 import com.google.common.cache.RemovalListener;
 import com.google.common.cache.RemovalNotification;
@@ -37,7 +38,8 @@ public class ForwardingRemovalListener<K, V> implements RemovalListener<K, V> {
   private String pluginName = PluginName.GERRIT;
 
   @Inject
-  ForwardingRemovalListener(
+  @VisibleForTesting
+  protected ForwardingRemovalListener(
       PluginSetContext<CacheRemovalListener> listeners, @Assisted String cacheName) {
     this.listeners = listeners;
     this.cacheName = cacheName;
diff --git a/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java b/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java
index 42f4879b40..bf295e0fca 100644
--- a/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java
+++ b/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java
@@ -27,6 +27,7 @@ import com.google.common.cache.CacheBuilder;
 import com.google.common.cache.CacheLoader;
 import com.google.common.cache.LoadingCache;
 import com.google.common.cache.RemovalNotification;
+import com.google.common.util.concurrent.MoreExecutors;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.server.cache.CacheBackend;
 import com.google.gerrit.server.cache.CacheDef;
@@ -34,20 +35,37 @@ import com.google.gerrit.server.cache.ForwardingRemovalListener;
 import com.google.gerrit.server.cache.MemoryCacheFactory;
 import com.google.gerrit.server.config.ConfigUtil;
 import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.git.WorkQueue;
 import com.google.inject.Inject;
 import java.time.Duration;
+import java.util.concurrent.Executor;
 import org.eclipse.jgit.lib.Config;
 
 class DefaultMemoryCacheFactory implements MemoryCacheFactory {
+  static final String CACHE_EXECUTOR_PREFIX = "InMemoryCacheExecutor";
+  private static final int DEFAULT_CACHE_EXECUTOR_THREADS = -1;
+
   private final Config cfg;
   private final ForwardingRemovalListener.Factory forwardingRemovalListenerFactory;
+  private int executorThreads;
+  private final Executor executor;
 
   @Inject
   DefaultMemoryCacheFactory(
       @GerritServerConfig Config config,
-      ForwardingRemovalListener.Factory forwardingRemovalListenerFactory) {
+      ForwardingRemovalListener.Factory forwardingRemovalListenerFactory,
+      WorkQueue workQueue) {
     this.cfg = config;
     this.forwardingRemovalListenerFactory = forwardingRemovalListenerFactory;
+    this.executorThreads = config.getInt("cache", "threads", DEFAULT_CACHE_EXECUTOR_THREADS);
+
+    if (executorThreads == 0) {
+      executor = MoreExecutors.newDirectExecutorService();
+    } else if (executorThreads > DEFAULT_CACHE_EXECUTOR_THREADS) {
+      executor = workQueue.createQueue(executorThreads, CACHE_EXECUTOR_PREFIX);
+    } else {
+      executor = null;
+    }
   }
 
   @Override
@@ -129,6 +147,10 @@ class DefaultMemoryCacheFactory implements MemoryCacheFactory {
     builder.recordStats();
     builder.maximumWeight(cacheMaximumWeight(def));
     builder = builder.removalListener(newRemovalListener(def.name()));
+
+    if (executor != null) {
+      builder.executor(executor);
+    }
     builder.weigher(newWeigher(def.weigher()));
 
     Duration expireAfterWrite = def.expireAfterWrite();
diff --git a/javatests/com/google/gerrit/httpd/gitweb/GitwebServletTest.java b/javatests/com/google/gerrit/httpd/gitweb/GitwebServletTest.java
new file mode 100644
index 0000000000..d1598b9b90
--- /dev/null
+++ b/javatests/com/google/gerrit/httpd/gitweb/GitwebServletTest.java
@@ -0,0 +1,107 @@
+// 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.httpd.gitweb;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.mockito.Mockito.mock;
+
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.server.config.AllProjectsName;
+import com.google.gerrit.server.config.AllProjectsNameProvider;
+import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.config.GitwebCgiConfig;
+import com.google.gerrit.server.config.GitwebConfig;
+import com.google.gerrit.server.config.SitePaths;
+import com.google.gerrit.server.git.LocalDiskRepositoryManager;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.project.ProjectCache;
+import com.google.inject.AbstractModule;
+import com.google.inject.Guice;
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.Repository;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+public class GitwebServletTest {
+  @Rule public TemporaryFolder temporaryFolder = new TemporaryFolder();
+
+  private Config cfg;
+  private SitePaths site;
+  private LocalDiskRepositoryManager repoManager;
+  private ProjectCache projectCache;
+  private PermissionBackend permissionBackendMock;
+  private GitwebCgiConfig gitWebCgiConfig;
+  private GitwebConfig gitWebConfig;
+  private GitwebServlet servlet;
+  private AllProjectsName allProjectsName;
+
+  @Before
+  public void setUp() throws Exception {
+    site = new SitePaths(temporaryFolder.newFolder().toPath());
+    site.resolve("git").toFile().mkdir();
+    cfg = new Config();
+    cfg.setString("gerrit", null, "basePath", "git");
+    repoManager =
+        Guice.createInjector(
+                new AbstractModule() {
+                  @Override
+                  protected void configure() {
+                    bind(SitePaths.class).toInstance(site);
+                    bind(Config.class).annotatedWith(GerritServerConfig.class).toInstance(cfg);
+                  }
+                })
+            .getInstance(LocalDiskRepositoryManager.class);
+    projectCache = mock(ProjectCache.class);
+    permissionBackendMock = mock(PermissionBackend.class);
+    gitWebCgiConfig = mock(GitwebCgiConfig.class);
+    gitWebConfig = mock(GitwebConfig.class);
+    allProjectsName = new AllProjectsName(AllProjectsNameProvider.DEFAULT);
+    // All-Projects must exist prior to calling GitwebServlet ctor
+    repoManager.createRepository(allProjectsName);
+    servlet =
+        new GitwebServlet(
+            repoManager,
+            projectCache,
+            permissionBackendMock,
+            null,
+            site,
+            cfg,
+            null,
+            null,
+            gitWebConfig,
+            gitWebCgiConfig,
+            allProjectsName);
+  }
+
+  @Test
+  public void projectRootSetToBasePathForSimpleRepository() throws Exception {
+    Project.NameKey foo = Project.nameKey("foo");
+    try (Repository repo = repoManager.createRepository(foo)) {
+      assertThat(servlet.getProjectRoot(foo))
+          .isEqualTo(repoManager.getBasePath(foo).toAbsolutePath().toString());
+    }
+  }
+
+  @Test
+  public void projectRootSetToBasePathForNestedRepository() throws Exception {
+    Project.NameKey baz = Project.nameKey("foo/bar/baz");
+    try (Repository repo = repoManager.createRepository(baz)) {
+      assertThat(servlet.getProjectRoot(baz))
+          .isEqualTo(repoManager.getBasePath(baz).toAbsolutePath().toString());
+    }
+  }
+}
diff --git a/javatests/com/google/gerrit/server/cache/mem/BUILD b/javatests/com/google/gerrit/server/cache/mem/BUILD
index a263c7b315..5ae4b73da3 100644
--- a/javatests/com/google/gerrit/server/cache/mem/BUILD
+++ b/javatests/com/google/gerrit/server/cache/mem/BUILD
@@ -4,6 +4,7 @@ junit_tests(
     name = "tests",
     srcs = glob(["*Test.java"]),
     deps = [
+        "//java/com/google/gerrit/metrics",
         "//java/com/google/gerrit/server",
         "//java/com/google/gerrit/server/cache/mem",
         "//lib:jgit",
diff --git a/javatests/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactoryTest.java b/javatests/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactoryTest.java
index 9b504a94f7..da4834126b 100644
--- a/javatests/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactoryTest.java
+++ b/javatests/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactoryTest.java
@@ -19,16 +19,25 @@ import static com.google.common.truth.Truth.assertThat;
 
 import com.google.common.cache.CacheLoader;
 import com.google.common.cache.LoadingCache;
+import com.google.common.cache.RemovalNotification;
 import com.google.common.cache.Weigher;
 import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.metrics.DisabledMetricMaker;
 import com.google.gerrit.server.cache.CacheBackend;
 import com.google.gerrit.server.cache.CacheDef;
+import com.google.gerrit.server.cache.ForwardingRemovalListener;
+import com.google.gerrit.server.git.WorkQueue;
+import com.google.gerrit.server.util.IdGenerator;
+import com.google.inject.Guice;
 import com.google.inject.TypeLiteral;
 import java.time.Duration;
 import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
 import java.util.concurrent.BrokenBarrierException;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.CyclicBarrier;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Executors;
@@ -48,20 +57,44 @@ public class DefaultMemoryCacheFactoryTest {
   private static final String TEST_CACHE = "test-cache";
   private static final long TEST_TIMEOUT_SEC = 1;
   private static final int TEST_CACHE_KEY = 1;
+  private static final int TEST_CACHE_VALUE = 2;
 
   private DefaultMemoryCacheFactory memoryCacheFactory;
+  private DefaultMemoryCacheFactory memoryCacheFactoryDirectExecutor;
+  private DefaultMemoryCacheFactory memoryCacheFactoryWithThreadPool;
   private Config memoryCacheConfig;
   private ScheduledExecutorService executor;
+  private Config memoryCacheConfigDirectExecutor;
+  private Config memoryCacheConfigWithThreadPool;
   private CyclicBarrier cacheGetStarted;
   private CyclicBarrier cacheGetCompleted;
+  private CyclicBarrier evictionReceived;
+  private ForwardingRemovalTrackerListener forwardingRemovalListener;
+  private WorkQueue workQueue;
 
   @Before
   public void setUp() {
+    IdGenerator idGenerator = Guice.createInjector().getInstance(IdGenerator.class);
+    workQueue = new WorkQueue(idGenerator, 10, new DisabledMetricMaker());
     memoryCacheConfig = new Config();
-    memoryCacheFactory = new DefaultMemoryCacheFactory(memoryCacheConfig, null);
+    memoryCacheConfigDirectExecutor = new Config();
+    memoryCacheConfigDirectExecutor.setInt("cache", null, "threads", 0);
+    memoryCacheConfigWithThreadPool = new Config();
+    memoryCacheConfigWithThreadPool.setInt("cache", null, "threads", 1);
+    forwardingRemovalListener = new ForwardingRemovalTrackerListener();
+    memoryCacheFactory =
+        new DefaultMemoryCacheFactory(
+            memoryCacheConfig, (cache) -> forwardingRemovalListener, workQueue);
+    memoryCacheFactoryDirectExecutor =
+        new DefaultMemoryCacheFactory(
+            memoryCacheConfigDirectExecutor, (cache) -> forwardingRemovalListener, workQueue);
+    memoryCacheFactoryWithThreadPool =
+        new DefaultMemoryCacheFactory(
+            memoryCacheConfigWithThreadPool, (cache) -> forwardingRemovalListener, workQueue);
     executor = Executors.newScheduledThreadPool(1);
     cacheGetStarted = new CyclicBarrier(2);
     cacheGetCompleted = new CyclicBarrier(2);
+    evictionReceived = new CyclicBarrier(2);
   }
 
   @Test
@@ -97,6 +130,46 @@ public class DefaultMemoryCacheFactoryTest {
   }
 
   @Test
+  public void shouldRunEvictionListenerInBackgroundByDefault() throws Exception {
+    shouldRunEvictionListenerInThreadPool(memoryCacheFactory, "ForkJoinPool");
+  }
+
+  @Test
+  public void shouldRunEvictionListenerInThreadPool() throws Exception {
+    shouldRunEvictionListenerInThreadPool(
+        memoryCacheFactoryWithThreadPool, DefaultMemoryCacheFactory.CACHE_EXECUTOR_PREFIX);
+  }
+
+  private void shouldRunEvictionListenerInThreadPool(
+      DefaultMemoryCacheFactory cacheFactory, String threadPoolPrefix) throws Exception {
+    LoadingCache<Integer, Integer> cache =
+        cacheFactory.build(newCacheDef(1), newCacheLoader(identity()), CacheBackend.CAFFEINE);
+
+    cache.put(TEST_CACHE_KEY, TEST_CACHE_VALUE);
+    cache.invalidate(TEST_CACHE_KEY);
+
+    assertThat(forwardingRemovalListener.contains(TEST_CACHE_KEY, TEST_CACHE_VALUE)).isFalse();
+
+    evictionReceived.await(TEST_TIMEOUT_SEC, TimeUnit.SECONDS);
+
+    assertThat(forwardingRemovalListener.contains(TEST_CACHE_KEY, TEST_CACHE_VALUE)).isTrue();
+    assertThat(forwardingRemovalListener.removalThreadName(TEST_CACHE_KEY))
+        .startsWith(threadPoolPrefix);
+  }
+
+  @Test
+  public void shouldRunEvictionListenerWithDirectExecutor() throws Exception {
+    LoadingCache<Integer, Integer> cache =
+        memoryCacheFactoryDirectExecutor.build(
+            newCacheDef(1), newCacheLoader(identity()), CacheBackend.CAFFEINE);
+
+    cache.put(TEST_CACHE_KEY, TEST_CACHE_VALUE);
+    cache.invalidate(TEST_CACHE_KEY);
+
+    assertThat(forwardingRemovalListener.contains(TEST_CACHE_KEY, TEST_CACHE_VALUE)).isTrue();
+  }
+
+  @Test
   public void shouldLoadAllKeysWithDisabledCache() throws Exception {
     LoadingCache<Integer, Integer> disabledCache =
         memoryCacheFactory.build(newCacheDef(0), newCacheLoader(identity()), CacheBackend.CAFFEINE);
@@ -146,6 +219,48 @@ public class DefaultMemoryCacheFactoryTest {
     };
   }
 
+  private class ForwardingRemovalTrackerListener extends ForwardingRemovalListener<Object, Object> {
+    private final ConcurrentHashMap<Object, Set<Object>> removalEvents;
+    private final ConcurrentHashMap<Object, String> removalThreads;
+
+    public ForwardingRemovalTrackerListener() {
+      super(null, null);
+
+      removalEvents = new ConcurrentHashMap<>();
+      removalThreads = new ConcurrentHashMap<>();
+    }
+
+    @Override
+    public void onRemoval(RemovalNotification<Object, Object> notification) {
+      Set<Object> setOfValues =
+          removalEvents.computeIfAbsent(
+              notification.getKey(),
+              (key) -> {
+                Set<Object> elements = ConcurrentHashMap.newKeySet();
+                return elements;
+              });
+      setOfValues.add(notification.getValue());
+
+      removalThreads.put(notification.getKey(), Thread.currentThread().getName());
+
+      try {
+        evictionReceived.await(TEST_TIMEOUT_SEC, TimeUnit.SECONDS);
+      } catch (InterruptedException | BrokenBarrierException | TimeoutException e) {
+        throw new IllegalStateException(e);
+      }
+    }
+
+    private boolean contains(Object key, Object value) {
+      return Optional.ofNullable(removalEvents.get(key))
+          .map(sv -> sv.contains(value))
+          .orElse(false);
+    }
+
+    private String removalThreadName(Object key) {
+      return removalThreads.get(key);
+    }
+  }
+
   private CacheDef<Integer, Integer> newCacheDef(long maximumWeight) {
     return new CacheDef<Integer, Integer>() {