Merge branch 'master' into no-scanning-restarts-on-duplicate-detection-devices

davidgyoung · davidgyoung · commit 1e49cd6bffd7 · 2017-04-21T18:04:47.000-04:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,6 +11,11 @@ Bug Fixes:
 - Deprecate misspelled methods `removeMonitoreNotifier` and
   `setRegionStatePeristenceEnabled` in favor of correctly spelled alternatives.
   (#461, Marco Salis)
+- Fix bug causing brief scan dropouts after starting a scan after a long period
+  of inactivity (i.e. startup and background-foreground transitions) due to
+  Android N scan limits (#489, Aaron Kromer)
+- Ensure thread safety for singleton creation of `BeaconManager`,
+  `DetectionTracker`, `MonitoringStatus`, and `Stats`. (#494, Aaron Kromer)
 
 
 ### 2.9.2 / 2016-11-22
diff --git a/src/main/java/org/altbeacon/beacon/BeaconManager.java b/src/main/java/org/altbeacon/beacon/BeaconManager.java
@@ -108,7 +108,7 @@
 public class BeaconManager {
     private static final String TAG = "BeaconManager";
     private Context mContext;
-    protected static BeaconManager client = null;
+    protected static volatile BeaconManager client = null;
     private final ConcurrentMap<BeaconConsumer, ConsumerInfo> consumers = new ConcurrentHashMap<BeaconConsumer,ConsumerInfo>();
     private Messenger serviceMessenger = null;
     protected final Set<RangeNotifier> rangeNotifiers = new CopyOnWriteArraySet<>();
@@ -123,6 +123,11 @@ public class BeaconManager {
     private static boolean sAndroidLScanningDisabled = false;
     private static boolean sManifestCheckingDisabled = false;
 
+    /**
+     * Private lock object for singleton initialization protecting against denial-of-service attack.
+     */
+    private static final Object SINGLETON_LOCK = new Object();
+
     /**
      * Set to true if you want to show library debugging.
      *
@@ -239,11 +244,29 @@ public static long getRegionExitPeriod(){
      * or non-Service class, you can attach it to another singleton or a subclass of the Android Application class.
      */
     public static BeaconManager getInstanceForApplication(Context context) {
-        if (client == null) {
-            LogManager.d(TAG, "BeaconManager instance creation");
-            client = new BeaconManager(context);
+        /*
+         * Follow double check pattern from Effective Java v2 Item 71.
+         *
+         * Bloch recommends using the local variable for this for performance reasons:
+         *
+         * > What this variable does is ensure that `field` is read only once in the common case
+         * > where it's already initialized. While not strictly necessary, this may improve
+         * > performance and is more elegant by the standards applied to low-level concurrent
+         * > programming. On my machine, [this] is about 25 percent faster than the obvious
+         * > version without a local variable.
+         *
+         * Joshua Bloch. Effective Java, Second Edition. Addison-Wesley, 2008. pages 283-284
+         */
+        BeaconManager instance = client;
+        if (instance == null) {
+            synchronized (SINGLETON_LOCK) {
+                instance = client;
+                if (instance == null) {
+                    client = instance = new BeaconManager(context);
+                }
+            }
         }
-        return client;
+        return instance;
     }
 
    protected BeaconManager(Context context) {
@@ -271,17 +294,10 @@ public List<BeaconParser> getBeaconParsers() {
      */
     @TargetApi(18)
     public boolean checkAvailability() throws BleNotAvailableException {
-        if (android.os.Build.VERSION.SDK_INT < 18) {
+        if (!isBleAvailable()) {
             throw new BleNotAvailableException("Bluetooth LE not supported by this device");
         }
-        if (!mContext.getPackageManager().hasSystemFeature(PackageManager.FEATURE_BLUETOOTH_LE)) {
-            throw new BleNotAvailableException("Bluetooth LE not supported by this device");
-        } else {
-            if (((BluetoothManager) mContext.getSystemService(Context.BLUETOOTH_SERVICE)).getAdapter().isEnabled()) {
-                return true;
-            }
-        }
-        return false;
+        return ((BluetoothManager) mContext.getSystemService(Context.BLUETOOTH_SERVICE)).getAdapter().isEnabled();
     }
 
     /**
@@ -292,8 +308,12 @@ public boolean checkAvailability() throws BleNotAvailableException {
      * @param consumer the <code>Activity</code> or <code>Service</code> that will receive the callback when the service is ready.
      */
     public void bind(BeaconConsumer consumer) {
-        if (android.os.Build.VERSION.SDK_INT < 18) {
-            LogManager.w(TAG, "Not supported prior to API 18.  Method invocation will be ignored");
+        if (!isBleAvailable()) {
+            LogManager.w(TAG, "Method invocation will be ignored.");
+            return;
+        }
+        if (!mContext.getPackageManager().hasSystemFeature(PackageManager.FEATURE_BLUETOOTH_LE)) {
+            LogManager.w(TAG, "This device does not support bluetooth LE.  Will not start beacon scanning.");
             return;
         }
         synchronized (consumers) {
@@ -318,8 +338,8 @@ public void bind(BeaconConsumer consumer) {
      * @param consumer the <code>Activity</code> or <code>Service</code> that no longer needs to use the service.
      */
     public void unbind(BeaconConsumer consumer) {
-        if (android.os.Build.VERSION.SDK_INT < 18) {
-            LogManager.w(TAG, "Not supported prior to API 18.  Method invocation will be ignored");
+        if (!isBleAvailable()) {
+            LogManager.w(TAG, "Method invocation will be ignored.");
             return;
         }
         synchronized (consumers) {
@@ -391,8 +411,9 @@ public boolean isAnyConsumerBound() {
      * @see #setBackgroundBetweenScanPeriod(long p)
      */
     public void setBackgroundMode(boolean backgroundMode) {
-        if (android.os.Build.VERSION.SDK_INT < 18) {
-            LogManager.w(TAG, "Not supported prior to API 18.  Method invocation will be ignored");
+        if (!isBleAvailable()) {
+            LogManager.w(TAG, "Method invocation will be ignored.");
+            return;
         }
         mBackgroundModeUninitialized = false;
         if (backgroundMode != mBackgroundMode) {
@@ -608,8 +629,8 @@ public void requestStateForRegion(Region region) {
      */
     @TargetApi(18)
     public void startRangingBeaconsInRegion(Region region) throws RemoteException {
-        if (android.os.Build.VERSION.SDK_INT < 18) {
-            LogManager.w(TAG, "Not supported prior to API 18.  Method invocation will be ignored");
+        if (!isBleAvailable()) {
+            LogManager.w(TAG, "Method invocation will be ignored.");
             return;
         }
         if (serviceMessenger == null) {
@@ -636,8 +657,8 @@ public void startRangingBeaconsInRegion(Region region) throws RemoteException {
      */
     @TargetApi(18)
     public void stopRangingBeaconsInRegion(Region region) throws RemoteException {
-        if (android.os.Build.VERSION.SDK_INT < 18) {
-            LogManager.w(TAG, "Not supported prior to API 18.  Method invocation will be ignored");
+        if (!isBleAvailable()) {
+            LogManager.w(TAG, "Method invocation will be ignored.");
             return;
         }
         if (serviceMessenger == null) {
@@ -671,8 +692,8 @@ public void stopRangingBeaconsInRegion(Region region) throws RemoteException {
      */
     @TargetApi(18)
     public void startMonitoringBeaconsInRegion(Region region) throws RemoteException {
-        if (android.os.Build.VERSION.SDK_INT < 18) {
-            LogManager.w(TAG, "Not supported prior to API 18.  Method invocation will be ignored");
+        if (!isBleAvailable()) {
+            LogManager.w(TAG, "Method invocation will be ignored.");
             return;
         }
         if (serviceMessenger == null) {
@@ -699,8 +720,8 @@ public void startMonitoringBeaconsInRegion(Region region) throws RemoteException
      */
     @TargetApi(18)
     public void stopMonitoringBeaconsInRegion(Region region) throws RemoteException {
-        if (android.os.Build.VERSION.SDK_INT < 18) {
-            LogManager.w(TAG, "Not supported prior to API 18.  Method invocation will be ignored");
+        if (!isBleAvailable()) {
+            LogManager.w(TAG, "Method invocation will be ignored.");
             return;
         }
         if (serviceMessenger == null) {
@@ -721,8 +742,8 @@ public void stopMonitoringBeaconsInRegion(Region region) throws RemoteException
      */
     @TargetApi(18)
     public void updateScanPeriods() throws RemoteException {
-        if (android.os.Build.VERSION.SDK_INT < 18) {
-            LogManager.w(TAG, "Not supported prior to API 18.  Method invocation will be ignored");
+        if (!isBleAvailable()) {
+            LogManager.w(TAG, "Method invocation will be ignored.");
             return;
         }
         if (serviceMessenger == null) {
@@ -895,6 +916,18 @@ public void setNonBeaconLeScanCallback(NonBeaconLeScanCallback callback) {
         mNonBeaconLeScanCallback = callback;
     }
 
+    private boolean isBleAvailable() {
+        boolean available = false;
+        if (android.os.Build.VERSION.SDK_INT < android.os.Build.VERSION_CODES.JELLY_BEAN_MR2) {
+            LogManager.w(TAG, "Bluetooth LE not supported prior to API 18.");
+        } else if (!mContext.getPackageManager().hasSystemFeature(PackageManager.FEATURE_BLUETOOTH_LE)) {
+            LogManager.w(TAG, "This device does not support bluetooth LE.");
+        } else {
+            available = true;
+        }
+        return available;
+    }
+
     private long getScanPeriod() {
         if (mBackgroundMode) {
             return backgroundScanPeriod;
diff --git a/src/main/java/org/altbeacon/beacon/powersave/BackgroundPowerSaver.java b/src/main/java/org/altbeacon/beacon/powersave/BackgroundPowerSaver.java
@@ -44,8 +44,8 @@ public BackgroundPowerSaver(Context context) {
             LogManager.w(TAG, "BackgroundPowerSaver requires API 18 or higher.");
             return;
         }
-        ((Application)context.getApplicationContext()).registerActivityLifecycleCallbacks(this);
         beaconManager = BeaconManager.getInstanceForApplication(context);
+        ((Application)context.getApplicationContext()).registerActivityLifecycleCallbacks(this);
     }
 
     @Override
diff --git a/src/main/java/org/altbeacon/beacon/service/DetectionTracker.java b/src/main/java/org/altbeacon/beacon/service/DetectionTracker.java
@@ -6,16 +6,14 @@
  * Created by dyoung on 1/10/15.
  */
 public class DetectionTracker {
-    private static DetectionTracker sDetectionTracker = null;
+    private static final DetectionTracker INSTANCE = new DetectionTracker();
+
     private long mLastDetectionTime = 0l;
     private DetectionTracker() {
 
     }
-    public static synchronized DetectionTracker getInstance() {
-        if (sDetectionTracker == null) {
-            sDetectionTracker  = new DetectionTracker();
-        }
-        return sDetectionTracker;
+    public static DetectionTracker getInstance() {
+        return INSTANCE;
     }
     public long getLastDetectionTime() {
         return mLastDetectionTime;
diff --git a/src/main/java/org/altbeacon/beacon/service/MonitoringStatus.java b/src/main/java/org/altbeacon/beacon/service/MonitoringStatus.java
@@ -23,7 +23,7 @@
 import static android.content.Context.MODE_PRIVATE;
 
 public class MonitoringStatus {
-    private static MonitoringStatus sInstance;
+    private static volatile MonitoringStatus sInstance;
     private static final int MAX_REGIONS_FOR_STATUS_PRESERVATION = 50;
     private static final int MAX_STATUS_PRESERVATION_FILE_AGE_TO_RESTORE_SECS = 60 * 15;
     private static final String TAG = MonitoringStatus.class.getSimpleName();
@@ -35,15 +35,35 @@ public class MonitoringStatus {
 
     private boolean mStatePreservationIsOn = true;
 
+    /**
+     * Private lock object for singleton initialization protecting against denial-of-service attack.
+     */
+    private static final Object SINGLETON_LOCK = new Object();
+
     public static MonitoringStatus getInstanceForApplication(Context context) {
-        if (sInstance == null) {
-            synchronized (MonitoringStatus.class) {
-                if (sInstance == null) {
-                    sInstance = new MonitoringStatus(context.getApplicationContext());
+        /*
+         * Follow double check pattern from Effective Java v2 Item 71.
+         *
+         * Bloch recommends using the local variable for this for performance reasons:
+         *
+         * > What this variable does is ensure that `field` is read only once in the common case
+         * > where it's already initialized. While not strictly necessary, this may improve
+         * > performance and is more elegant by the standards applied to low-level concurrent
+         * > programming. On my machine, [this] is about 25 percent faster than the obvious
+         * > version without a local variable.
+         *
+         * Joshua Bloch. Effective Java, Second Edition. Addison-Wesley, 2008. pages 283-284
+         */
+        MonitoringStatus instance = sInstance;
+        if (instance == null) {
+            synchronized (SINGLETON_LOCK) {
+                instance = sInstance;
+                if (instance == null) {
+                    sInstance = instance = new MonitoringStatus(context.getApplicationContext());
                 }
             }
         }
-        return sInstance;
+        return instance;
     }
 
     public MonitoringStatus(Context context) {
diff --git a/src/main/java/org/altbeacon/beacon/service/Stats.java b/src/main/java/org/altbeacon/beacon/service/Stats.java
@@ -12,7 +12,12 @@
  * Created by dyoung on 10/16/14.
  */
 public class Stats {
+    private static final Stats INSTANCE = new Stats();
     private static final String TAG = "Stats";
+
+    /**
+     * Synchronize all usage as this is not a thread safe class.
+     */
     private static final SimpleDateFormat SIMPLE_DATE_FORMAT = new SimpleDateFormat("HH:mm:ss.SSS");
 
     private ArrayList<Sample> mSamples;
@@ -21,14 +26,11 @@ public class Stats {
     private boolean mEnableHistoricalLogging;
     private boolean mEnabled;
     private Sample mSample;
-    private static Stats mInstance;
 
     public static Stats getInstance() {
-        if(mInstance == null) {
-            mInstance = new Stats();
-        }
-        return mInstance;
+        return INSTANCE;
     }
+
     private Stats() {
         mSampleIntervalMillis = 0l;
         clearSamples();
@@ -105,7 +107,13 @@ private void logSample(Sample sample, boolean showHeader) {
     }
 
     private String formattedDate(Date d) {
-        return d == null ? "" : SIMPLE_DATE_FORMAT.format(d);
+        String formattedDate = "";
+        if (d != null) {
+            synchronized (SIMPLE_DATE_FORMAT) {
+                formattedDate = SIMPLE_DATE_FORMAT.format(d);
+            }
+        }
+        return formattedDate;
     }
 
     private void logSamples() {
diff --git a/src/main/java/org/altbeacon/beacon/service/scanner/CycledLeScanner.java b/src/main/java/org/altbeacon/beacon/service/scanner/CycledLeScanner.java
@@ -31,7 +31,6 @@ public abstract class CycledLeScanner {
     private long mLastScanCycleEndTime = 0l;
     protected long mNextScanCycleStartTime = 0l;
     private long mScanCycleStopTime = 0l;
-    private long mLastScanStopTime = 0l;
 
     private boolean mScanning;
     protected boolean mScanningPaused;
@@ -158,10 +157,8 @@ public void stop() {
         mScanningEnabled = false;
         if (mScanCyclerStarted) {
             scanLeDevice(false);
-        }
-        if (mBluetoothAdapter != null) {
-            stopScan();
-            mLastScanCycleEndTime = SystemClock.elapsedRealtime();
+        } else {
+            LogManager.d(TAG, "scanning already stopped");
         }
     }
 
@@ -289,23 +286,22 @@ private void finishScanCycle() {
                         // packets in the same cycle, we will not restart scanning and just keep it
                         // going.
                         if (!getDistinctPacketsDetectedPerScan()) {
-                            long now = System.currentTimeMillis();
+                            long now = SystemClock.elapsedRealtime();
                             if (android.os.Build.VERSION.SDK_INT >= Build.VERSION_CODES.N &&
                                     mBetweenScanPeriod+mScanPeriod < ANDROID_N_MIN_SCAN_CYCLE_MILLIS &&
-                                    now-mLastScanStopTime < ANDROID_N_MIN_SCAN_CYCLE_MILLIS) {
+                                    now-mLastScanCycleStartTime < ANDROID_N_MIN_SCAN_CYCLE_MILLIS) {
                                 // As of Android N, only 5 scans may be started in a 30 second period (6
                                 // seconds per cycle)  otherwise they are blocked.  So we check here to see
                                 // if the scan period is 6 seconds or less, and if we last stopped scanning
                                 // fewer than 6 seconds ag and if so, we simply do not stop scanning
                                 LogManager.d(TAG, "Not stopping scan because this is Android N and we" +
                                         " keep scanning for a minimum of 6 seconds at a time. "+
-                                        "We will stop in "+(ANDROID_N_MIN_SCAN_CYCLE_MILLIS-(now-mLastScanStopTime))+" millisconds.");
+                                        "We will stop in "+(ANDROID_N_MIN_SCAN_CYCLE_MILLIS-(now-mLastScanCycleStartTime))+" millisconds.");
                             }
                             else {
                                 try {
                                     LogManager.d(TAG, "stopping bluetooth le scan");
                                     finishScan();
-                                    mLastScanStopTime = now;
                                 } catch (Exception e) {
                                     LogManager.w(e, TAG, "Internal Android exception scanning for beacons");
                                 }

Original file line number	Diff line number	Diff line change
`@@ -44,8 +44,8 @@ public BackgroundPowerSaver(Context context) {`
`44`	`44`	`LogManager.w(TAG, "BackgroundPowerSaver requires API 18 or higher.");`
`45`	`45`	`return;`
`46`	`46`	`}`
`47`		`- ((Application)context.getApplicationContext()).registerActivityLifecycleCallbacks(this);`
`48`	`47`	`beaconManager = BeaconManager.getInstanceForApplication(context);`
	`48`	`+ ((Application)context.getApplicationContext()).registerActivityLifecycleCallbacks(this);`
`49`	`49`	`}`
`50`	`50`
`51`	`51`	`@Override`