diff --git a/CHANGELOG.md b/CHANGELOG.md index 0276b67e25..76e9db699b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ ### Internal - Hold the hostname cache on `SentryOptions` instead of a static singleton ([#6117](https://github.com/getsentry/sentry-java/pull/6117)) +- Measure the hostname cache TTL on the monotonic ticker the options provide, so that a device time change no longer shortens or extends it ([#6100](https://github.com/getsentry/sentry-java/pull/6100)) ### Dependencies diff --git a/sentry/src/main/java/io/sentry/HostnameCache.java b/sentry/src/main/java/io/sentry/HostnameCache.java index b4a19124b8..dd1a215703 100644 --- a/sentry/src/main/java/io/sentry/HostnameCache.java +++ b/sentry/src/main/java/io/sentry/HostnameCache.java @@ -1,5 +1,7 @@ package io.sentry; +import io.sentry.time.Deadline; +import io.sentry.time.MonotonicTicker; import io.sentry.util.Objects; import java.net.InetAddress; import java.util.concurrent.Callable; @@ -16,14 +18,15 @@ import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.jetbrains.annotations.VisibleForTesting; /** * Time sensitive cache in charge of keeping track of the hostname. The {@code * InetAddress.getLocalHost().getCanonicalHostName()} call can be quite expensive and could be * called for the creation of each {@link SentryEvent}. This system will prevent unnecessary costs - * by keeping track of the hostname for a period defined during the construction. For performance - * purposes, the operation of retrieving the hostname will automatically fail after a period of time - * defined by {@link #GET_HOSTNAME_TIMEOUT} without result. + * by keeping track of the hostname for a period defined by {@link #HOSTNAME_CACHE_DURATION}. For + * performance purposes, the operation of retrieving the hostname will automatically fail after a + * period of time defined by {@link #GET_HOSTNAME_TIMEOUT} without result. * *

