From 1be324b5ae49bbc12f177c2e3f048e4431993f60 Mon Sep 17 00:00:00 2001 From: Aaron Kromer Date: Wed, 24 May 2017 13:33:13 -0400 Subject: [PATCH] Start documenting thread usage for service objects 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 - https://github.com/AltBeacon/android-beacon-library/pull/430 --- .../altbeacon/beacon/service/BeaconService.java | 17 ++++++++++++++++- .../beacon/service/scanner/CycledLeScanner.java | 10 +++++++--- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/altbeacon/beacon/service/BeaconService.java b/src/main/java/org/altbeacon/beacon/service/BeaconService.java index 3a50855a7..29f0d9007 100644 --- a/src/main/java/org/altbeacon/beacon/service/BeaconService.java +++ b/src/main/java/org/altbeacon/beacon/service/BeaconService.java @@ -37,8 +37,10 @@ import android.os.Build; import android.os.Handler; import android.os.IBinder; +import android.os.Looper; import android.os.Message; import android.os.Messenger; +import android.support.annotation.MainThread; import android.support.annotation.NonNull; import org.altbeacon.beacon.Beacon; @@ -145,9 +147,16 @@ static class IncomingHandler extends Handler { private final WeakReference mService; IncomingHandler(BeaconService service) { + /* + * Explicitly state this uses the main thread. Without this we defer to where the + * service instance is initialized/created; which is usually the main thread anyways. + * But by being explicit we document our code design expectations for where things run. + */ + super(Looper.getMainLooper()); mService = new WeakReference(service); } + @MainThread @Override public void handleMessage(Message msg) { BeaconService service = mService.get(); @@ -206,7 +215,7 @@ else if (msg.what == MSG_SYNC_SETTINGS) { */ final Messenger mMessenger = new Messenger(new IncomingHandler(this)); - + @MainThread @Override public void onCreate() { bluetoothCrashResolver = new BluetoothCrashResolver(this); @@ -294,6 +303,7 @@ public boolean onUnbind(Intent intent) { return false; } + @MainThread @Override public void onDestroy() { LogManager.e(TAG, "onDestroy()"); @@ -330,6 +340,7 @@ private PendingIntent getRestartIntent() { /** * methods for clients */ + @MainThread public void startRangingBeaconsInRegion(Region region, Callback callback) { synchronized (rangedRegionState) { if (rangedRegionState.containsKey(region)) { @@ -342,6 +353,7 @@ public void startRangingBeaconsInRegion(Region region, Callback callback) { mCycledScanner.start(); } + @MainThread public void stopRangingBeaconsInRegion(Region region) { int rangedRegionCount; synchronized (rangedRegionState) { @@ -355,6 +367,7 @@ public void stopRangingBeaconsInRegion(Region region) { } } + @MainThread public void startMonitoringBeaconsInRegion(Region region, Callback callback) { LogManager.d(TAG, "startMonitoring called"); monitoringStatus.addRegion(region, callback); @@ -362,6 +375,7 @@ public void startMonitoringBeaconsInRegion(Region region, Callback callback) { mCycledScanner.start(); } + @MainThread public void stopMonitoringBeaconsInRegion(Region region) { LogManager.d(TAG, "stopMonitoring called"); monitoringStatus.removeRegion(region); @@ -371,6 +385,7 @@ public void stopMonitoringBeaconsInRegion(Region region) { } } + @MainThread public void setScanPeriods(long scanPeriod, long betweenScanPeriod, boolean backgroundFlag) { mCycledScanner.setScanPeriods(scanPeriod, betweenScanPeriod, backgroundFlag); } diff --git a/src/main/java/org/altbeacon/beacon/service/scanner/CycledLeScanner.java b/src/main/java/org/altbeacon/beacon/service/scanner/CycledLeScanner.java index 4c640cd6f..a176da53d 100644 --- a/src/main/java/org/altbeacon/beacon/service/scanner/CycledLeScanner.java +++ b/src/main/java/org/altbeacon/beacon/service/scanner/CycledLeScanner.java @@ -1,7 +1,6 @@ package org.altbeacon.beacon.service.scanner; import android.Manifest; -import android.annotation.SuppressLint; import android.annotation.TargetApi; import android.app.AlarmManager; import android.app.PendingIntent; @@ -125,6 +124,7 @@ public static CycledLeScanner createScanner(Context context, long scanPeriod, lo * between LOW_POWER_MODE vs. LOW_LATENCY_MODE * @param backgroundFlag */ + @MainThread public void setScanPeriods(long scanPeriod, long betweenScanPeriod, boolean backgroundFlag) { LogManager.d(TAG, "Set scan periods called with %s, %s Background mode must have changed.", scanPeriod, betweenScanPeriod); @@ -165,6 +165,7 @@ public void setScanPeriods(long scanPeriod, long betweenScanPeriod, boolean back } } + @MainThread public void start() { LogManager.d(TAG, "start called"); mScanningEnabled = true; @@ -175,7 +176,7 @@ public void start() { } } - @SuppressLint("NewApi") + @MainThread public void stop() { LogManager.d(TAG, "stop called"); mScanningEnabled = false; @@ -194,6 +195,7 @@ public void setDistinctPacketsDetectedPerScan(boolean detected) { mDistinctPacketsDetectedPerScan = detected; } + @MainThread public void destroy() { LogManager.d(TAG, "Destroying"); // We cannot quit the thread used by the handler until queued Runnables have been processed, @@ -218,7 +220,7 @@ public void run() { protected abstract void startScan(); - @SuppressLint("NewApi") + @MainThread protected void scanLeDevice(final Boolean enable) { try { mScanCyclerStarted = true; @@ -285,6 +287,7 @@ protected void scanLeDevice(final Boolean enable) { } } + @MainThread protected void scheduleScanCycleStop() { // Stops scanning after a pre-defined scan period. long millisecondsUntilStop = mScanCycleStopTime - SystemClock.elapsedRealtime(); @@ -308,6 +311,7 @@ public void run() { protected abstract void finishScan(); + @MainThread private void finishScanCycle() { LogManager.d(TAG, "Done with scan cycle"); try {