Skip to content

Commit 7a94735

Browse files
committed
Use transaction time for deletion time cache ticker
Basically, what happened is that the cache's expireAfterWrite was being called some number of milliseconds (say, 50-100) after the transaction was started. That method used the transaction time instead of the current time, so as a result the entries were sticking around 50-100ms longer in the cache than they should have been. This fix contains two parts, each of which I believe would be sufficient on their own to fix the issue: 1. Use the currentTime passed in in Expiry::expireAfterCreate 2. Use the transaction time in the cache's Ticker. This keeps everything on the same schedule.
1 parent 6863f67 commit 7a94735

File tree

2 files changed

+29
-6
lines changed

2 files changed

+29
-6
lines changed

core/src/main/java/google/registry/flows/domain/DomainDeletionTimeCache.java

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,11 @@
2020
import com.github.benmanes.caffeine.cache.Caffeine;
2121
import com.github.benmanes.caffeine.cache.Expiry;
2222
import com.github.benmanes.caffeine.cache.LoadingCache;
23+
import com.github.benmanes.caffeine.cache.Ticker;
2324
import com.google.common.collect.ImmutableSet;
2425
import google.registry.model.ForeignKeyUtils;
2526
import google.registry.model.domain.Domain;
27+
import google.registry.util.DateTimeUtils;
2628
import java.util.Optional;
2729
import org.joda.time.DateTime;
2830

@@ -55,9 +57,9 @@
5557
public class DomainDeletionTimeCache {
5658

5759
// Max expiry time is ten minutes
58-
private static final int MAX_EXPIRY_MILLIS = 10 * 60 * 1000;
60+
private static final long NANOS_IN_ONE_MILLISECOND = 100000L;
61+
private static final long MAX_EXPIRY_NANOS = 10L * 60L * 1000L * NANOS_IN_ONE_MILLISECOND;
5962
private static final int MAX_ENTRIES = 500;
60-
private static final int NANOS_IN_ONE_MILLISECOND = 100000;
6163

6264
/**
6365
* Expire after the max duration, or after the domain is set to drop (whichever comes first).
@@ -71,9 +73,13 @@ public class DomainDeletionTimeCache {
7173
new Expiry<>() {
7274
@Override
7375
public long expireAfterCreate(String key, DateTime value, long currentTime) {
74-
long millisUntilDeletion = value.getMillis() - tm().getTransactionTime().getMillis();
75-
return NANOS_IN_ONE_MILLISECOND
76-
* Math.max(0L, Math.min(MAX_EXPIRY_MILLIS, millisUntilDeletion));
76+
// Watch out for Long overflow
77+
long deletionTimeNanos =
78+
value.equals(DateTimeUtils.END_OF_TIME)
79+
? Long.MAX_VALUE
80+
: value.getMillis() * NANOS_IN_ONE_MILLISECOND;
81+
long nanosUntilDeletion = deletionTimeNanos - currentTime;
82+
return Math.max(0L, Math.min(MAX_EXPIRY_NANOS, nanosUntilDeletion));
7783
}
7884

7985
/** Reset the time entirely on update, as if we were creating the entry anew. */
@@ -101,9 +107,14 @@ public long expireAfterRead(
101107
return mostRecentResource == null ? null : mostRecentResource.deletionTime();
102108
};
103109

110+
// Unfortunately, maintenance tasks aren't necessarily already in a transaction
111+
private static final Ticker TRANSACTION_TIME_TICKER =
112+
() -> tm().reTransact(() -> tm().getTransactionTime().getMillis() * NANOS_IN_ONE_MILLISECOND);
113+
104114
public static DomainDeletionTimeCache create() {
105115
return new DomainDeletionTimeCache(
106116
Caffeine.newBuilder()
117+
.ticker(TRANSACTION_TIME_TICKER)
107118
.expireAfter(EXPIRY_POLICY)
108119
.maximumSize(MAX_ENTRIES)
109120
.build(CACHE_LOADER));

core/src/test/java/google/registry/flows/domain/DomainDeletionTimeCacheTest.java

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import google.registry.testing.FakeClock;
2727
import java.util.Optional;
2828
import org.joda.time.DateTime;
29+
import org.joda.time.Duration;
2930
import org.junit.jupiter.api.BeforeEach;
3031
import org.junit.jupiter.api.Test;
3132
import org.junit.jupiter.api.extension.RegisterExtension;
@@ -67,7 +68,8 @@ void testCache_returnsOldData() {
6768
Domain domain = persistActiveDomain("domain.tld");
6869
assertThat(getDeletionTimeFromCache("domain.tld")).hasValue(END_OF_TIME);
6970
persistDomainAsDeleted(domain, clock.nowUtc().plusDays(1));
70-
// Without intervention, the cache should have the old data
71+
// Without intervention, the cache should have the old data, even if a few minutes have passed
72+
clock.advanceBy(Duration.standardMinutes(5));
7173
assertThat(getDeletionTimeFromCache("domain.tld")).hasValue(END_OF_TIME);
7274
}
7375

@@ -79,6 +81,16 @@ void testCache_returnsNewDataAfterDomainCreate() {
7981
assertThat(getDeletionTimeFromCache("domain.tld")).hasValue(clock.nowUtc().plusDays(1));
8082
}
8183

84+
@Test
85+
void testCache_expires() {
86+
Domain domain = persistActiveDomain("domain.tld");
87+
assertThat(getDeletionTimeFromCache("domain.tld")).hasValue(END_OF_TIME);
88+
DateTime elevenMinutesFromNow = clock.nowUtc().plusMinutes(11);
89+
persistDomainAsDeleted(domain, elevenMinutesFromNow);
90+
clock.advanceBy(Duration.standardMinutes(30));
91+
assertThat(getDeletionTimeFromCache("domain.tld")).hasValue(elevenMinutesFromNow);
92+
}
93+
8294
private Optional<DateTime> getDeletionTimeFromCache(String domainName) {
8395
return tm().transact(() -> cache.getDeletionTimeForDomain(domainName));
8496
}

0 commit comments

Comments
 (0)