Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ANR in HostnameCache.updateCache while initialising SDK #2153

Closed
vibin opened this issue Jul 6, 2022 · 13 comments · Fixed by #2170
Closed

ANR in HostnameCache.updateCache while initialising SDK #2153

vibin opened this issue Jul 6, 2022 · 13 comments · Fixed by #2170
Assignees

Comments

@vibin
Copy link

vibin commented Jul 6, 2022

Integration

sentry-android

Build System

Gradle

AGP Version

7.0.4

Proguard

Enabled

Version

5.7.4

Steps to Reproduce

Initialise the SDK

Expected Result

SDK to initialise without causing any ANR.

Actual Result

ANR is thrown during initialisation.

ANR type: background ANR
OS: Android 10 and 11

main (timed waiting): tid=1 systid=6071 
       at sun.misc.Unsafe.park(Unsafe.java)
       at java.util.concurrent.locks.LockSupport.parkNanos(LockSupport.java:230)
       at java.util.concurrent.FutureTask.awaitDone(FutureTask.java:447)
       at java.util.concurrent.FutureTask.get(FutureTask.java:205)
       at io.sentry.HostnameCache.updateCache(HostnameCache.java:119)
       at io.sentry.HostnameCache.<init>(HostnameCache.java:75)
       at io.sentry.HostnameCache.<init>(HostnameCache.java:62)
       at io.sentry.HostnameCache.<init>(HostnameCache.java:58)
       at io.sentry.HostnameCache.getInstance(HostnameCache.java:52)
       at io.sentry.MainEventProcessor.<init>(MainEventProcessor.java:37)
       at io.sentry.SentryOptions.<init>(SentryOptions.java:1620)
       at io.sentry.SentryOptions.<init>(SentryOptions.java:1600)
       at io.sentry.android.core.SentryAndroidOptions.<init>(SentryAndroidOptions.java:90)
       at java.lang.reflect.Constructor.newInstance0(Constructor.java)
       at java.lang.reflect.Constructor.newInstance(Constructor.java:343)
       at io.sentry.OptionsContainer.createInstance(OptionsContainer.java:24)
       at io.sentry.Sentry.init(Sentry.java:115)
       at io.sentry.android.core.SentryAndroid.init(SentryAndroid.java:83)
       at io.sentry.android.core.SentryAndroid.init(SentryAndroid.java:64)
       at redacted.core.CrashReporter$Companion.initSentry(CrashReporter.kt:96)
       at redacted.core.CrashReporter$Companion.init(CrashReporter.kt:90)
       at redacted.main.initializer.CrashReporterInitializer.initialize(CrashReporterInitializer.kt:12)
       at redacted.main.initializer.AppInitializers.init(AppInitializers.kt:42)
       at redacted.main.RedactedApplication.onCreate(SupplyApplication.java:80)
       at android.app.Instrumentation.callApplicationOnCreate(Instrumentation.java:1211)
       at android.app.ActivityThread.handleBindApplication(ActivityThread.java:7506)
       at android.app.ActivityThread.access$1600(ActivityThread.java:310)
       at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2281)
       at android.os.Handler.dispatchMessage(Handler.java:106)
       at android.os.Looper.loopOnce(Looper.java:226)
       at android.os.Looper.loop(Looper.java:313)
       at android.app.ActivityThread.main(ActivityThread.java:8663)
       at java.lang.reflect.Method.invoke(Method.java)
       at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:567)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1135)

Sentry init configuration:

