diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e0d8bd82d..3b62d13f3c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,7 @@ - Deprecate `AndroidCurrentDateProvider.getInstance()` in favor of `MonotonicTicker`, which counts time spent in deep sleep and cannot be confused with the epoch-based `CurrentDateProvider` ([#6103](https://github.com/getsentry/sentry-java/pull/6103)) - Measure the hostname cache TTL on a monotonic ticker, so that a device time change no longer shortens or extends it ([#6100](https://github.com/getsentry/sentry-java/pull/6100)) +- Hold the hostname cache on `SentryOptions` instead of a static singleton, so that it measures its TTL on the ticker the options provide ([#6117](https://github.com/getsentry/sentry-java/pull/6117)) ## 8.56.0 diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 24635fc5dd..d070668f03 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -633,7 +633,6 @@ public final class io/sentry/Hint { public final class io/sentry/HostnameCache { public fun getHostname ()Ljava/lang/String; - public static fun getInstance ()Lio/sentry/HostnameCache; } public final class io/sentry/HttpStatusCodeRange { @@ -1392,9 +1391,8 @@ public abstract interface class io/sentry/JsonUnknown { public abstract fun setUnknown (Ljava/util/Map;)V } -public final class io/sentry/MainEventProcessor : io/sentry/EventProcessor, java/io/Closeable { +public final class io/sentry/MainEventProcessor : io/sentry/EventProcessor { public fun (Lio/sentry/SentryOptions;)V - public fun close ()V public fun getOrder ()Ljava/lang/Long; public fun process (Lio/sentry/SentryEvent;Lio/sentry/Hint;)Lio/sentry/SentryEvent; public fun process (Lio/sentry/SentryLogEvent;)Lio/sentry/SentryLogEvent; @@ -3716,6 +3714,7 @@ public class io/sentry/SentryOptions : io/sentry/transport/RateLimiterConfig { public fun getFlushTimeoutMillis ()J public fun getFullyDisplayedReporter ()Lio/sentry/FullyDisplayedReporter; public fun getGestureTargetLocators ()Ljava/util/List; + public fun getHostnameCache ()Lio/sentry/HostnameCache; public fun getIdleTimeout ()Ljava/lang/Long; public fun getIgnoredCheckIns ()Ljava/util/List; public fun getIgnoredErrors ()Ljava/util/List; diff --git a/sentry/src/main/java/io/sentry/HostnameCache.java b/sentry/src/main/java/io/sentry/HostnameCache.java index a5ae15258c..e32b319930 100644 --- a/sentry/src/main/java/io/sentry/HostnameCache.java +++ b/sentry/src/main/java/io/sentry/HostnameCache.java @@ -1,9 +1,7 @@ package io.sentry; import io.sentry.time.Deadline; -import io.sentry.time.JavaMonotonicTicker; import io.sentry.time.MonotonicTicker; -import io.sentry.util.AutoClosableReentrantLock; import io.sentry.util.Objects; import java.net.InetAddress; import java.util.concurrent.Callable; @@ -28,8 +26,8 @@ * performance purposes, the operation of retrieving the hostname will automatically fail after a * period of time defined by {@link #GET_HOSTNAME_TIMEOUT} without result. * - *

HostnameCache is a singleton and its instance should be obtained through {@link - * HostnameCache#getInstance()}. + *

One instance is held per {@link SentryOptions} and should be obtained through {@link + * SentryOptions#getHostnameCache()}. */ @ApiStatus.Internal public final class HostnameCache { @@ -41,10 +39,6 @@ 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; - private static volatile @Nullable HostnameCache INSTANCE; - private static final @NotNull AutoClosableReentrantLock staticLock = - new AutoClosableReentrantLock(); - private final @NotNull MonotonicTicker ticker; /** Current value for hostname (might change over time). */ @@ -60,22 +54,16 @@ public final class HostnameCache { private final @NotNull ExecutorService executorService; - public static @NotNull HostnameCache getInstance() { - if (INSTANCE == null) { - try (final @NotNull ISentryLifecycleToken ignored = staticLock.acquire()) { - if (INSTANCE == null) { - INSTANCE = new HostnameCache(); - } - } - } - - return INSTANCE; - } - - private HostnameCache() { + /** + * Names the only collaborator a hostname cache reads, rather than taking the whole {@link + * SentryOptions}. + * + * @param ticker the ticker the cache lifetime is measured on + */ + HostnameCache(final @NotNull MonotonicTicker ticker) { // avoid method refs on Android due to some issues with older AGP setups // noinspection Convert2MethodRef - this(() -> InetAddress.getLocalHost(), JavaMonotonicTicker.getInstance()); + this(() -> InetAddress.getLocalHost(), ticker); } /** @@ -93,7 +81,7 @@ private HostnameCache() { // 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. + // alive between the infrequent cache refreshes and nothing has to shut it down. final @NotNull ThreadPoolExecutor executor = new ThreadPoolExecutor( 1, @@ -107,14 +95,6 @@ private HostnameCache() { updateCache(); } - void close() { - this.executorService.shutdown(); - } - - boolean isClosed() { - return this.executorService.isShutdown(); - } - /** * Gets the hostname of the current machine. * diff --git a/sentry/src/main/java/io/sentry/MainEventProcessor.java b/sentry/src/main/java/io/sentry/MainEventProcessor.java index d84c9e47be..57a3a2e1c9 100644 --- a/sentry/src/main/java/io/sentry/MainEventProcessor.java +++ b/sentry/src/main/java/io/sentry/MainEventProcessor.java @@ -8,23 +8,19 @@ import io.sentry.protocol.SentryTransaction; import io.sentry.protocol.User; import io.sentry.util.HintUtils; -import java.io.Closeable; -import java.io.IOException; import java.util.ArrayList; import java.util.List; import java.util.Map; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; -import org.jetbrains.annotations.VisibleForTesting; @ApiStatus.Internal -public final class MainEventProcessor implements EventProcessor, Closeable { +public final class MainEventProcessor implements EventProcessor { private final @NotNull SentryOptions options; private final @NotNull SentryThreadFactory sentryThreadFactory; private final @NotNull SentryExceptionFactory sentryExceptionFactory; - private volatile @Nullable HostnameCache hostnameCache = null; public MainEventProcessor(final @NotNull SentryOptions options) { this.options = options; @@ -163,16 +159,7 @@ private void setServerName(final @NotNull SentryBaseEvent event) { } if (options.isAttachServerName() && event.getServerName() == null) { - ensureHostnameCache(); - if (hostnameCache != null) { - event.setServerName(hostnameCache.getHostname()); - } - } - } - - private void ensureHostnameCache() { - if (hostnameCache == null) { - hostnameCache = HostnameCache.getInstance(); + event.setServerName(options.getHostnameCache().getHostname()); } } @@ -271,27 +258,6 @@ private boolean isCachedHint(final @NotNull Hint hint) { return HintUtils.hasType(hint, Cached.class); } - @Override - public void close() throws IOException { - if (hostnameCache != null) { - hostnameCache.close(); - } - } - - boolean isClosed() { - if (hostnameCache != null) { - return hostnameCache.isClosed(); - } else { - return true; - } - } - - @VisibleForTesting - @Nullable - HostnameCache getHostnameCache() { - return hostnameCache; - } - @Override public @Nullable Long getOrder() { return 0L; diff --git a/sentry/src/main/java/io/sentry/SentryOptions.java b/sentry/src/main/java/io/sentry/SentryOptions.java index 280bc01174..ebdb0b9011 100644 --- a/sentry/src/main/java/io/sentry/SentryOptions.java +++ b/sentry/src/main/java/io/sentry/SentryOptions.java @@ -530,6 +530,17 @@ public class SentryOptions implements RateLimiterConfig { private final @NotNull LazyEvaluator dateProvider = new LazyEvaluator<>(() -> new SentryAutoDateProvider()); + /** + * Cache of the local hostname, used when {@link #isAttachServerName()} is enabled. + * + *

Evaluated lazily because resolving the hostname blocks on {@code + * InetAddress.getLocalHost()}, which no {@code Sentry.init} should pay for up front. Deferring + * also means {@link #getMonotonicTicker()} is read after subclasses have overridden it. + */ + @ApiStatus.Internal + private final @NotNull LazyEvaluator hostnameCache = + new LazyEvaluator<>(() -> new HostnameCache(getMonotonicTicker())); + private final @NotNull List performanceCollectors = new ArrayList<>(); /** Performance collector that collect performance stats while transactions run. */ @@ -3092,6 +3103,12 @@ public void setDateProvider(final @NotNull SentryDateProvider dateProvider) { return JavaMonotonicTicker.getInstance(); } + /** Returns the hostname cache, resolving the hostname on first use. */ + @ApiStatus.Internal + public @NotNull HostnameCache getHostnameCache() { + return hostnameCache.getValue(); + } + /** * Adds a ICollector. * diff --git a/sentry/src/main/java/io/sentry/logger/LoggerApi.java b/sentry/src/main/java/io/sentry/logger/LoggerApi.java index c203dcbfb8..ccf69325d8 100644 --- a/sentry/src/main/java/io/sentry/logger/LoggerApi.java +++ b/sentry/src/main/java/io/sentry/logger/LoggerApi.java @@ -1,6 +1,5 @@ package io.sentry.logger; -import io.sentry.HostnameCache; import io.sentry.IScope; import io.sentry.ISpan; import io.sentry.PropagationContext; @@ -263,7 +262,7 @@ private void setServerName( "server.address", new SentryLogEventAttributeValue(SentryAttributeType.STRING, optionsServerName)); } else if (options.isAttachServerName()) { - final @Nullable String hostname = HostnameCache.getInstance().getHostname(); + final @Nullable String hostname = options.getHostnameCache().getHostname(); if (hostname != null) { attributes.put( "server.address", diff --git a/sentry/src/main/java/io/sentry/metrics/MetricsApi.java b/sentry/src/main/java/io/sentry/metrics/MetricsApi.java index cebcad9735..4cab828b7e 100644 --- a/sentry/src/main/java/io/sentry/metrics/MetricsApi.java +++ b/sentry/src/main/java/io/sentry/metrics/MetricsApi.java @@ -1,6 +1,5 @@ package io.sentry.metrics; -import io.sentry.HostnameCache; import io.sentry.IScope; import io.sentry.ISpan; import io.sentry.PropagationContext; @@ -250,7 +249,7 @@ private void setServerName( "server.address", new SentryLogEventAttributeValue(SentryAttributeType.STRING, optionsServerName)); } else if (options.isAttachServerName()) { - final @Nullable String hostname = HostnameCache.getInstance().getHostname(); + final @Nullable String hostname = options.getHostnameCache().getHostname(); if (hostname != null) { attributes.put( "server.address", diff --git a/sentry/src/test/java/io/sentry/HostnameCacheTest.kt b/sentry/src/test/java/io/sentry/HostnameCacheTest.kt index 93cb993696..6a87c32a0a 100644 --- a/sentry/src/test/java/io/sentry/HostnameCacheTest.kt +++ b/sentry/src/test/java/io/sentry/HostnameCacheTest.kt @@ -48,11 +48,4 @@ class HostnameCacheTest { assertThat(executorService.corePoolSize).isEqualTo(1) assertThat(executorService.maximumPoolSize).isEqualTo(1) } - - @Test - fun `close shuts the executor down`() { - val cache = getSut() - cache.close() - assertThat(cache.isClosed).isTrue() - } } diff --git a/sentry/src/test/java/io/sentry/MainEventProcessorTest.kt b/sentry/src/test/java/io/sentry/MainEventProcessorTest.kt index ee88f06ae2..9a02d9f423 100644 --- a/sentry/src/test/java/io/sentry/MainEventProcessorTest.kt +++ b/sentry/src/test/java/io/sentry/MainEventProcessorTest.kt @@ -11,7 +11,6 @@ import io.sentry.util.HintUtils import java.lang.RuntimeException import java.net.InetAddress import java.util.concurrent.TimeUnit -import kotlin.test.AfterTest import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFalse @@ -19,7 +18,6 @@ import kotlin.test.assertNotNull import kotlin.test.assertNull import kotlin.test.assertSame import kotlin.test.assertTrue -import org.mockito.Mockito import org.mockito.kotlin.mock import org.mockito.kotlin.reset import org.mockito.kotlin.times @@ -28,18 +26,24 @@ import org.mockito.kotlin.whenever class MainEventProcessorTest { class Fixture { - val sentryOptions: SentryOptions = - SentryOptions().apply { - dsn = dsnString - release = "release" - dist = "dist" - sdkVersion = SdkVersion("test", "1.2.3") - } 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 + val sentryOptions: SentryOptions = + object : SentryOptions() { + // Qualified: an unqualified name here would resolve to this override, not the field. + override fun getHostnameCache(): HostnameCache = this@Fixture.hostnameCache + } + .apply { + dsn = dsnString + release = "release" + dist = "dist" + sdkVersion = SdkVersion("test", "1.2.3") + } lateinit var sentryTracer: SentryTracer - private val hostnameCacheMock = Mockito.mockStatic(HostnameCache::class.java) fun getSut( attachThreads: Boolean = true, @@ -76,21 +80,10 @@ class MainEventProcessorTest { } whenever(scopes.options).thenReturn(sentryOptions) sentryTracer = SentryTracer(TransactionContext("", ""), scopes) - - val hostnameCache = HostnameCache({ getLocalhost }, hostnameCacheTicker) - hostnameCacheMock.`when` { HostnameCache.getInstance() }.thenReturn(hostnameCache) + hostnameCache = HostnameCache({ getLocalhost }, hostnameCacheTicker) return MainEventProcessor(sentryOptions) } - - fun teardown() { - hostnameCacheMock.close() - } - } - - @AfterTest - fun teardown() { - fixture.teardown() } private val fixture = Fixture() @@ -571,16 +564,6 @@ class MainEventProcessorTest { } } - @Test - fun `when processor is closed, closes hostname cache`() { - val sut = fixture.getSut(serverName = null) - - sut.process(SentryTransaction(fixture.sentryTracer), Hint()) - - sut.close() - assertNotNull(sut.hostnameCache) { assertTrue(it.isClosed) } - } - @Test fun `when event has modules, appends to them`() { val sut = fixture.getSut(modules = mapOf("group1:artifact1" to "2.0.0")) diff --git a/sentry/src/test/java/io/sentry/SentryClientTest.kt b/sentry/src/test/java/io/sentry/SentryClientTest.kt index 3066d2c1b5..61181ee96a 100644 --- a/sentry/src/test/java/io/sentry/SentryClientTest.kt +++ b/sentry/src/test/java/io/sentry/SentryClientTest.kt @@ -214,16 +214,6 @@ class SentryClientTest { assertFalse(sut.isEnabled) } - @Test - fun `when client is closed, hostname cache is closed`() { - val sut = fixture.getSut() - assertTrue(sut.isEnabled) - sut.close() - val mainEventProcessor = - fixture.sentryOptions.eventProcessors.filterIsInstance().first() - assertTrue(mainEventProcessor.isClosed) - } - @Test fun `when beforeSend is set, callback is invoked`() { var invoked = false diff --git a/sentry/src/test/java/io/sentry/SentryOptionsTest.kt b/sentry/src/test/java/io/sentry/SentryOptionsTest.kt index 64482b5d5d..f0bc051e98 100644 --- a/sentry/src/test/java/io/sentry/SentryOptionsTest.kt +++ b/sentry/src/test/java/io/sentry/SentryOptionsTest.kt @@ -1,7 +1,12 @@ package io.sentry +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 import java.net.Proxy @@ -1167,4 +1172,28 @@ class SentryOptionsTest { options.scopesStorageFactory = null assertNull(options.scopesStorageFactory) } + + 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 ticker = TestMonotonicTicker() + val options = optionsWithTicker(ticker) + + assertThat(options.peekHostnameCache()).isNull() + + val cache = options.hostnameCache + + // 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) + } }