Repository navigation
Conversation
…read A slow PowerManager Binder call during background-service teardown was ANRing the main thread after high-priority messages.
There was a problem hiding this comment.
Code Review
This pull request updates the Android JobIntentService implementation to release wake locks asynchronously on a background executor, preventing potential Application Not Responding (ANR) issues during service teardown. It also adds Robolectric unit tests to verify this non-blocking behavior. The review feedback suggests using a custom ThreadFactory to name the background threads, which would improve debuggability and thread dump analysis.
|
|
||
| // WakeLock.release() is a Binder call into PowerManager. Doing it on the main | ||
| // thread during service teardown ANRs when the system power service is slow. | ||
| static volatile Executor wakeLockExecutor = Executors.newSingleThreadExecutor(); |
There was a problem hiding this comment.
It is a best practice to provide a custom ThreadFactory to name background threads. This makes debugging, profiling, and analyzing thread dumps much easier (e.g., identifying which thread is responsible for releasing wake locks).
static volatile Executor wakeLockExecutor =
Executors.newSingleThreadExecutor(
new java.util.concurrent.ThreadFactory() {
@Override
public Thread newThread(Runnable r) {
Thread thread = new Thread(r, "firebase-messaging-wakelock-release");
thread.setDaemon(true);
return thread;
}
});
Lyokone
left a comment
There was a problem hiding this comment.
The approach looks right to me. The run lock is read under the lock, each session gets its own lock, and releases are serialized, so a late release can't drop a later session's lock. Going back to one shared run lock makes delayedReleaseDoesNotDropLaterSessionLock and delayedReleaseDoesNotReacquireLaunchLock fail, as intended.
Main points inline:
- The start path still does a main-thread
acquire. - A pre-existing enqueuer-cache bug makes the launch-lock handling unreachable.
- The tests check the executor hand-off rather than non-blocking.
On the PR description:
- "Without this change the first two tests time out": without the change the test file doesn't compile, because
wakeLockExecutordoesn't exist. With only the field added and the release left inline, all four tests fail, and they fail on the release latches rather than on the condition in each test's name. - The numbered test list doesn't match the order of the tests in the file.
- Item 4 ("finishing while a new enqueue is in flight") can't happen in production today because of the enqueuer-cache issue (see the comment on
serviceProcessingFinished).
| // as if we had enqueued work without the service running. | ||
| if (mLaunchingService) { | ||
| try { | ||
| mLaunchWakeLock.acquire(60 * 1000); |
There was a problem hiding this comment.
Is this try/catch needed? It's unrelated to the ANR, it isn't tested, and the branch can't run today (see above). I'd leave it out of this PR.
…ake lock release ComponentNameWithWakeful lacked equals/hashCode, so enqueueWork and the service never shared a CompatWorkEnqueuer and the launch-lock handoff never ran. Tests now stall WakeLock.release on the production executor instead of swapping it, and CI caches Robolectric's android-all jars.
Description
On Android, high-priority background messages run through a wakeful
JobIntentService. When that service is destroyed,onDestroyruns on the main thread and released the run wake lock there.WakeLock.release()is a Binder call into PowerManager, so a slow system power service becomes an ANR. That matches the Crashlytics stack in #18866.This PR posts the run-wake-lock release onto a background executor so teardown does not wait on PowerManager. The start-path
acquirestays on the main thread; moving it would change the window whereenqueueWorkcanstartServicerelative to the run lock being held. Each processing session gets its own run wake lock, so a delayed release cannot drop a lock acquired for a later message. The launch-lock handoff stays under the same lock as the session flags.ComponentNameWithWakefulnow implementsequals/hashCode, soenqueueWorkand the service share oneCompatWorkEnqueuer. Without that, the launch-lock handoff inserviceProcessingFinishednever ran, each high-priority message leaked a 60s launch lock, andsClassWorkEnqueuergrew without bound. The launch-lock acquire on that path has no extra try/catch; it matchesenqueueWork. Android unit tests cache~/.m2/repository/org/robolectricso CI does not re-download the sdk=28 android-all jar on every run.Related Issues
Fixes #18866
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]).This will ensure a smooth and quick review process. Updating the
pubspec.yamland changelogs is not required.///).melos run analyze) does not report any problems on this PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?
Test plan
The ANR is a PowerManager Binder delay during
Service.onDestroy. It is not observable through Dartfirebase_messagingAPIs, and this repo has no FCM-push e2e that can stallIPowerManager.releaseWakeLock. Coverage is a Robolectric unit test that keeps the production executor and uses a WakeLock shadow whoserelease()blocks, so the old inlinerelease()would hangserviceProcessingFinished/onDestroy:serviceProcessingFinishedreturns while the run lock is still held, andrelease()runs on the executor thread.onDestroyreturns while the run lock is still held; the lock is released afterwards, andonHandleWorkis allowed to finish.getWorkEnqueuerreturns the same instance for the same component and wakeful flag, including the instance used fromonCreate.Ran locally:
JobIntentServiceWakeLockTest: 6 tests, 0 failures.