SentryAndroid.init(context) { options: SentryAndroidOptions ->
  options.apply {
      release = BuildConfig.VERSION_NAME
      dsn = BuildConfig.SENTRY_DSN
      isEnableNdk = false
      sampleRate = null // Send all errors. No sampling.
      tracesSampleRate = null // Disable tracing.
      tracesSampler = null
      isAnrEnabled = false
      isEnableAutoSessionTracking = true
      sessionTrackingIntervalMillis = redacted
      isSendDefaultPii = false
      addInAppInclude("redacted.android")
      addInAppInclude("redacted.analytics")
      isEnableAppComponentBreadcrumbs = true
      isEnableSystemEventBreadcrumbs = true
      isEnableAppLifecycleBreadcrumbs = true
      isEnableActivityLifecycleBreadcrumbs = false
      addIntegration(SentryActivityFragmentLifecycleIntegration(context)) // Custom integration.
      isEnableAutoActivityLifecycleTracing = false
      isEnableActivityLifecycleTracingAutoFinish = false
      isEnableUserInteractionBreadcrumbs = false
      beforeSend = BeforeSendCallback { event: SentryEvent, hint: Any? ->
          return@BeforeSendCallback if (event.isCrashed) {
              event
          } else {
              null // Swallow non-fatal errors.
          }
      }
  }
}
@philipphofmann
Copy link
Member

@vibin, is this happening every time or only under certain conditions?

@vibin
Copy link
Author

vibin commented Jul 7, 2022

@philipphofmann According to Crashlytics, this is happening for 0.035% of our DAU. But, Crashlytics only tracks ANRs in Android 10 and 11.

I'm not able to reproduce this issue, so not sure of the conditions (but it's only happening in background, in production)

@philipphofmann
Copy link
Member

philipphofmann commented Jul 7, 2022

The problem seems to be here

final Future<Void> futureTask = executorService.submit(hostRetriever);
futureTask.get(GET_HOSTNAME_TIMEOUT, TimeUnit.MILLISECONDS);

I'm not sure what is causing this. Thanks for reporting this, we need to look into this. I can't give you an ETA at the moment.

@philipphofmann philipphofmann added Type: Bug Something isn't working and removed Status: Untriaged labels Jul 7, 2022
@romtsn
Copy link
Member

romtsn commented Jul 8, 2022

future.get() is indeed a blocking call - but do we even need it @marandaneto? It's gonna always return localhost for an android device anyway (at least from my tests), so perhaps we can just have const here or drop it altogether for Android?

@marandaneto
Copy link
Contributor

marandaneto commented Jul 8, 2022

This is only read if attachServerName is enabled, Android sets setAttachServerName(false); by default, are you sure its coming from there?
In case the thread is reading and updating the cache on Android, it should not.

@romtsn
Copy link
Member

romtsn commented Jul 8, 2022

hm, it was definitely hitting the debug breakpoint for me, will re-check

@romtsn
Copy link
Member

romtsn commented Jul 12, 2022

So it's actually being called, because SentryAndroidOptions ctor calls to SentryAndroid ctor before we setAttachServerName(false) and then it calls all the way to HostnameCache through the MainEventProcessor:

eventProcessors.add(new MainEventProcessor(this));

this(options, options.isAttachServerName() ? HostnameCache.getInstance() : null);

what if we lazily initialize HostnameCache when it's actually needed in setServerName here @adinauer ?

if (options.isAttachServerName() && hostnameCache != null && event.getServerName() == null) {

@adinauer
Copy link
Member

@romtsn we're also closing the hostnameCache and checking if it's closed in MainEventProcessor so we'd have to keep the hostnameCache field, remove it from ctor and assign it lazily when we need it. If that's what you're suggesting, sounds good to me.

@romtsn
Copy link
Member

romtsn commented Jul 12, 2022

yeah, we should keep the field 👍

@vibin
Copy link
Author

vibin commented Jul 14, 2022

Thanks!

@nienienienie
Copy link

@vibin is initializing sentry on main thread, isn't that the root cause of the ANR?

@vibin
Copy link
Author

vibin commented Aug 3, 2022

@vibin is initializing sentry on main thread, isn't that the root cause of the ANR?

yes, we're. But initialising sentry on main thread is a valid usecase. The root cause seems to be HostNameCache

@nienienienie
Copy link

no really, initializing anything that can take some time, especially from external library which can change without notice, is not valid

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants