diff --git a/core/src/main/java/google/registry/model/tld/label/PremiumList.java b/core/src/main/java/google/registry/model/tld/label/PremiumList.java index 709f68767b0..caa05b77248 100644 --- a/core/src/main/java/google/registry/model/tld/label/PremiumList.java +++ b/core/src/main/java/google/registry/model/tld/label/PremiumList.java @@ -59,10 +59,13 @@ public final class PremiumList extends BaseDomainLabelListThis 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 - * database processing on our own so we can detach it after load. + * 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 ImmutableMap labelsToPrices; + @Insignificant @Transient volatile ImmutableMap labelsToPrices; + + /** A lock to make sure we don't load the same price map twice. */ + @Insignificant @Transient private final Object labelsToPricesLock = new Object(); @Column(nullable = false) BloomFilter bloomFilter; @@ -76,18 +79,27 @@ public CurrencyUnit getCurrency() { * Returns a {@link Map} of domain labels to prices. * *

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. + * + *

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 getLabelsToPrices() { + public ImmutableMap getLabelsToPrices() { if (labelsToPrices == null) { - labelsToPrices = - PremiumListDao.loadAllPremiumEntries(name).stream() - .collect( - toImmutableMap( - PremiumEntry::getDomainLabel, - // Set the correct amount of precision for the premium list's currency. - premiumEntry -> convertAmountToMoney(premiumEntry.getValue()).getAmount())); + synchronized (labelsToPricesLock) { + // Extra null check to avoid race conditions + if (labelsToPrices == null) { + labelsToPrices = + PremiumListDao.loadAllPremiumEntries(name).stream() + .collect( + toImmutableMap( + PremiumEntry::getDomainLabel, + // Set the correct amount of precision for the list's currency. + premiumEntry -> + convertAmountToMoney(premiumEntry.getValue()).getAmount())); + } + } } return labelsToPrices; } diff --git a/core/src/main/java/google/registry/model/tld/label/ReservedList.java b/core/src/main/java/google/registry/model/tld/label/ReservedList.java index ce37884e14e..f256871c378 100644 --- a/core/src/main/java/google/registry/model/tld/label/ReservedList.java +++ b/core/src/main/java/google/registry/model/tld/label/ReservedList.java @@ -23,7 +23,6 @@ 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,10 @@ 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 reservedListMap; + @Insignificant @Transient volatile ImmutableMap reservedListMap; + + /** A lock to make sure we don't load the reserved list map twice. */ + @Insignificant @Transient private final Object reservedListMapLoadLock = new Object(); @RecursivePreRemove void preRemove() { @@ -149,7 +151,7 @@ public String toString() { } /** A builder for constructing {@link ReservedListEntry} objects, since they are immutable. */ - private static class Builder + public static class Builder extends DomainLabelEntry.Builder { Builder() {} @@ -185,19 +187,27 @@ public boolean isInUse() { * *

Note that this involves a database fetch of a potentially large number of elements and * should be avoided unless necessary. + * + *

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 getReservedListEntries() { + public ImmutableMap getReservedListEntries() { if (reservedListMap == null) { - reservedListMap = - tm().reTransact( - () -> - tm() - .createQueryComposer(ReservedListEntry.class) - .where("revisionId", EQ, revisionId) - .stream() - .collect(toImmutableMap(ReservedListEntry::getDomainLabel, e -> e))); + synchronized (reservedListMapLoadLock) { + // Extra null check to avoid race conditions + if (reservedListMap == null) { + reservedListMap = + tm().reTransact( + () -> + tm() + .createQueryComposer(ReservedListEntry.class) + .where("revisionId", EQ, revisionId) + .stream() + .collect(toImmutableMap(ReservedListEntry::getDomainLabel, e -> e))); + } + } } - return ImmutableMap.copyOf(nullToEmpty(reservedListMap)); + return reservedListMap; } /** @@ -220,7 +230,7 @@ public static Optional get(String listName) { */ public static ImmutableSet 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() diff --git a/core/src/main/java/google/registry/tmch/TmchCertificateAuthority.java b/core/src/main/java/google/registry/tmch/TmchCertificateAuthority.java index 555f552c0e3..5f89da86649 100644 --- a/core/src/main/java/google/registry/tmch/TmchCertificateAuthority.java +++ b/core/src/main/java/google/registry/tmch/TmchCertificateAuthority.java @@ -127,9 +127,7 @@ private static ImmutableMap loadRootCertificates() * @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 void updateCrl(String asciiCrl, String url) throws GeneralSecurityExcepti } public X509Certificate getAndValidateRoot() throws GeneralSecurityException { - try { - X509Certificate root = ROOT_CERTS.get(tmchCaMode); - // 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 - // new root certificate! (preferably before the current one expires...) - root.checkValidity(Date.from(clock.now())); - return root; - } catch (Exception e) { - if (e instanceof GeneralSecurityException generalSecurityException) { - throw generalSecurityException; - } else if (e instanceof RuntimeException runtimeException) { - throw runtimeException; - } - throw new RuntimeException(e); - } + X509Certificate root = ROOT_CERTS.get(tmchCaMode); + // The current production certificate expires on 2042-11-15. Future code monkey be reminded, + // 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...) + root.checkValidity(Date.from(clock.now())); + return root; } 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) { - throw runtimeException; + } else { + throw e; } - throw new RuntimeException(e); } } }