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
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>
📲 Install BuildsAndroid
|
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 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
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
HostnameCachewas a process-wide static singleton (HostnameCache.getInstance()) whose private constructor reached forJavaMonotonicTicker.getInstance()directly.SentryOptionsnow holds one instance, built fromgetMonotonicTicker()the same wayRateLimiter(SentryOptions)is. It is wrapped in aLazyEvaluatorbecause resolving the hostname blocks onInetAddress.getLocalHost(), which noSentry.initshould pay for up front — deferring also means theSentryAndroidOptionsticker override is in effect by the time the ticker is read.MainEventProcessorstill closes the cache, and now drops both its own reference and the options-held one. Both the processor and the options surviveScopes.close(isRestarting = true), so a closed cache left in either place would never refresh the hostname again.MetricsApiandLoggerApiread the cache off theoptionsthey 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 tomainonce #6100 merges.💡 Motivation and Context
Hard-coding
JavaMonotonicTickermeant the hostname cache measured its 5-hour TTL onSystem.nanoTime()even where options supply anelapsedRealtimeNanos()-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 setsattachServerName = falseby default, so the wrong-clock behavior was latent rather than actively broken.Removing the singleton also let
MainEventProcessorTestdrop aMockito.mockStatic(HostnameCache::class.java)hack in favor of a plain setter.💚 How did you test it?
:sentry:testand:sentry:apiCheck. NewSentryOptionsTestcases cover the lazy construction (asserting that constructing options does not resolve the hostname), instance reuse,resetHostnameCache(), and that the cache takes its ticker fromgetMonotonicTicker().📝 Checklist
sendDefaultPIIis enabled.getHostnameCache(),setHostnameCache()andresetHostnameCache()are@ApiStatus.Internal, and the removedHostnameCache.getInstance()was on an@ApiStatus.Internalclass — sosentry.apichanges without a public contract change.🔮 Next steps
None.
🤖 Generated with Claude Code