diff --git a/CHANGELOG.md b/CHANGELOG.md index f9f5493972..0276b67e25 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,10 @@ - Keep the Android feedback form available when its options configurator throws ([#6239](https://github.com/getsentry/sentry-java/pull/6239)). - Isolate nonfatal callback throwables while continuing to propagate fatal JVM errors ([#6251](https://github.com/getsentry/sentry-java/pull/6251)). +### Internal + +- Hold the hostname cache on `SentryOptions` instead of a static singleton ([#6117](https://github.com/getsentry/sentry-java/pull/6117)) + ### Dependencies - Bump Native SDK from v0.17.2 to v0.17.3 ([#6250](https://github.com/getsentry/sentry-java/pull/6250)) diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 400b602aa1..813a4691e8 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -694,7 +694,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/HttpBodyType : java/lang/Enum { @@ -3808,6 +3807,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 4e9e2bc4c0..b4a19124b8 100644 --- a/sentry/src/main/java/io/sentry/HostnameCache.java +++ b/sentry/src/main/java/io/sentry/HostnameCache.java @@ -1,6 +1,5 @@ package io.sentry; -import io.sentry.util.AutoClosableReentrantLock; import io.sentry.util.Objects; import java.net.InetAddress; import java.util.concurrent.Callable; @@ -26,8 +25,8 @@ * 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 { @@ -39,10 +38,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(); - /** Time for which the cache is kept. */ private final long cacheDuration; @@ -59,19 +54,13 @@ 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() { + /** + * Do not construct a hostname cache directly; get it from {@link + * 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. + */ + HostnameCache() { this(HOSTNAME_CACHE_DURATION); } diff --git a/sentry/src/main/java/io/sentry/MainEventProcessor.java b/sentry/src/main/java/io/sentry/MainEventProcessor.java index 25ec4b6966..7e1edfb071 100644 --- a/sentry/src/main/java/io/sentry/MainEventProcessor.java +++ b/sentry/src/main/java/io/sentry/MainEventProcessor.java @@ -22,7 +22,6 @@ public final class MainEventProcessor implements SentryEventProcessor { 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; @@ -161,16 +160,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()); } } diff --git a/sentry/src/main/java/io/sentry/SentryOptions.java b/sentry/src/main/java/io/sentry/SentryOptions.java index 2b2f9d370a..f9b9fb13c9 100644 --- a/sentry/src/main/java/io/sentry/SentryOptions.java +++ b/sentry/src/main/java/io/sentry/SentryOptions.java @@ -535,6 +535,16 @@ 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. + */ + @ApiStatus.Internal + private final @NotNull LazyEvaluator hostnameCache = + new LazyEvaluator<>(() -> new HostnameCache()); + private final @NotNull List performanceCollectors = new ArrayList<>(); /** Performance collector that collect performance stats while transactions run. */ @@ -3140,6 +3150,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 3741ddc620..139ac61539 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; @@ -256,7 +255,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 d70b4ab5c9..93564e4dda 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; @@ -241,7 +240,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/MainEventProcessorTest.kt b/sentry/src/test/java/io/sentry/MainEventProcessorTest.kt index ed6efffb07..cded7e8775 100644 --- a/sentry/src/test/java/io/sentry/MainEventProcessorTest.kt +++ b/sentry/src/test/java/io/sentry/MainEventProcessorTest.kt @@ -10,7 +10,6 @@ import io.sentry.protocol.User import io.sentry.util.HintUtils import java.lang.RuntimeException import java.net.InetAddress -import kotlin.test.AfterTest import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFalse @@ -19,7 +18,6 @@ import kotlin.test.assertNull import kotlin.test.assertSame import kotlin.test.assertTrue import org.awaitility.kotlin.await -import org.mockito.Mockito import org.mockito.kotlin.mock import org.mockito.kotlin.reset import org.mockito.kotlin.times @@ -28,17 +26,23 @@ 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() + // 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(hostnameCacheDuration) { getLocalhost } - hostnameCacheMock.`when` { HostnameCache.getInstance() }.thenReturn(hostnameCache) + hostnameCache = HostnameCache(hostnameCacheDuration) { getLocalhost } return MainEventProcessor(sentryOptions) } - - fun teardown() { - hostnameCacheMock.close() - } - } - - @AfterTest - fun teardown() { - fixture.teardown() } private val fixture = Fixture() diff --git a/sentry/src/test/java/io/sentry/SentryOptionsTest.kt b/sentry/src/test/java/io/sentry/SentryOptionsTest.kt index 6b47d36e53..101a136f64 100644 --- a/sentry/src/test/java/io/sentry/SentryOptionsTest.kt +++ b/sentry/src/test/java/io/sentry/SentryOptionsTest.kt @@ -3,6 +3,8 @@ 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.util.LazyEvaluator import io.sentry.util.StringUtils import java.io.File import java.net.Proxy @@ -1331,4 +1333,20 @@ class SentryOptionsTest { options.scopesStorageFactory = null assertNull(options.scopesStorageFactory) } + + private fun SentryOptions.peekHostnameCache(): HostnameCache? = + getProperty>("hostnameCache").getProperty("value") + + @Test + fun `hostname is resolved on first use, not when options are constructed`() { + val options = SentryOptions() + + 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) + } }