Fix 2 issues related to thread safety in CustomProperties

microsoft · Oct 11, 2018 · cb45279 · cb45279
1 parent 6eaf73b
commit cb45279
Show file tree

Hide file tree

Showing 6 changed files with 29 additions and 20 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,7 @@
 * **[Feature]** Added a `setMaxStorageSize` API which allows setting a maximum size limit on the local SQLite storage. Previously, up to 300 logs were stored of any size. The default value is 10MB.
 * **[Security]** To enforce TLS 1.2 on all HTTPS connections the SDK makes, we are dropping support for API level 15 (which supports only TLS 1.0), the minimum SDK version thus becomes 16. Previous versions of the SDK were already using TLS 1.2 on API level 16+.
 * **[Bug fix]** Fix validating and discarding `NaN` and infinite double values when calling `setCustomProperties`.
+* **[Bug fix]** Fix thread safety issues when using `setCustomProperties`.
 
 ### AppCenterAnalytics
 

diff --git a/...roidTest/java/com/microsoft/appcenter/ingestion/models/json/LogSerializerAndroidTest.java b/...roidTest/java/com/microsoft/appcenter/ingestion/models/json/LogSerializerAndroidTest.java
@@ -1,6 +1,7 @@
 package com.microsoft.appcenter.ingestion.models.json;
 
 import com.microsoft.appcenter.AndroidTestUtils;
+import com.microsoft.appcenter.Constants;
 import com.microsoft.appcenter.ingestion.models.CustomPropertiesLog;
 import com.microsoft.appcenter.ingestion.models.Log;
 import com.microsoft.appcenter.ingestion.models.LogContainer;
