diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a8eea2d9c..a1b63b2e28 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,10 @@ - Sentry can now configure Log4j2 automatically for Spring Boot 3 when `sentry-log4j2` is on the classpath and Log4j2 Core is the active logging backend ([#6072](https://github.com/getsentry/sentry-java/pull/6072)) - Disabled by default for now; enable it and configure levels the same way as described in the Spring Boot 4 entry above (`sentry.logging.enabled=true`) +### Fixes + +- Keep resolving the server name after `Sentry.close()` or a re-init. Closing the SDK shut down the shared hostname cache for the life of the process, so `server_name` silently froze at the value it had last resolved ([#6119](https://github.com/getsentry/sentry-java/pull/6119)) + ### Internal - 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)) diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 24635fc5dd..f28fffd6b8 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -1392,9 +1392,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; diff --git a/sentry/src/main/java/io/sentry/HostnameCache.java b/sentry/src/main/java/io/sentry/HostnameCache.java index 56cc0c2e84..4e9e2bc4c0 100644 --- a/sentry/src/main/java/io/sentry/HostnameCache.java +++ b/sentry/src/main/java/io/sentry/HostnameCache.java @@ -8,6 +8,7 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Future; import java.util.concurrent.LinkedBlockingQueue; +import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.ThreadFactory; import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; @@ -91,7 +92,7 @@ private HostnameCache() { 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. + // alive between the infrequent cache refreshes and nothing has to shut it down. final @NotNull ThreadPoolExecutor executor = new ThreadPoolExecutor( 1, @@ -105,14 +106,6 @@ private HostnameCache() { updateCache(); } - void close() { - this.executorService.shutdown(); - } - - boolean isClosed() { - return this.executorService.isShutdown(); - } - /** * Gets the hostname of the current machine. * @@ -144,8 +137,21 @@ private void updateCache() { return null; }; + final Future futureTask; + try { + futureTask = executorService.submit(hostRetriever); + } catch (RejectedExecutionException e) { + // updateRunning is cleared by the callable's finally block, which never runs if the callable + // was never queued. Clearing it here keeps a failure to queue from latching the flag on and + // silencing every later refresh. + updateRunning.set(false); + handleCacheUpdateFailure(); + return; + } + + // A timeout or interrupt below leaves the callable running, so it still clears updateRunning + // itself; doing it here as well would let refreshes pile up behind a slow lookup. try { - final Future futureTask = executorService.submit(hostRetriever); futureTask.get(GET_HOSTNAME_TIMEOUT, TimeUnit.MILLISECONDS); } catch (InterruptedException e) { Thread.currentThread().interrupt(); diff --git a/sentry/src/main/java/io/sentry/MainEventProcessor.java b/sentry/src/main/java/io/sentry/MainEventProcessor.java index d84c9e47be..bde148134a 100644 --- a/sentry/src/main/java/io/sentry/MainEventProcessor.java +++ b/sentry/src/main/java/io/sentry/MainEventProcessor.java @@ -8,18 +8,15 @@ 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; @@ -271,27 +268,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/test/java/io/sentry/HostnameCacheTest.kt b/sentry/src/test/java/io/sentry/HostnameCacheTest.kt index 3cc3a52aa2..6d0398858a 100644 --- a/sentry/src/test/java/io/sentry/HostnameCacheTest.kt +++ b/sentry/src/test/java/io/sentry/HostnameCacheTest.kt @@ -2,9 +2,11 @@ package io.sentry import com.google.common.truth.Truth.assertThat import io.sentry.test.getProperty +import io.sentry.test.injectForField import java.net.InetAddress import java.util.concurrent.ThreadPoolExecutor import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicBoolean import kotlin.test.Test import org.mockito.kotlin.mock import org.mockito.kotlin.whenever @@ -23,6 +25,21 @@ class HostnameCacheTest { assertThat(cache.hostname).isEqualTo("myhost") } + @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. + cache.getProperty("executorService").shutdown() + cache.injectForField("expirationTimestamp", 0L) + + assertThat(cache.hostname).isEqualTo("myhost") + + // The callable never ran, so nothing else clears this flag; left set, it would fail the + // compareAndSet guard in getHostname() and no refresh would ever be attempted again. + assertThat(cache.getProperty("updateRunning").get()).isFalse() + } + @Test fun `worker thread times out while idle instead of staying alive`() { val cache = getSut() @@ -31,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 fe5c835c90..a35f0e0e02 100644 --- a/sentry/src/test/java/io/sentry/MainEventProcessorTest.kt +++ b/sentry/src/test/java/io/sentry/MainEventProcessorTest.kt @@ -571,16 +571,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