-
-
Notifications
You must be signed in to change notification settings - Fork 435
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
Reduce timeout of AsyncHttpTransport to avoid ANR #2879
Conversation
…fault 4s in Android and 15s in Java) to avoid ANRs in Android
|
Performance metrics 🚀
|
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## feat/7.0.0 #2879 +/- ##
================================================
+ Coverage 81.26% 81.44% +0.17%
- Complexity 4560 4648 +88
================================================
Files 350 354 +4
Lines 16866 17108 +242
Branches 2272 2313 +41
================================================
+ Hits 13706 13933 +227
- Misses 2219 2226 +7
- Partials 941 949 +8
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -140,7 +140,7 @@ public void close() throws IOException { | |||
executor.shutdown(); | |||
options.getLogger().log(SentryLevel.DEBUG, "Shutting down"); | |||
try { | |||
if (!executor.awaitTermination(1, TimeUnit.MINUTES)) { | |||
if (!executor.awaitTermination(options.getFlushTimeoutMillis(), TimeUnit.MILLISECONDS)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adinauer this change will also affect backend, hope that's fine
📜 Description
reduced timeout of AsyncHttpTransport close to flushTimeoutMillis (default 4s in Android and 15s in Java) to avoid ANRs in Android
💡 Motivation and Context
Closing the hub could cause an ANR, since the SDK would wait for 1 minute to send any cached envelope
Fixes #2864
💚 How did you test it?
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps