Streamline large synch blocks in list/TMCH CA loading (#3183)

For reserved/premium lists:
Use double-check locking so that subsequent calls to get the entire map
of entries don't need to even check the locking object. This makes
things quicker and removes lock-tracking overhead.

For TMCH CA:
we can just remove the synchronization block entirely. Everything inside
of it is either constants (e.g. ROOT_CERTS) or a Guava loading cache
(CRL_CACHE) which takes care of synchronization for us anyway.

core/src/main/java/google/registry/model/tld/label/PremiumList.java

@@ -59,10 +59,10 @@ public final class PremiumList extends BaseDomainLabelList<BigDecimal, PremiumEn
    * Mapping from unqualified domain names to their prices.
    *
    * <p>This field requires special treatment since we want to lazy load it. We have to remove it
-   * from the immutability contract so we can modify it after construction and we have to handle the
+   * from the immutability contract so we can modify it after construction, and we have to handle
-   * database processing on our own so we can detach it after load.
+   * the database processing on our own so we can detach it after load.
    */
-  @Insignificant @Transient ImmutableMap<String, BigDecimal> labelsToPrices;
+  @Insignificant @Transient volatile ImmutableMap<String, BigDecimal> labelsToPrices;
 
   @Column(nullable = false)
   BloomFilter<String> bloomFilter;
@@ -76,18 +76,27 @@ public final class PremiumList extends BaseDomainLabelList<BigDecimal, PremiumEn
    * Returns a {@link Map} of domain labels to prices.
    *
    * <p>Note that this is lazily loaded and thus must be called inside a transaction. You generally
-   * should not be using this anyway as it's inefficient to load all of the PremiumEntry rows if you
+   * should not be using this anyway as it's inefficient to load all the PremiumEntry rows if you
    * don't need them. To check prices, use {@link PremiumListDao#getPremiumPrice} instead.
+   *
+   * <p>We use locking to memoize the resulting object. We cannot use a simple memoizing Supplier
+   * because we need to be able to set this value when creating the lists.
    */
-  public synchronized ImmutableMap<String, BigDecimal> getLabelsToPrices() {
+  public ImmutableMap<String, BigDecimal> getLabelsToPrices() {
     if (labelsToPrices == null) {
-      labelsToPrices =
+      synchronized (this) {
-          PremiumListDao.loadAllPremiumEntries(name).stream()
+        // Extra null check to avoid race conditions
-              .collect(
+        if (labelsToPrices == null) {
-                  toImmutableMap(
+          labelsToPrices =
-                      PremiumEntry::getDomainLabel,
+              PremiumListDao.loadAllPremiumEntries(name).stream()
-                      // Set the correct amount of precision for the premium list's currency.
+                  .collect(
-                      premiumEntry -> convertAmountToMoney(premiumEntry.getValue()).getAmount()));
+                      toImmutableMap(
+                          PremiumEntry::getDomainLabel,
+                          // Set the correct amount of precision for the list's currency.
+                          premiumEntry ->
+                              convertAmountToMoney(premiumEntry.getValue()).getAmount()));
+        }
+      }
     }
     return labelsToPrices;
   }

core/src/main/java/google/registry/model/tld/label/ReservedList.java

@@ -23,7 +23,6 @@ import static google.registry.config.RegistryConfig.getDomainLabelListCacheDurat
 import static google.registry.model.tld.label.ReservationType.FULLY_BLOCKED;
 import static google.registry.persistence.transaction.QueryComposer.Comparator.EQ;
 import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
-import static google.registry.util.CollectionUtils.nullToEmpty;
 
 import com.github.benmanes.caffeine.cache.LoadingCache;
 import com.google.common.base.Splitter;
@@ -69,7 +68,7 @@ public final class ReservedList
    * from the immutability contract so we can modify it after construction and we have to handle the
    * database processing on our own so we can detach it after load.
    */
-  @Insignificant @Transient Map<String, ReservedListEntry> reservedListMap;
+  @Insignificant @Transient volatile ImmutableMap<String, ReservedListEntry> reservedListMap;
 
   @RecursivePreRemove
   void preRemove() {
@@ -149,7 +148,7 @@ public final class ReservedList
     }
 
     /** A builder for constructing {@link ReservedListEntry} objects, since they are immutable. */
-    private static class Builder
+    public static class Builder
         extends DomainLabelEntry.Builder<ReservedListEntry, ReservedListEntry.Builder> {
 
       Builder() {}
@@ -185,19 +184,27 @@ public final class ReservedList
    *
    * <p>Note that this involves a database fetch of a potentially large number of elements and
    * should be avoided unless necessary.
+   *
+   * <p>We use locking to memoize the resulting object. We cannot use a simple memoizing Supplier
+   * because we need to be able to set this value when creating the lists.
    */
-  public synchronized ImmutableMap<String, ReservedListEntry> getReservedListEntries() {
+  public ImmutableMap<String, ReservedListEntry> getReservedListEntries() {
     if (reservedListMap == null) {
-      reservedListMap =
+      synchronized (this) {
-          tm().reTransact(
+        // Extra null check to avoid race conditions
-                  () ->
+        if (reservedListMap == null) {
-                      tm()
+          reservedListMap =
-                          .createQueryComposer(ReservedListEntry.class)
+              tm().reTransact(
-                          .where("revisionId", EQ, revisionId)
+                      () ->
-                          .stream()
+                          tm()
-                          .collect(toImmutableMap(ReservedListEntry::getDomainLabel, e -> e)));
+                              .createQueryComposer(ReservedListEntry.class)
+                              .where("revisionId", EQ, revisionId)
+                              .stream()
+                              .collect(toImmutableMap(ReservedListEntry::getDomainLabel, e -> e)));
+        }
+      }
     }
-    return ImmutableMap.copyOf(nullToEmpty(reservedListMap));
+    return reservedListMap;
   }
 
   /**
@@ -220,7 +227,7 @@ public final class ReservedList
    */
   public static ImmutableSet<ReservationType> getReservationTypes(String label, String tld) {
     checkNotNull(label, "label");
-    if (label.length() == 0) {
+    if (label.isEmpty()) {
       return ImmutableSet.of(FULLY_BLOCKED);
     }
     return getReservedListEntries(label, tld).stream()

core/src/main/java/google/registry/tmch/TmchCertificateAuthority.java

@@ -127,9 +127,7 @@ public final class TmchCertificateAuthority {
    * @see X509Utils#verifyCertificate
    */
   public void verify(X509Certificate cert) throws GeneralSecurityException {
-    synchronized (TmchCertificateAuthority.class) {
+    X509Utils.verifyCertificate(getAndValidateRoot(), getCrl(), cert, clock.now());
-      X509Utils.verifyCertificate(getAndValidateRoot(), getCrl(), cert, clock.now());
-    }
   }
 
   /**
@@ -157,33 +155,23 @@ public final class TmchCertificateAuthority {
   }
 
   public X509Certificate getAndValidateRoot() throws GeneralSecurityException {
-    try {
+    X509Certificate root = ROOT_CERTS.get(tmchCaMode);
-      X509Certificate root = ROOT_CERTS.get(tmchCaMode);
+    // The current production certificate expires on 2042-11-15. Future code monkey be reminded,
-      // The current production certificate expires on 2023-07-23. Future code monkey be reminded,
+    // if you are looking at this code because the next line throws an exception, ask ICANN for a
-      // if you are looking at this code because the next line throws an exception, ask ICANN for a
+    // new root certificate! (preferably before the current one expires...)
-      // new root certificate! (preferably before the current one expires...)
+    root.checkValidity(Date.from(clock.now()));
-      root.checkValidity(Date.from(clock.now()));
+    return root;
-      return root;
-    } catch (Exception e) {
-      if (e instanceof GeneralSecurityException generalSecurityException) {
-        throw generalSecurityException;
-      } else if (e instanceof RuntimeException runtimeException) {
-        throw runtimeException;
-      }
-      throw new RuntimeException(e);
-    }
   }
 
   public X509CRL getCrl() throws GeneralSecurityException {
     try {
       return CRL_CACHE.get(tmchCaMode);
-    } catch (Exception e) {
+    } catch (RuntimeException e) {
       if (e.getCause() instanceof GeneralSecurityException generalSecurityException) {
         throw generalSecurityException;
-      } else if (e instanceof RuntimeException runtimeException) {
+      } else {
-        throw runtimeException;
+        throw e;
       }
-      throw new RuntimeException(e);
     }
   }
 }