-
-
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
Don't wait on main thread when SDK restarts #3200
Conversation
- IHub.close(Boolean) - ISentryClient.close(Boolean) - ITransport.close(Boolean) when the SDK restarts, the executor service is now closed in the background, and current network connections are dropped
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
93a76ca | 377.96 ms | 447.52 ms | 69.56 ms |
baaf637 | 462.32 ms | 579.22 ms | 116.90 ms |
2465853 | 411.39 ms | 461.10 ms | 49.72 ms |
c3f503e | 360.41 ms | 434.67 ms | 74.27 ms |
93a76ca | 381.08 ms | 459.22 ms | 78.14 ms |
c7e2fbc | 393.98 ms | 478.24 ms | 84.27 ms |
2465853 | 422.61 ms | 491.20 ms | 68.58 ms |
5e04ee8 | 302.68 ms | 343.36 ms | 40.68 ms |
283d83e | 416.81 ms | 497.22 ms | 80.41 ms |
d6d2b2e | 413.20 ms | 486.76 ms | 73.56 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
93a76ca | 1.72 MiB | 2.29 MiB | 576.75 KiB |
baaf637 | 1.72 MiB | 2.27 MiB | 558.42 KiB |
2465853 | 1.70 MiB | 2.27 MiB | 583.82 KiB |
c3f503e | 1.72 MiB | 2.29 MiB | 577.04 KiB |
93a76ca | 1.72 MiB | 2.29 MiB | 576.75 KiB |
c7e2fbc | 1.72 MiB | 2.29 MiB | 576.40 KiB |
2465853 | 1.70 MiB | 2.27 MiB | 583.82 KiB |
5e04ee8 | 1.70 MiB | 2.27 MiB | 584.64 KiB |
283d83e | 1.72 MiB | 2.29 MiB | 577.69 KiB |
d6d2b2e | 1.72 MiB | 2.27 MiB | 555.05 KiB |
Previous results on branch: enh/dont-wait-restart
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
588e62b | 318.55 ms | 360.32 ms | 41.77 ms |
0807309 | 368.15 ms | 456.16 ms | 88.02 ms |
866a6af | 614.82 ms | 743.69 ms | 128.87 ms |
0d43908 | 410.02 ms | 483.90 ms | 73.88 ms |
0e03bb0 | 367.81 ms | 440.85 ms | 73.05 ms |
4dc1651 | 376.40 ms | 456.41 ms | 80.01 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
588e62b | 1.70 MiB | 2.27 MiB | 584.99 KiB |
0807309 | 1.70 MiB | 2.27 MiB | 585.11 KiB |
866a6af | 1.70 MiB | 2.27 MiB | 585.11 KiB |
0d43908 | 1.70 MiB | 2.27 MiB | 585.00 KiB |
0e03bb0 | 1.70 MiB | 2.27 MiB | 584.98 KiB |
4dc1651 | 1.70 MiB | 2.27 MiB | 585.11 KiB |
I added a small timeout to |
…ectedExecutionHandler, to save the envelope to disk
Ok, I removed that small timeout. Now, when |
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.
Looking good, I left 2 comments for clarification!
...on-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/SdkInitTests.kt
Outdated
Show resolved
Hide resolved
sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java
Outdated
Show resolved
Hide resolved
…runs it in the rejectedExecutionHandler on close
…runs it in the rejectedExecutionHandler on close
} | ||
|
||
@Test | ||
fun `close shuts down the executor and runs executing runnable through rejectedExecutionHandler`() { |
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.
nice 👍
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!
📜 Description
added new methods to close instances with isRestarting flag:
when the SDK restarts
We still wait for any processing that is happening, as that's more complex, but the biggest slowdown when restarting the SDK is waiting for the network to send data.
💡 Motivation and Context
When the SDK is initialized multiple times, it closes the old instance, waiting for any task to be completed.
This PR aims to reduce the wait time by dropping network connections and let any job scheduled to the executor to be finished in the background.
Relates to #3162
💚 How did you test it?
Unit tests
Added Ui test to check it waits for network to timeout on Sentry.close and doesn't wait on multiple Sentry.init()
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps