diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java index 8352f57697..dc2639b051 100644 --- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java +++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java @@ -102,6 +102,7 @@ import com.google.gerrit.server.OptionUtil; import com.google.gerrit.server.OutputFormat; import com.google.gerrit.server.audit.AuditService; import com.google.gerrit.server.audit.ExtendedHttpAuditEvent; +import com.google.gerrit.server.cache.PerThreadCache; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.git.LockFailureException; import com.google.gerrit.server.permissions.GlobalPermission; @@ -278,7 +279,7 @@ public class RestApiServlet extends HttpServlet { RestResource rsrc = TopLevelResource.INSTANCE; ViewData viewData = null; - try { + try (PerThreadCache ignored = PerThreadCache.create()) { if (isCorsPreflight(req)) { doCorsPreflight(req, res); return; diff --git a/java/com/google/gerrit/server/cache/PerThreadCache.java b/java/com/google/gerrit/server/cache/PerThreadCache.java new file mode 100644 index 0000000000..81cfc44e6a --- /dev/null +++ b/java/com/google/gerrit/server/cache/PerThreadCache.java @@ -0,0 +1,128 @@ +// Copyright (C) 2018 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.cache; + +import static com.google.common.base.Preconditions.checkState; + +import com.google.common.base.Objects; +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.function.Supplier; + +/** + * Caches object instances for a request as {@link ThreadLocal} in the serving thread. + * + *

This class is intended to cache objects that have a high instantiation cost, are specific to + * the current request and potentially need to be instantiated multiple times while serving a + * request. + * + *

This is different from the key-value storage in {@code CurrentUser}: {@code CurrentUser} + * offers a key-value storage by providing thread-safe {@code get} and {@code put} methods. Once the + * value is retrieved through {@code get} there is not thread-safety anymore - apart from the the + * retrieved object guarantees. Depending on the implementation of {@code CurrentUser}, it might be + * shared between the request serving thread as well as sub- or background treads. + * + *

In comparison to that, this class guarantees thread safety even on non-thread-safe objects as + * it's cache is tied to the serving thread only. While allowing to cache non-thread-safe objects, + * it has the downside of not sharing any objects with background threads or executors. + * + *

Lastly, this class offers a cache, that requires callers to also provide a {@code Supplier} in + * case the object is not present in the cache, while {@code CurrentUser} provides a storage where + * just retrieving stored values is a valid operation. + */ +public class PerThreadCache implements AutoCloseable { + private static final ThreadLocal CACHE = new ThreadLocal<>(); + + /** + * 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. + */ + public static final class Key { + private final Class clazz; + private final ImmutableList identifiers; + + /** + * Returns a key based on the value's class and an identifier that uniquely identify the value. + * The identifier needs to implement {@code equals()} and {@hashCode()}. + */ + public static Key create(Class clazz, Object identifier) { + return new Key<>(clazz, ImmutableList.of(identifier)); + } + + /** + * Returns a key based on the value's class and a set of identifiers that uniquely identify the + * value. Identifiers need to implement {@code equals()} and {@hashCode()}. + */ + public static Key create(Class clazz, Object... identifiers) { + return new Key<>(clazz, ImmutableList.copyOf(identifiers)); + } + + public Key(Class clazz, ImmutableList identifiers) { + this.clazz = clazz; + this.identifiers = identifiers; + } + + @Override + public int hashCode() { + return Objects.hashCode(clazz, identifiers); + } + + @Override + public boolean equals(Object o) { + if (!(o instanceof Key)) { + return false; + } + Key other = (Key) o; + return this.clazz == other.clazz && this.identifiers.equals(other.identifiers); + } + } + + public static PerThreadCache create() { + checkState(CACHE.get() == null, "called create() twice on the same request"); + PerThreadCache cache = new PerThreadCache(); + CACHE.set(cache); + return cache; + } + + @Nullable + public static PerThreadCache get() { + return CACHE.get(); + } + + private final Map cache = Maps.newHashMapWithExpectedSize(10); + + private PerThreadCache() {} + + /** + * Returns an instance of {@code T} that was either loaded from the cache or obtained from the + * provided {@link Supplier}. + */ + public T get(Key key, Supplier loader) { + T value = (T) cache.get(key); + if (value == null) { + value = loader.get(); + cache.put(key, value); + } + return value; + } + + @Override + public void close() { + CACHE.remove(); + } +} diff --git a/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java b/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java index 8487d6e630..93f1683325 100644 --- a/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java +++ b/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java @@ -31,6 +31,7 @@ import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.PeerDaemonUser; import com.google.gerrit.server.account.CapabilityCollection; +import com.google.gerrit.server.cache.PerThreadCache; import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectState; @@ -108,7 +109,15 @@ public class DefaultPermissionBackend extends PermissionBackend { try { ProjectState state = projectCache.checkedGet(project); if (state != null) { - return projectControlFactory.create(user, state).asForProject().database(db); + PerThreadCache perThreadCache = PerThreadCache.get(); + if (perThreadCache == null) { + return projectControlFactory.create(user, state).asForProject().database(db); + } + PerThreadCache.Key cacheKey = + PerThreadCache.Key.create(ProjectControl.class, project, user.getCacheKey()); + ProjectControl control = + perThreadCache.get(cacheKey, () -> projectControlFactory.create(user, state)); + return control.asForProject().database(db); } return FailedPermissionBackend.project("not found", new NoSuchProjectException(project)); } catch (IOException e) { diff --git a/javatests/com/google/gerrit/server/cache/BUILD b/javatests/com/google/gerrit/server/cache/BUILD new file mode 100644 index 0000000000..6eca1722cf --- /dev/null +++ b/javatests/com/google/gerrit/server/cache/BUILD @@ -0,0 +1,12 @@ +load("//tools/bzl:junit.bzl", "junit_tests") + +junit_tests( + name = "tests", + srcs = ["PerThreadCacheTest.java"], + deps = [ + "//java/com/google/gerrit/server", + "//lib:guava", + "//lib:junit", + "//lib:truth", + ], +) diff --git a/javatests/com/google/gerrit/server/cache/PerThreadCacheTest.java b/javatests/com/google/gerrit/server/cache/PerThreadCacheTest.java new file mode 100644 index 0000000000..ae5e911d14 --- /dev/null +++ b/javatests/com/google/gerrit/server/cache/PerThreadCacheTest.java @@ -0,0 +1,86 @@ +// Copyright (C) 2018 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.cache; + +import static com.google.common.truth.Truth.assertThat; + +import java.util.function.Supplier; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; + +public class PerThreadCacheTest { + @Rule public ExpectedException exception = ExpectedException.none(); + + @Test + public void key_respectsClass() { + assertThat(PerThreadCache.Key.create(String.class)) + .isEqualTo(PerThreadCache.Key.create(String.class)); + assertThat(PerThreadCache.Key.create(String.class)) + .isNotEqualTo(PerThreadCache.Key.create(Integer.class)); + } + + @Test + public void key_respectsIdentifiers() { + assertThat(PerThreadCache.Key.create(String.class, "id1")) + .isEqualTo(PerThreadCache.Key.create(String.class, "id1")); + assertThat(PerThreadCache.Key.create(String.class, "id1")) + .isNotEqualTo(PerThreadCache.Key.create(String.class, "id2")); + } + + @Test + public void endToEndCache() { + try (PerThreadCache ignored = PerThreadCache.create()) { + PerThreadCache cache = PerThreadCache.get(); + PerThreadCache.Key key1 = PerThreadCache.Key.create(String.class); + + String value1 = cache.get(key1, () -> "value1"); + assertThat(value1).isEqualTo("value1"); + + Supplier neverCalled = + () -> { + throw new IllegalStateException("this method must not be called"); + }; + assertThat(cache.get(key1, neverCalled)).isEqualTo("value1"); + } + } + + @Test + public void cleanUp() { + PerThreadCache.Key key = PerThreadCache.Key.create(String.class); + try (PerThreadCache ignored = PerThreadCache.create()) { + PerThreadCache cache = PerThreadCache.get(); + String value1 = cache.get(key, () -> "value1"); + assertThat(value1).isEqualTo("value1"); + } + + // 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()) { + PerThreadCache cache = PerThreadCache.get(); + String value1 = cache.get(key, () -> "value2"); + assertThat(value1).isEqualTo("value2"); + } + } + + @Test + public void doubleInstantiationFails() { + try (PerThreadCache ignored = PerThreadCache.create()) { + exception.expect(IllegalStateException.class); + exception.expectMessage("called create() twice on the same request"); + PerThreadCache.create(); + } + } +}