[2661] Fix BroadcastReceiver ANR at ScreenOffTrigger#2732
[2661] Fix BroadcastReceiver ANR at ScreenOffTrigger#2732pyricau merged 2 commits intosquare:mainfrom
Conversation
| analysisClient.newJob(JobContext(ScreenOffTrigger::class)) | ||
| currentJob = job | ||
| analysisExecutor.execute { | ||
| analysisExecutor.execute { |
There was a problem hiding this comment.
If analysisExecutor.execute runs the task quickly enough, there's a chance that we might dump the heap & freeze the VM before the process has had a chance to report back to system_server, right?
Looks to me like if we want to avoid that, we need to execute with a delay instead"?
There was a problem hiding this comment.
Good point. I just updated it with a delayed executor. So far seems to be working as well and ANR is no longer reproducible internally
|
|
| currentJob = null | ||
| analysisCallback(result) | ||
| if(currentJob.compareAndSet(null, job)){ | ||
| delayedScheduledExecutorService.schedule { |
There was a problem hiding this comment.
Couldn't this have been a simple Handler(Looper.getMainLooper()).postDelayed({}, 500) ?
There was a problem hiding this comment.
(you can cancel runnables on handler just fine)
There was a problem hiding this comment.
Good point, simplified to use Handler
| * | ||
| * If not specified, the initial delay is 500 ms | ||
| */ | ||
| private val analysisExecutorDelayMillis: Long = INITIAL_EXECUTOR_DELAY_MILLIS |
There was a problem hiding this comment.
API nit: take in a Duration instead, rename the field to analysisExecutorDelay
There was a problem hiding this comment.
good callout, updated now
FranAguilera
force-pushed
the
franjam/fix_broadcast_receiver_anr
branch
from
2668668 to
d55327c
Compare
FranAguilera
force-pushed
the
franjam/fix_broadcast_receiver_anr
branch
7 times, most recently
from
c2d8541 to
6146c35
Compare
FranAguilera
force-pushed
the
franjam/fix_broadcast_receiver_anr
branch
4 times, most recently
from
eff1023 to
a9147ce
Compare
FranAguilera
force-pushed
the
franjam/fix_broadcast_receiver_anr
branch
2 times, most recently
from
ba4b3f6 to
51e25f9
Compare
Adding delay Add a delayedScheduledExecutorService Fix BroadcastReceiver ANR PR feedback Fix typo
FranAguilera
force-pushed
the
franjam/fix_broadcast_receiver_anr
branch
from
51e25f9 to
0ad89b1
Compare
|
Context from the past: the key issue here wasn't that we were actively blocking the main thread, but rather that we were starting the analysis immediately, which triggered a heap dump, so the VM froze and the receiver wouldn't respond in time. |
3 participants
Resolves #2661
Issue
BroadcastReceiver#onReceive runs by default on the main thread. If onReceive doesn't complete in a timely manner, the system will report an Application Not Responding
We've seen instances of this occurring frequently on internal builds
Solution
NOTE: Kudos to @pyricau for the guidance on adding a delay.
Let the broadcast receiver complete immediately and schedule the executor with an initial delay to avoid issues with the broadcast receiver not completing
Verification
Verified that the the ANR no longer occurs and reporting still completes without issues on one of our main apps (internal release build)
This was heavily tested on internal builds