diff --git a/CHANGELOG.md b/CHANGELOG.md index 6accb560053..097921188d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Performance + +- Reduce the number of SDK threads: the `HostnameCache` worker thread now times out while idle instead of staying alive for the whole process lifetime ([#5817](https://github.com/getsentry/sentry-java/pull/5817)) + ## 8.50.0 ### Android 17 support diff --git a/sentry/src/main/java/io/sentry/HostnameCache.java b/sentry/src/main/java/io/sentry/HostnameCache.java index beeb6f2d1b9..56cc0c2e845 100644 --- a/sentry/src/main/java/io/sentry/HostnameCache.java +++ b/sentry/src/main/java/io/sentry/HostnameCache.java @@ -6,9 +6,10 @@ import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; import java.util.concurrent.Future; +import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.ThreadFactory; +import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; @@ -34,6 +35,9 @@ public final class HostnameCache { /** Time before the get hostname operation times out (in ms). */ private static final long GET_HOSTNAME_TIMEOUT = TimeUnit.SECONDS.toMillis(1); + /** 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(); @@ -52,8 +56,7 @@ public final class HostnameCache { private final @NotNull Callable getLocalhost; - private final @NotNull ExecutorService executorService = - Executors.newSingleThreadExecutor(new HostnameCacheThreadFactory()); + private final @NotNull ExecutorService executorService; public static @NotNull HostnameCache getInstance() { if (INSTANCE == null) { @@ -87,6 +90,18 @@ private HostnameCache() { HostnameCache(long cacheDuration, final @NotNull Callable getLocalhost) { this.cacheDuration = cacheDuration; this.getLocalhost = Objects.requireNonNull(getLocalhost, "getLocalhost is required"); + // A single thread executor whose worker thread times out while idle, so no thread is kept + // alive between the infrequent cache refreshes. + final @NotNull ThreadPoolExecutor executor = + new ThreadPoolExecutor( + 1, + 1, + THREAD_KEEP_ALIVE_SECONDS, + TimeUnit.SECONDS, + new LinkedBlockingQueue<>(), + new HostnameCacheThreadFactory()); + executor.allowCoreThreadTimeOut(true); + this.executorService = executor; updateCache(); } diff --git a/sentry/src/test/java/io/sentry/HostnameCacheTest.kt b/sentry/src/test/java/io/sentry/HostnameCacheTest.kt new file mode 100644 index 00000000000..3cc3a52aa26 --- /dev/null +++ b/sentry/src/test/java/io/sentry/HostnameCacheTest.kt @@ -0,0 +1,41 @@ +package io.sentry + +import com.google.common.truth.Truth.assertThat +import io.sentry.test.getProperty +import java.net.InetAddress +import java.util.concurrent.ThreadPoolExecutor +import java.util.concurrent.TimeUnit +import kotlin.test.Test +import org.mockito.kotlin.mock +import org.mockito.kotlin.whenever + +class HostnameCacheTest { + + private fun getSut(): HostnameCache { + val address = mock() + whenever(address.canonicalHostName).thenReturn("myhost") + return HostnameCache(TimeUnit.HOURS.toMillis(1)) { address } + } + + @Test + fun `hostname is resolved and cached`() { + val cache = getSut() + assertThat(cache.hostname).isEqualTo("myhost") + } + + @Test + fun `worker thread times out while idle instead of staying alive`() { + val cache = getSut() + val executorService = cache.getProperty("executorService") + assertThat(executorService.allowsCoreThreadTimeOut()).isTrue() + 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() + } +}