Skip to content

Commit

Permalink
Start documenting thread usage for service objects
Browse files Browse the repository at this point in the history
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
  • Loading branch information
cupakromer committed May 24, 2017
1 parent e505ff9 commit 4d852d9
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 4 deletions.
17 changes: 16 additions & 1 deletion src/main/java/org/altbeacon/beacon/service/BeaconService.java
Original file line number Diff line number Diff line change
Expand Up @@ -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 org.altbeacon.beacon.Beacon;
import org.altbeacon.beacon.BeaconManager;
Expand Down Expand Up @@ -144,9 +146,16 @@ static class IncomingHandler extends Handler {
private final WeakReference<BeaconService> 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<BeaconService>(service);
}

@MainThread
@Override
public void handleMessage(Message msg) {
BeaconService service = mService.get();
Expand Down Expand Up @@ -205,7 +214,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);
Expand Down Expand Up @@ -293,6 +302,7 @@ public boolean onUnbind(Intent intent) {
return false;
}

@MainThread
@Override
public void onDestroy() {
LogManager.e(TAG, "onDestroy()");
Expand Down Expand Up @@ -329,6 +339,7 @@ private PendingIntent getRestartIntent() {
/**
* methods for clients
*/
@MainThread
public void startRangingBeaconsInRegion(Region region, Callback callback) {
synchronized (rangedRegionState) {
if (rangedRegionState.containsKey(region)) {
Expand All @@ -341,6 +352,7 @@ public void startRangingBeaconsInRegion(Region region, Callback callback) {
mCycledScanner.start();
}

@MainThread
public void stopRangingBeaconsInRegion(Region region) {
int rangedRegionCount;
synchronized (rangedRegionState) {
Expand All @@ -354,13 +366,15 @@ public void stopRangingBeaconsInRegion(Region region) {
}
}

@MainThread
public void startMonitoringBeaconsInRegion(Region region, Callback callback) {
LogManager.d(TAG, "startMonitoring called");
monitoringStatus.addRegion(region, callback);
LogManager.d(TAG, "Currently monitoring %s regions.", monitoringStatus.regionsCount());
mCycledScanner.start();
}

@MainThread
public void stopMonitoringBeaconsInRegion(Region region) {
LogManager.d(TAG, "stopMonitoring called");
monitoringStatus.removeRegion(region);
Expand All @@ -370,6 +384,7 @@ public void stopMonitoringBeaconsInRegion(Region region) {
}
}

@MainThread
public void setScanPeriods(long scanPeriod, long betweenScanPeriod, boolean backgroundFlag) {
mCycledScanner.setScanPeriods(scanPeriod, betweenScanPeriod, backgroundFlag);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -165,6 +165,7 @@ public void setScanPeriods(long scanPeriod, long betweenScanPeriod, boolean back
}
}

@MainThread
public void start() {
LogManager.d(TAG, "start called");
mScanningEnabled = true;
Expand All @@ -175,7 +176,7 @@ public void start() {
}
}

@SuppressLint("NewApi")
@MainThread
public void stop() {
LogManager.d(TAG, "stop called");
mScanningEnabled = false;
Expand All @@ -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,
Expand All @@ -218,7 +220,7 @@ public void run() {

protected abstract void startScan();

@SuppressLint("NewApi")
@MainThread
protected void scanLeDevice(final Boolean enable) {
try {
mScanCyclerStarted = true;
Expand Down Expand Up @@ -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();
Expand All @@ -308,6 +311,7 @@ public void run() {

protected abstract void finishScan();

@MainThread
private void finishScanCycle() {
LogManager.d(TAG, "Done with scan cycle");
try {
Expand Down

0 comments on commit 4d852d9

Please sign in to comment.