Merge pull request #494 from AltBeacon/fix-singleton-creation-thread-safety

Ensure singleton object creation thread safety

Author: davidgyoung

CHANGELOG.md

@@ -8,6 +8,8 @@ Bug Fixes:
 - 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

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) {

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;

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) {

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() {