RUM-16123: Move broadcast receiver dispatch off the main thread#3524
Conversation
The trampoline introduced in #3420 didn't fix the ANRs (see #3480) — PairIP wraps onReceive() at entry on the main thread, before the trampoline can offload its body. Remove the executorService parameters and wiring on both BroadcastReceiver*InfoProviders ahead of a follow-up commit that registers the receivers with a background Handler. Keep @volatile on systemInfo, networkInfo, and lastNetworkInfo since they're independently useful for cross-thread visibility once dispatch moves off-main.
Introduce BroadcastReceiverThread, a dedicated HandlerThread used to deliver CONNECTIVITY_ACTION, ACTION_BATTERY_CHANGED, and ACTION_POWER_SAVE_MODE_CHANGED broadcasts to BroadcastReceiverNetworkInfoProvider and BroadcastReceiverSystemInfoProvider on a background thread instead of the main thread. The handler is passed to registerReceiver via ThreadSafeReceiver, and the thread is created in setupExecutors() and shut down in shutDownExecutors(). The @volatile fields on networkInfo and systemInfo continue to guarantee visibility for any reader thread.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3524 +/- ##
===========================================
+ Coverage 72.21% 72.24% +0.03%
===========================================
Files 965 966 +1
Lines 35643 35652 +9
Branches 5948 5948
===========================================
+ Hits 25739 25756 +17
+ Misses 8278 8275 -3
+ Partials 1626 1621 -5
🚀 New features to boost your workflow:
|
| * Lifecycle: the underlying thread is started eagerly at construction time. | ||
| * Call [shutdown] when the SDK is torn down to release the thread. | ||
| */ | ||
| internal class BroadcastReceiverThread { |
There was a problem hiding this comment.
This is a bit confusing, because this class is not a thread, despite having Thread in the name.
Maybe we can make it extend HandlerThread? internal class BroadcastReceiverThread: HandlerThread("datadog-broadcast-receiver-thread") without any properties and simply do Handler creation and wiring in CoreFeature? WDYT?
There was a problem hiding this comment.
Yeah, make sense. 🙇♂️
Make BroadcastReceiverThread extend HandlerThread directly instead of wrapping one, removing the handler property and shutdown() wrapper. Create a single Handler from the thread's looper in setupInfoProviders() and share it across BroadcastReceiverSystemInfoProvider and BroadcastReceiverNetworkInfoProvider.
| // region BroadcastReceiver | ||
|
|
||
| override fun onReceive(context: Context, intent: Intent?) { | ||
| executorService.executeSafe(HANDLE_INTENT_OPERATION_NAME, internalLogger) { |
There was a problem hiding this comment.
Question about how ANR can happen with previous approach, since we offload immediately the onReceive from main thread to executorService, the blocking task which causes ANR must happen before onReceive is called?
There was a problem hiding this comment.
Correct. My guess is that PairIP instruments onReceive at the method entry point — before any code in the body runs, including the executorService.executeSafe(...) call. The overhead is in the PairIP VM's instrumentation of the method itself on the main thread looper, not in anything the body does. So by the time the executor would offload the work, the ANR-causing delay has already occurred on the main thread. Passing a background Handler to registerReceiver moves the entire dispatch — including the PairIP entry instrumentation — off the main thread, which is why I hope this approach will work.
5 participants
What does this PR do?
Introduces
BroadcastReceiverThread, a dedicatedHandlerThreadthat serves as the dispatch thread for the SDK'sBroadcastReceivers. The handler is passed toContext.registerReceiverviaThreadSafeReceiver, soCONNECTIVITY_ACTION,ACTION_BATTERY_CHANGED, andACTION_POWER_SAVE_MODE_CHANGEDcallbacks are delivered on the background thread instead of the main thread.Motivation
Fixes ANRs reported in #3480. Root cause:
Context.registerReceiver(...)without aHandlerargument dispatchesonReceiveon the main thread by default. On devices protected by PairIP (Google Play Integrity Code Transparency), eachonReceivecall incurs non-deterministic overhead fromcom.pairip.VMRunner.executeVMon the main thread looper, causing ANRs. PR #3420 attempted to fix this by trampoliningonReceivebody through an executor, but PairIP wrapsonReceiveat entry on the main thread — before the trampoline could offload work — so the ANRs persisted. This PR reverts that executor-trampoline approach and takes a different one instead: passing a backgroundHandlertoregisterReceiverso the framework never dispatches on the main thread at all. The@Volatilefields onnetworkInfoandsystemInfointroduced in #3420 are retained for cross-thread visibility.Review checklist (to be filled by reviewers)