One instance is held per {@link SentryOptions} and should be obtained through {@link * SentryOptions#getHostnameCache()}. @@ -38,14 +41,13 @@ public final class HostnameCache { /** How long the worker thread may stay idle before it self-terminates. */ private static final long THREAD_KEEP_ALIVE_SECONDS = 30; - /** Time for which the cache is kept. */ - private final long cacheDuration; + private final @NotNull MonotonicTicker ticker; /** Current value for hostname (might change over time). */ @Nullable private volatile String hostname; - /** Time at which the cache should expire. */ - private volatile long expirationTimestamp; + /** When the cached hostname goes stale. */ + private volatile @NotNull Deadline cacheFreshUntil; /** Whether a cache update thread is currently running or not. */ private final @NotNull AtomicBoolean updateRunning = new AtomicBoolean(false); @@ -59,27 +61,30 @@ public final class HostnameCache { * SentryOptions#getHostnameCache()}. Every instance resolves the hostname as soon as it is built * and caches it on its own, so another instance would repeat that blocking lookup instead of * sharing the cached value. + * + * @param ticker the ticker the cache lifetime is measured on */ - HostnameCache() { - this(HOSTNAME_CACHE_DURATION); - } - - HostnameCache(long cacheDuration) { + HostnameCache(final @NotNull MonotonicTicker ticker) { // avoid method refs on Android due to some issues with older AGP setups // noinspection Convert2MethodRef - this(cacheDuration, () -> InetAddress.getLocalHost()); + this(() -> InetAddress.getLocalHost(), ticker); } /** * Sets up a cache for the hostname. * - * @param cacheDuration cache duration in milliseconds. * @param getLocalhost a callback to obtain the localhost address - this is mostly here because of * testability + * @param ticker the ticker the cache lifetime is measured on */ - HostnameCache(long cacheDuration, final @NotNull Callable getLocalhost) { - this.cacheDuration = cacheDuration; + @VisibleForTesting + HostnameCache( + final @NotNull Callable getLocalhost, final @NotNull MonotonicTicker ticker) { this.getLocalhost = Objects.requireNonNull(getLocalhost, "getLocalhost is required"); + this.ticker = Objects.requireNonNull(ticker, "ticker is required"); + // Nothing resolved yet, so the cache is stale rather than fresh until updateCache says + // otherwise. + this.cacheFreshUntil = Deadline.passed(ticker); // A single thread executor whose worker thread times out while idle, so no thread is kept // alive between the infrequent cache refreshes and nothing has to shut it down. final @NotNull ThreadPoolExecutor executor = @@ -104,8 +109,7 @@ public final class HostnameCache { */ @Nullable public String getHostname() { - if (expirationTimestamp < System.currentTimeMillis() - && updateRunning.compareAndSet(false, true)) { + if (cacheFreshUntil.hasPassed() && updateRunning.compareAndSet(false, true)) { updateCache(); } @@ -118,7 +122,8 @@ private void updateCache() { () -> { try { hostname = getLocalhost.call().getCanonicalHostName(); - expirationTimestamp = System.currentTimeMillis() + cacheDuration; + cacheFreshUntil = + Deadline.after(ticker, HOSTNAME_CACHE_DURATION, TimeUnit.MILLISECONDS); } finally { updateRunning.set(false); } @@ -151,7 +156,7 @@ private void updateCache() { } private void handleCacheUpdateFailure() { - expirationTimestamp = System.currentTimeMillis() + TimeUnit.SECONDS.toMillis(1); + cacheFreshUntil = Deadline.after(ticker, 1, TimeUnit.SECONDS); } private static final class HostnameCacheThreadFactory implements ThreadFactory { diff --git a/sentry/src/main/java/io/sentry/SentryOptions.java b/sentry/src/main/java/io/sentry/SentryOptions.java index f9b9fb13c9..3706a9f0a1 100644 --- a/sentry/src/main/java/io/sentry/SentryOptions.java +++ b/sentry/src/main/java/io/sentry/SentryOptions.java @@ -543,7 +543,7 @@ public class SentryOptions implements RateLimiterConfig { */ @ApiStatus.Internal private final @NotNull LazyEvaluator hostnameCache = - new LazyEvaluator<>(() -> new HostnameCache()); + new LazyEvaluator<>(() -> new HostnameCache(getMonotonicTicker())); private final @NotNull List performanceCollectors = new ArrayList<>(); diff --git a/sentry/src/test/java/io/sentry/HostnameCacheTest.kt b/sentry/src/test/java/io/sentry/HostnameCacheTest.kt index 6d0398858a..b72b9d4051 100644 --- a/sentry/src/test/java/io/sentry/HostnameCacheTest.kt +++ b/sentry/src/test/java/io/sentry/HostnameCacheTest.kt @@ -2,7 +2,7 @@ package io.sentry import com.google.common.truth.Truth.assertThat import io.sentry.test.getProperty -import io.sentry.test.injectForField +import io.sentry.time.TestMonotonicTicker import java.net.InetAddress import java.util.concurrent.ThreadPoolExecutor import java.util.concurrent.TimeUnit @@ -16,7 +16,7 @@ class HostnameCacheTest { private fun getSut(): HostnameCache { val address = mock() whenever(address.canonicalHostName).thenReturn("myhost") - return HostnameCache(TimeUnit.HOURS.toMillis(1)) { address } + return HostnameCache({ address }, TestMonotonicTicker()) } @Test @@ -25,13 +25,32 @@ class HostnameCacheTest { assertThat(cache.hostname).isEqualTo("myhost") } + @Test + fun `hostname is re-resolved only once the cache duration has elapsed`() { + val ticker = TestMonotonicTicker() + val address = mock() + whenever(address.canonicalHostName).thenReturn("first", "second") + val cache = HostnameCache({ address }, ticker) + + assertThat(cache.hostname).isEqualTo("first") + + ticker.advance(4, TimeUnit.HOURS) + assertThat(cache.hostname).isEqualTo("first") + + ticker.advance(1, TimeUnit.HOURS) + assertThat(cache.hostname).isEqualTo("second") + } + @Test fun `a refresh that cannot be queued does not stop later refreshes`() { - val cache = getSut() - // Reject the next submit the way an executor that could not start a thread would, and mark the - // cache stale so that reading the hostname attempts a refresh. + val ticker = TestMonotonicTicker() + val address = mock() + whenever(address.canonicalHostName).thenReturn("myhost") + val cache = HostnameCache({ address }, ticker) + // Reject the next submit the way an executor that could not start a thread would, and let the + // cache go stale so that reading the hostname attempts a refresh. cache.getProperty("executorService").shutdown() - cache.injectForField("expirationTimestamp", 0L) + ticker.advance(5, TimeUnit.HOURS) assertThat(cache.hostname).isEqualTo("myhost") diff --git a/sentry/src/test/java/io/sentry/MainEventProcessorTest.kt b/sentry/src/test/java/io/sentry/MainEventProcessorTest.kt index cded7e8775..7529d0a3e5 100644 --- a/sentry/src/test/java/io/sentry/MainEventProcessorTest.kt +++ b/sentry/src/test/java/io/sentry/MainEventProcessorTest.kt @@ -7,9 +7,11 @@ import io.sentry.protocol.DebugMeta import io.sentry.protocol.SdkVersion import io.sentry.protocol.SentryTransaction import io.sentry.protocol.User +import io.sentry.time.TestMonotonicTicker import io.sentry.util.HintUtils import java.lang.RuntimeException import java.net.InetAddress +import java.util.concurrent.TimeUnit import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFalse @@ -17,7 +19,6 @@ import kotlin.test.assertNotNull import kotlin.test.assertNull import kotlin.test.assertSame import kotlin.test.assertTrue -import org.awaitility.kotlin.await import org.mockito.kotlin.mock import org.mockito.kotlin.reset import org.mockito.kotlin.times @@ -28,6 +29,7 @@ class MainEventProcessorTest { class Fixture { val scopes = mock() val getLocalhost = mock() + val hostnameCacheTicker = TestMonotonicTicker() // Built in getSut() rather than here: the constructor resolves the hostname straight away, // so it has to run after getLocalhost is stubbed. lateinit var hostnameCache: HostnameCache @@ -53,7 +55,6 @@ class MainEventProcessorTest { serverName: String? = "server", host: String? = null, resolveHostDelay: Long? = null, - hostnameCacheDuration: Long = 10, proguardUuid: String? = null, bundleIds: List? = null, modules: Map? = null, @@ -80,7 +81,7 @@ class MainEventProcessorTest { } whenever(scopes.options).thenReturn(sentryOptions) sentryTracer = SentryTracer(TransactionContext("", ""), scopes) - hostnameCache = HostnameCache(hostnameCacheDuration) { getLocalhost } + hostnameCache = HostnameCache({ getLocalhost }, hostnameCacheTicker) return MainEventProcessor(sentryOptions) } @@ -428,7 +429,7 @@ class MainEventProcessorTest { @Test fun `uses cache to retrieve servername for subsequent events`() { - val processor = fixture.getSut(serverName = null, host = "aHost", hostnameCacheDuration = 1000) + val processor = fixture.getSut(serverName = null, host = "aHost") val firstEvent = SentryEvent() processor.process(firstEvent, Hint()) assertEquals("aHost", firstEvent.serverName) @@ -447,12 +448,11 @@ class MainEventProcessorTest { reset(fixture.getLocalhost) whenever(fixture.getLocalhost.canonicalHostName).thenReturn("newHost") + fixture.hostnameCacheTicker.advance(6, TimeUnit.HOURS) - await.untilAsserted { - val secondEvent = SentryEvent() - processor.process(secondEvent, Hint()) - assertEquals("newHost", secondEvent.serverName) - } + val secondEvent = SentryEvent() + processor.process(secondEvent, Hint()) + assertEquals("newHost", secondEvent.serverName) } @Test diff --git a/sentry/src/test/java/io/sentry/SentryOptionsTest.kt b/sentry/src/test/java/io/sentry/SentryOptionsTest.kt index 101a136f64..284c0a98cd 100644 --- a/sentry/src/test/java/io/sentry/SentryOptionsTest.kt +++ b/sentry/src/test/java/io/sentry/SentryOptionsTest.kt @@ -4,6 +4,8 @@ import com.google.common.truth.Truth.assertThat import io.sentry.SentryOptions.RequestSize import io.sentry.logger.ILoggerBatchProcessorFactory import io.sentry.test.getProperty +import io.sentry.time.MonotonicTicker +import io.sentry.time.TestMonotonicTicker import io.sentry.util.LazyEvaluator import io.sentry.util.StringUtils import java.io.File @@ -1337,9 +1339,16 @@ class SentryOptionsTest { private fun SentryOptions.peekHostnameCache(): HostnameCache? = getProperty>("hostnameCache").getProperty("value") + /** Options whose hostname cache resolves against a ticker the test controls. */ + private fun optionsWithTicker(ticker: MonotonicTicker): SentryOptions = + object : SentryOptions() { + override fun getMonotonicTicker(): MonotonicTicker = ticker + } + @Test fun `hostname is resolved on first use, not when options are constructed`() { - val options = SentryOptions() + val ticker = TestMonotonicTicker() + val options = optionsWithTicker(ticker) assertThat(options.peekHostnameCache()).isNull() @@ -1348,5 +1357,6 @@ class SentryOptionsTest { // Also keeps the assertion above honest: peek does report a cache once one exists. assertThat(options.peekHostnameCache()).isSameInstanceAs(cache) assertThat(options.hostnameCache).isSameInstanceAs(cache) + assertThat(cache.getProperty("ticker")).isSameInstanceAs(ticker) } }