Start and stop BLE scans from a background thread to prevent blocking the UI (issue #136) #430
Conversation
|
Thanks, @marcosalis. One concern I have is that this change appears to enforce that callbacks always happen on the main thread. This may cause ANR problems for existing applications that do not expect this and upgrade to a version of the library with this change. This would happen for any app doing heavy processing in a beacon callback. Thoughts? |
|
Thank you for your answer @davidgyoung.
If you're talking about the former, from what I see in the codebase the callbacks are always executed from |
|
@davidgyoung any updates on this? Just as additional information and motivation to implement this change, here's a screenshot of a method profiling on the main thread when the |
|
Sorry for the delay on this, @marcosalis. I will do some testing on this today. |
| protected final Handler mHandler = new Handler(); | ||
|
|
||
| protected final Handler mHandler = new Handler(Looper.getMainLooper()); | ||
| protected final Handler mScanHandler; |
There was a problem hiding this comment.
Is there a reason to use a HandlerThread here? Why not just construct mScanHandler = new Handler();? Then you don't need to clean up the handler thread later.
There was a problem hiding this comment.
@davidgyoung see my answer below (and this doc), this is the change that makes the whole PR work: by creating it with new Handler() you'd be using the Looper from the thread that's instantiating CycledLeScanner, which is the main thread.
Instead, by creating a HandlerThread (which is just a thread with a Looper attached to it) we make sure the "expensive" Bluetooth code from the SDK is posted and executed in our own worker thread.
There was a problem hiding this comment.
OK, got it now... apologies, it's been awhile, it's been awhile since I looked at this code. I was incorrectly thinking that new Handler() always creates a new thread.
| @@ -40,7 +42,10 @@ | |||
| private long mScanPeriod; | |||
|
|
|||
| protected long mBetweenScanPeriod; | |||
| protected final Handler mHandler = new Handler(); | |||
|
|
|||
| protected final Handler mHandler = new Handler(Looper.getMainLooper()); | |||
There was a problem hiding this comment.
What is the point of calling Looper.getMainLooper() here? This handler will never need to communicate with the main UI thread, so why not let it have its own looper?
There was a problem hiding this comment.
The default, parameterless Handler constructor uses the Looper from the current thread (as documented here and here).
In this particular case, CycledLeScanner is always instantiated from the main thread (from BeaconService.onCreate()) and consequently, that Handler will already use the main thread's looper.
This addition doesn't modify the current logic, it's only a futureproof guarantee that the class behaviour won't change if in the future if CycledLeScanner is instantiated from another thread that doesn't have a Looper (you would immediately notice because the Handler creation would crash).
|
I've tested this code, and it works for me, but I have some questions about the handler setup I'd like to resolve before merging. |
|
Thanks for your patience here, @marcosalis. Another question: Wouldn't it be simpler to create a single |
|
@davidgyoung you could use this approach, but then you'd have to make all the |
|
Fair point. Ok, looks good to me. |
|
Thank you @davidgyoung ! |
This makes the current implicit assumption that the scan cycler is started/stopped on the main thread explicit. This helps quickly inform developers what the callback expectations are in regards to race conditions and state safety. In order to verify this we need to trace the `start`/`stop`/`destroy` calls back to the service. The service's core methods `onCreate` and `onDestroy` are called on the main thread (see links below). This means the service creates it's `mMessenger` on the main thread, which creates the `IncomingHandler` on the main thread which binds to the main looper. However, the `IncomingHandler` class itself leaves the looper unspecified relying on looper for the thread which creates it. As we did in #430, this modifies `IncomingHandler` to explicitly use the main looper as a future proofing mechanism. This way we can ensure this implicit expectation doesn't change or at least make it obvious when troubleshooting cases where someone expects it to have changed. Similarly, this adds the main thread annotation to it's `handleMessage` implementation. Working from here we can confirm that the only places where the beacon monitoring/ranging is updated is from the main thread through the `IncomingHandler`. As the `IncomingHandler` is the main communication channel for the service we transfer the main thread expectation to the associated ranging/monitoring methods. _If_ someone decides to call these methods directly on the service these annotations help the IDEs check/document that such calls are expected from the main thread. With all of that documented we can now confidently state that the scan cycler's `start`, `stop`, and `destroy` are also meant to be called from the main thread. As `start` and `stop` both call `scanLeDevice` we can start tracing any other threading expectations for it. We already know it's called from the main thread through deferred jobs. This leaves the `finishScanCycle` as the last call site. `finishScanCycle` is only called from `scheduleScanCycleStop`. As this method name implies it's called through a deferred job on the main thread. It is also called the "first" time in `scanLeDevice`. Thus we've shown that `scanLeDevice`, `finishScanCycle`, and `scheduleScanCycleStop` are expected to run on the main thread. See Also: - https://developer.android.com/reference/android/os/Handler.html#Handler%28%29 - https://developer.android.com/training/multiple-threads/communicate-ui.html - https://developer.android.com/reference/android/app/Service.html - https://developer.android.com/guide/components/services.html - #430
This makes the current implicit assumption that the scan cycler is started/stopped on the main thread explicit. This helps quickly inform developers what the callback expectations are in regards to race conditions and state safety. In order to verify this we need to trace the `start`/`stop`/`destroy` calls back to the service. The service's core methods `onCreate` and `onDestroy` are called on the main thread (see links below). This means the service creates it's `mMessenger` on the main thread, which creates the `IncomingHandler` on the main thread which binds to the main looper. However, the `IncomingHandler` class itself leaves the looper unspecified relying on looper for the thread which creates it. As we did in #430, this modifies `IncomingHandler` to explicitly use the main looper as a future proofing mechanism. This way we can ensure this implicit expectation doesn't change or at least make it obvious when troubleshooting cases where someone expects it to have changed. Similarly, this adds the main thread annotation to it's `handleMessage` implementation. Working from here we can confirm that the only places where the beacon monitoring/ranging is updated is from the main thread through the `IncomingHandler`. As the `IncomingHandler` is the main communication channel for the service we transfer the main thread expectation to the associated ranging/monitoring methods. _If_ someone decides to call these methods directly on the service these annotations help the IDEs check/document that such calls are expected from the main thread. With all of that documented we can now confidently state that the scan cycler's `start`, `stop`, and `destroy` are also meant to be called from the main thread. As `start` and `stop` both call `scanLeDevice` we can start tracing any other threading expectations for it. We already know it's called from the main thread through deferred jobs. This leaves the `finishScanCycle` as the last call site. `finishScanCycle` is only called from `scheduleScanCycleStop`. As this method name implies it's called through a deferred job on the main thread. It is also called the "first" time in `scanLeDevice`. Thus we've shown that `scanLeDevice`, `finishScanCycle`, and `scheduleScanCycleStop` are expected to run on the main thread. See Also: - https://developer.android.com/reference/android/os/Handler.html#Handler%28%29 - https://developer.android.com/training/multiple-threads/communicate-ui.html - https://developer.android.com/reference/android/app/Service.html - https://developer.android.com/guide/components/services.html - #430
This makes the current implicit assumption that the scan cycler is started/stopped on the main thread explicit. This helps quickly inform developers what the callback expectations are in regards to race conditions and state safety. In order to verify this we need to trace the `start`/`stop`/`destroy` calls back to the service. The service's core methods `onCreate` and `onDestroy` are called on the main thread (see links below). This means the service creates it's `mMessenger` on the main thread, which creates the `IncomingHandler` on the main thread which binds to the main looper. However, the `IncomingHandler` class itself leaves the looper unspecified relying on looper for the thread which creates it. As we did in #430, this modifies `IncomingHandler` to explicitly use the main looper as a future proofing mechanism. This way we can ensure this implicit expectation doesn't change or at least make it obvious when troubleshooting cases where someone expects it to have changed. Similarly, this adds the main thread annotation to it's `handleMessage` implementation. Working from here we can confirm that the only places where the beacon monitoring/ranging is updated is from the main thread through the `IncomingHandler`. As the `IncomingHandler` is the main communication channel for the service we transfer the main thread expectation to the associated ranging/monitoring methods. _If_ someone decides to call these methods directly on the service these annotations help the IDEs check/document that such calls are expected from the main thread. With all of that documented we can now confidently state that the scan cycler's `start`, `stop`, and `destroy` are also meant to be called from the main thread. As `start` and `stop` both call `scanLeDevice` we can start tracing any other threading expectations for it. We already know it's called from the main thread through deferred jobs. This leaves the `finishScanCycle` as the last call site. `finishScanCycle` is only called from `scheduleScanCycleStop`. As this method name implies it's called through a deferred job on the main thread. It is also called the "first" time in `scanLeDevice`. Thus we've shown that `scanLeDevice`, `finishScanCycle`, and `scheduleScanCycleStop` are expected to run on the main thread. See Also: - https://developer.android.com/reference/android/os/Handler.html#Handler%28%29 - https://developer.android.com/training/multiple-threads/communicate-ui.html - https://developer.android.com/reference/android/app/Service.html - https://developer.android.com/guide/components/services.html - #430
This pull request fixes the problems reported in issue #136 where ANRs are caused by AltBeacon accessing the BluetoothAdapter (which has heavy internal synchronization and even explicit wait() calls) from the main thread.
A HandlerThread is used for the purpose, and no internal state is accessed from within the posted runnables so that it's not necessary to make the CycledLeScanner classes thread safe. In the same way, callbacks from the BluetoothAdapter are already always executed from the main thread (they're called, internally, from a Handler created with the main looper).