@@ -87,7 +88,7 @@ public void customPropertiesLog() throws JSONException {
         properties.put("t2", new Date(0));
         properties.put("t3", 0);
         properties.put("t4", false);
-        properties.put("t5", null);
+        properties.put("t5", Constants.CLEAR_VALUE);
         log.setProperties(properties);
         UUID sid = UUIDUtils.randomUUID();
         log.setSid(sid);

diff --git a/sdk/appcenter/src/main/java/com/microsoft/appcenter/AppCenter.java b/sdk/appcenter/src/main/java/com/microsoft/appcenter/AppCenter.java
@@ -33,6 +33,7 @@
 
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
@@ -484,7 +485,7 @@ private synchronized void setInstanceCustomProperties(CustomProperties customPro
             AppCenterLog.error(LOG_TAG, "Custom properties may not be null.");
             return;
         }
-        final Map<String, Object> properties = customProperties.getProperties();
+        final Map<String, Object> properties = new HashMap<>(customProperties.getProperties());
         if (properties.size() == 0) {
             AppCenterLog.error(LOG_TAG, "Custom properties may not be empty.");
             return;

diff --git a/sdk/appcenter/src/main/java/com/microsoft/appcenter/Constants.java b/sdk/appcenter/src/main/java/com/microsoft/appcenter/Constants.java
@@ -17,6 +17,11 @@ public class Constants {
      */
     public static final int DEFAULT_TRIGGER_INTERVAL = 3 * 1000;
 
+    /**
+     * Clear special value for custom properties.
+     */
+    public static final Object CLEAR_VALUE = new Object();
+
     /**
      * Number of metrics queue items which will trigger synchronization.
      */

diff --git a/sdk/appcenter/src/main/java/com/microsoft/appcenter/CustomProperties.java b/sdk/appcenter/src/main/java/com/microsoft/appcenter/CustomProperties.java
@@ -5,10 +5,12 @@
 import com.microsoft.appcenter.utils.AppCenterLog;
 
 import java.util.Date;
-import java.util.HashMap;
 import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.regex.Pattern;
 
+import static com.microsoft.appcenter.AppCenter.LOG_TAG;
+
 /**
  * Custom properties builder.
  * Collect multiple properties for send its at once in the same log.
@@ -31,9 +33,9 @@ public class CustomProperties {
 
     /**
      * Properties key/value pairs.
-     * Null value means that key marked to clear.
+     * Clear has a special value (cannot use null in a concurrent map).
      */
-    private final Map<String, Object> mProperties = new HashMap<>();
+    private final Map<String, Object> mProperties = new ConcurrentHashMap<>();
 
     /**
      * Get the properties value.
@@ -74,7 +76,7 @@ public CustomProperties set(String key, Date value) {
             if (value != null) {
                 addProperty(key, value);
             } else {
-                AppCenterLog.error(AppCenter.LOG_TAG, VALUE_NULL_ERROR_MESSAGE);
+                AppCenterLog.error(LOG_TAG, VALUE_NULL_ERROR_MESSAGE);
             }
         }
         return this;
@@ -120,56 +122,54 @@ public CustomProperties set(String key, boolean value) {
      */
     public CustomProperties clear(String key) {
         if (isValidKey(key)) {
-
-            /* Null value means that key marked to clear. */
-            addProperty(key, null);
+            addProperty(key, Constants.CLEAR_VALUE);
         }
         return this;
     }
 
-    private void addProperty(String key, Object value) {
+    private synchronized void addProperty(String key, Object value) {
         if (mProperties.containsKey(key) || mProperties.size() < MAX_PROPERTIES_COUNT) {
             mProperties.put(key, value);
         } else {
-            AppCenterLog.error(AppCenter.LOG_TAG, "Custom properties cannot contain more than " + MAX_PROPERTIES_COUNT + " items");
+            AppCenterLog.error(LOG_TAG, "Custom properties cannot contain more than " + MAX_PROPERTIES_COUNT + " items");
         }
     }
 
     private boolean isValidKey(String key) {
         if (key == null || !KEY_PATTERN.matcher(key).matches()) {
-            AppCenterLog.error(AppCenter.LOG_TAG, "Custom property \""+ key + "\" must match \"" + KEY_PATTERN + "\"");
+            AppCenterLog.error(LOG_TAG, "Custom property \"" + key + "\" must match \"" + KEY_PATTERN + "\"");
             return false;
         }
         if (key.length() > MAX_PROPERTY_KEY_LENGTH) {
-            AppCenterLog.error(AppCenter.LOG_TAG, "Custom property \""+ key + "\" length cannot be longer than " + MAX_PROPERTY_KEY_LENGTH + " characters.");
+            AppCenterLog.error(LOG_TAG, "Custom property \"" + key + "\" length cannot be longer than " + MAX_PROPERTY_KEY_LENGTH + " characters.");
             return false;
         }
         if (mProperties.containsKey(key)) {
-            AppCenterLog.warn(AppCenter.LOG_TAG, "Custom property \"" + key + "\" is already set or cleared and will be overridden.");
+            AppCenterLog.warn(LOG_TAG, "Custom property \"" + key + "\" is already set or cleared and will be overridden.");
         }
         return true;
     }
 
     private boolean isValidStringValue(String key, String value) {
         if (value == null) {
-            AppCenterLog.error(AppCenter.LOG_TAG, VALUE_NULL_ERROR_MESSAGE);
+            AppCenterLog.error(LOG_TAG, VALUE_NULL_ERROR_MESSAGE);
             return false;
         }
         if (value.length() > MAX_PROPERTY_VALUE_LENGTH) {
-            AppCenterLog.error(AppCenter.LOG_TAG, "Custom property \""+ key + "\" value length cannot be longer than " + MAX_PROPERTY_VALUE_LENGTH + " characters.");
+            AppCenterLog.error(LOG_TAG, "Custom property \"" + key + "\" value length cannot be longer than " + MAX_PROPERTY_VALUE_LENGTH + " characters.");
             return false;
         }
         return true;
     }
 
     private boolean isValidNumberValue(String key, Number value) {
         if (value == null) {
-            AppCenterLog.error(AppCenter.LOG_TAG, VALUE_NULL_ERROR_MESSAGE);
+            AppCenterLog.error(LOG_TAG, VALUE_NULL_ERROR_MESSAGE);
             return false;
         }
         double doubleValue = value.doubleValue();
         if (Double.isInfinite(doubleValue) || Double.isNaN(doubleValue)) {
-            AppCenterLog.error(AppCenter.LOG_TAG, "Custom property \""+ key + "\" value cannot be NaN or infinite.");
+            AppCenterLog.error(LOG_TAG, "Custom property \"" + key + "\" value cannot be NaN or infinite.");
             return false;
         }
         return true;

diff --git a/...appcenter/src/main/java/com/microsoft/appcenter/ingestion/models/CustomPropertiesLog.java b/...appcenter/src/main/java/com/microsoft/appcenter/ingestion/models/CustomPropertiesLog.java
@@ -1,5 +1,6 @@
 package com.microsoft.appcenter.ingestion.models;
 
+import com.microsoft.appcenter.Constants;
 import com.microsoft.appcenter.ingestion.models.json.JSONDateUtils;
 import com.microsoft.appcenter.ingestion.models.json.JSONUtils;
 
@@ -62,7 +63,7 @@ private static Object readPropertyValue(JSONObject object) throws JSONException
         String type = object.getString(PROPERTY_TYPE);
         Object value;
         if (type.equals(PROPERTY_TYPE_CLEAR)) {
-            value = null;
+            value = Constants.CLEAR_VALUE;
         } else if (type.equals(PROPERTY_TYPE_BOOLEAN)) {
             value = object.getBoolean(PROPERTY_VALUE);
         } else if (type.equals(PROPERTY_TYPE_NUMBER)) {
@@ -96,7 +97,7 @@ private static void writeProperties(JSONStringer writer, Map<String, Object> pro
     }
 
     private static void writePropertyValue(JSONStringer writer, Object value) throws JSONException {
-        if (value == null) {
+        if (value == Constants.CLEAR_VALUE) {
             JSONUtils.write(writer, PROPERTY_TYPE, PROPERTY_TYPE_CLEAR);
         } else if (value instanceof Boolean) {
             JSONUtils.write(writer, PROPERTY_TYPE, PROPERTY_TYPE_BOOLEAN);