ref(core): Hold the hostname cache on SentryOptions (JAVA-579)

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
 

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 <init> (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;

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.
  *
- * <p>HostnameCache is a singleton and its instance should be obtained through {@link
- * HostnameCache#getInstance()}.
+ * <p>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.
    *

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;

sentry/src/main/java/io/sentry/SentryOptions.java

@@ -530,6 +530,17 @@ public class SentryOptions implements RateLimiterConfig {
   private final @NotNull LazyEvaluator<SentryDateProvider> dateProvider =
       new LazyEvaluator<>(() -> new SentryAutoDateProvider());
 
+  /**
+   * Cache of the local hostname, used when {@link #isAttachServerName()} is enabled.
+   *
+   * <p>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> hostnameCache =
+      new LazyEvaluator<>(() -> new HostnameCache(getMonotonicTicker()));
+
   private final @NotNull List<IPerformanceCollector> 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.
    *

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",

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",

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()
-  }
 }

sentry/src/test/java/io/sentry/MainEventProcessorTest.kt

@@ -11,15 +11,13 @@ 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
 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<IScopes>()
     val getLocalhost = mock<InetAddress>()
     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`<Any> { 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"))

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<MainEventProcessor>().first()
-    assertTrue(mainEventProcessor.isClosed)
-  }
-
   @Test
   fun `when beforeSend is set, callback is invoked`() {
     var invoked = false