-
Notifications
You must be signed in to change notification settings - Fork 123
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
Bug 1910449 - Use daemon-threads with the MPS timer instead of the default user-threads #2930
Conversation
This requires a bit of communication with product and DS folks before landing and release to ensure no one is surprised by any changes to the data, expected or unexpected. |
6608a21
to
7e74f7f
Compare
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.
As called out in the bug what about this trivial patch:
diff --git glean-core/android/src/main/java/mozilla/telemetry/glean/scheduler/MetricsPingScheduler.kt glean-core/android/src/main/java/mozilla/telemetry/glean/scheduler/MetricsPingScheduler.kt
index 6387ceff5..0ed70bed8 100644
--- glean-core/android/src/main/java/mozilla/telemetry/glean/scheduler/MetricsPingScheduler.kt
+++ glean-core/android/src/main/java/mozilla/telemetry/glean/scheduler/MetricsPingScheduler.kt
@@ -107,7 +107,9 @@ internal class MetricsPingScheduler(
// currently-running task.
cancel()
- timer = Timer("glean.MetricsPingScheduler")
+ if (timer == null) {
+ timer = Timer("glean.MetricsPingScheduler", true)
+ }
timer?.schedule(MetricsPingTimer(this, reason), millisUntilNextDueTime)
}
Not sure how you tested this change, but if you have a strategy for that maybe with the above daemon thread we can test again?
glean-core/android/src/main/java/mozilla/telemetry/glean/scheduler/MetricsPingScheduler.kt
Outdated
Show resolved
Hide resolved
Well... it involved an actual device and about a week of letting it sit for about 2 days at a time waiting for things to happen naturally 😅! It was very manual and time consuming due to the 1/day scheduling, but it seemed to behave as I intended. I'm not even sure all devices and Android versions behave the same with regards to apps running in the background, making this even more "fun" to tinker with. I'm still set up to test this so I'll try the daemon thread approach as an option to this, since it seems like it would be an even cleaner approach than relying on the lifecycle detection & cancellation. |
Turns out the timer is always going to be |
4af2424
to
e300026
Compare
Use a daemon-thread instead of the default user-thread in the `java.util.Timer` used to schedule pings within the MetricsPingScheduler. User-threads can prevent the application from terminating gracefully while daemon-threads do not.
Changing the MPS
java.util.Timer
to use daemon-threads instead of the default user-threads because user-threads can potentially prevent the application from terminating properly since the OS requires all user-threads to be joined before the app can terminate. Daemon-threads are terminated by the OS once all user-threads have completed and are not awaited before the application can terminate.