ref(core): Hold the hostname cache on SentryOptions (JAVA-579) - #6117
Draft
runningcode wants to merge 4 commits into
Draft
runningcode wants to merge 4 commits into
runningcode wants to merge 4 commits into
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
HostnameCache was a process-wide static singleton whose constructor reached for JavaMonotonicTicker.getInstance() directly. That was the last place in the SDK hard-coding a ticker rather than taking one from options, so the cache measured its TTL on System.nanoTime() even where options supply an elapsedRealtimeNanos()-backed ticker that counts deep sleep. SentryOptions now holds one instance, built from getMonotonicTicker() the same way RateLimiter(SentryOptions) is. It is wrapped in a LazyEvaluator because resolving the hostname blocks on InetAddress.getLocalHost(), which no Sentry.init should pay for up front; deferring also means the SentryAndroidOptions override is in effect by the time the ticker is read. MainEventProcessor still closes the cache, and now drops both its own reference and the options-held one. Both it and the options outlive Scopes.close(isRestarting = true), so a closed cache left in either place would never refresh the hostname again. MainEventProcessorTest no longer needs Mockito.mockStatic to intercept getInstance(); it sets the cache on options instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Sorry, something went wrong.
Copy link
Copy Markdown
📲 Install BuildsAndroid
|
Sorry, something went wrong.
HostnameCache reads exactly one collaborator, so it now takes a MonotonicTicker rather than a SentryOptions it would only call getMonotonicTicker() on. This follows RateLimiter.create(MonotonicTicker, RateLimiterConfig), which names the collaborators a rate limiter actually reads for the same reason; a config interface is unnecessary here because there is only the one. Also drops SentryOptions.setHostnameCache(), which had no production caller. Unlike setDateProvider(), which AndroidOptionsInitializer uses, it was only a test seam, so MainEventProcessorTest overrides getHostnameCache() instead — the way CheckInUtilsTest already overrides getMonotonicTicker(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The cache's executor is a single daemon thread with allowCoreThreadTimeOut(true) and a 30 second keep-alive, so it self-terminates once idle and never holds up process exit. close() was switching off something that switches itself off. Scopes.close() already leaves the timer executor running for this reason. Dropping it removes the reason SentryOptions.resetHostnameCache() existed. That method was only there because MainEventProcessor closed a cache the options own, which a re-init with the same options object would then keep handing out shut down. Nothing closes the cache now, so nothing has to undo it. MainEventProcessor no longer implements Closeable, and no longer memoizes the cache either: the options build it on first use, so the field and ensureHostnameCache() were duplicating LazyEvaluator. Both removed methods are on @ApiStatus.Internal types. SentryClientTest's `when client is closed, hostname cache is closed` went with them; it asserted isClosed() on a processor that had never resolved a hostname, which returned true because the cache was still null. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📜 Description
HostnameCache was a process-wide static singleton (HostnameCache.getInstance()) whose private constructor reached for JavaMonotonicTicker.getInstance() directly.
SentryOptions now holds one instance, built from getMonotonicTicker() the same way RateLimiter(SentryOptions) is. It is wrapped in a LazyEvaluator because resolving the hostname blocks on InetAddress.getLocalHost(), which no Sentry.init should pay for up front — deferring also means the SentryAndroidOptions ticker override is in effect by the time the ticker is read.
MainEventProcessor still closes the cache, and now drops both its own reference and the options-held one. Both the processor and the options survive Scopes.close(isRestarting = true), so a closed cache left in either place would never refresh the hostname again.
MetricsApi and LoggerApi read the cache off the options they already hold.
Stacked on #6100, which this depends on for the HostnameCache(Callable, MonotonicTicker) constructor. It targets that branch so the diff here is just this change; GitHub will retarget to main once #6100 merges.
💡 Motivation and Context
Hard-coding JavaMonotonicTicker meant the hostname cache measured its 5-hour TTL on System.nanoTime() even where options supply an elapsedRealtimeNanos()-backed ticker, so it ignored time the device spent in deep sleep. This was the last place in the SDK hard-coding a ticker instead of taking one from options. Android sets attachServerName = false by default, so the wrong-clock behavior was latent rather than actively broken.
Removing the singleton also let MainEventProcessorTest drop a Mockito.mockStatic(HostnameCache::class.java) hack in favor of a plain setter.
💚 How did you test it?
:sentry:test and :sentry:apiCheck. New SentryOptionsTest cases cover the lazy construction (asserting that constructing options does not resolve the hostname), instance reuse, resetHostnameCache(), and that the cache takes its ticker from getMonotonicTicker().
📝 Checklist
getHostnameCache(), setHostnameCache() and resetHostnameCache() are @ApiStatus.Internal, and the removed HostnameCache.getInstance() was on an @ApiStatus.Internal class — so sentry.api changes without a public contract change.
🔮 Next steps
None.
🤖 Generated with Claude Code