From fbe9f74f3014c2293b4908f3d36f10ee4a8f0803 Mon Sep 17 00:00:00 2001 From: jinliu9508 Date: Tue, 2 Jan 2024 15:48:08 -0500 Subject: [PATCH] Resolve the deadlock and pass the unit test --- .../com/onesignal/common/modeling/Model.kt | 118 +++++++++++------- .../onesignal/common/modeling/ModelStore.kt | 6 +- .../subscriptions/SubscriptionManagerTests.kt | 7 +- 3 files changed, 76 insertions(+), 55 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/Model.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/Model.kt index 7864872838..1f335bf8d3 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/Model.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/Model.kt @@ -80,33 +80,52 @@ open class Model( * @param jsonObject The [JSONObject] to initialize this model from. */ fun initializeFromJson(jsonObject: JSONObject) { - data.clear() - for (property in jsonObject.keys()) { - val jsonValue = jsonObject.get(property) - if (jsonValue is JSONObject) { - val childModel = createModelForProperty(property, jsonValue) - if (childModel != null) { - data[property] = childModel - } - } else if (jsonValue is JSONArray) { - val listOfItems = createListForProperty(property, jsonValue) - if (listOfItems != null) { - data[property] = listOfItems - } - } else { - val method = this.javaClass.methods.firstOrNull { it.returnType != Void::class.java && it.name.contains(property, true) } - - if (method == null) { - data[property] = jsonObject.get(property) + synchronized(data) { + data.clear() + for (property in jsonObject.keys()) { + val jsonValue = jsonObject.get(property) + if (jsonValue is JSONObject) { + val childModel = createModelForProperty(property, jsonValue) + if (childModel != null) { + data[property] = childModel + } + } else if (jsonValue is JSONArray) { + val listOfItems = createListForProperty(property, jsonValue) + if (listOfItems != null) { + data[property] = listOfItems + } } else { - when (method.returnType) { - Double::class.java, java.lang.Double::class.java -> data[property] = jsonObject.getDouble(property) - Long::class.java, java.lang.Long::class.java -> data[property] = jsonObject.getLong(property) - Float::class.java, java.lang.Float::class.java -> data[property] = jsonObject.getDouble(property).toFloat() - Int::class.java, java.lang.Integer::class.java -> data[property] = jsonObject.getInt(property) - Boolean::class.java, java.lang.Boolean::class.java -> data[property] = jsonObject.getBoolean(property) - String::class.java, java.lang.String::class.java -> data[property] = jsonObject.getString(property) - else -> data[property] = jsonObject.get(property) + val method = this.javaClass.methods.firstOrNull { + it.returnType != Void::class.java && it.name.contains( + property, + true + ) + } + + if (method == null) { + data[property] = jsonObject.get(property) + } else { + when (method.returnType) { + Double::class.java, java.lang.Double::class.java -> data[property] = + jsonObject.getDouble(property) + + Long::class.java, java.lang.Long::class.java -> data[property] = + jsonObject.getLong(property) + + Float::class.java, java.lang.Float::class.java -> data[property] = + jsonObject.getDouble(property).toFloat() + + Int::class.java, java.lang.Integer::class.java -> data[property] = + jsonObject.getInt(property) + + Boolean::class.java, java.lang.Boolean::class.java -> data[property] = + jsonObject.getBoolean(property) + + String::class.java, java.lang.String::class.java -> data[property] = + jsonObject.getString(property) + + else -> data[property] = jsonObject.get(property) + } } } } @@ -141,8 +160,10 @@ open class Model( } synchronized(initializationLock) { - data.clear() - data.putAll(newData) + synchronized(data) { + data.clear() + data.putAll(newData) + } } } @@ -436,8 +457,8 @@ open class Model( tag: String = ModelChangeTags.NORMAL, forceChange: Boolean = false, ) { + val oldValue = data[name] synchronized(data) { - val oldValue = data[name] if (oldValue == value && !forceChange) { return } @@ -447,9 +468,8 @@ open class Model( } else if (data.containsKey(name)) { data.remove(name) } - - notifyChanged(name, name, tag, oldValue, value) } + notifyChanged(name, name, tag, oldValue, value) } /** @@ -672,24 +692,28 @@ open class Model( fun toJSON(): JSONObject { synchronized(initializationLock) { val jsonObject = JSONObject() - for (kvp in data) { - when (val value = kvp.value) { - is Model -> { - jsonObject.put(kvp.key, value.toJSON()) - } - is List<*> -> { - val jsonArray = JSONArray() - for (arrayItem in value) { - if (arrayItem is Model) { - jsonArray.put(arrayItem.toJSON()) - } else { - jsonArray.put(arrayItem) + synchronized(data) { + for (kvp in data) { + when (val value = kvp.value) { + is Model -> { + jsonObject.put(kvp.key, value.toJSON()) + } + + is List<*> -> { + val jsonArray = JSONArray() + for (arrayItem in value) { + if (arrayItem is Model) { + jsonArray.put(arrayItem.toJSON()) + } else { + jsonArray.put(arrayItem) + } } + jsonObject.put(kvp.key, jsonArray) + } + + else -> { + jsonObject.put(kvp.key, value) } - jsonObject.put(kvp.key, jsonArray) - } - else -> { - jsonObject.put(kvp.key, value) } } } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/ModelStore.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/ModelStore.kt index e39679d1b0..782ab203a1 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/ModelStore.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/ModelStore.kt @@ -84,7 +84,6 @@ abstract class ModelStore( tag: String, ) { persist() - changeSubscription.fire { it.onModelUpdated(args, tag) } } @@ -102,10 +101,11 @@ abstract class ModelStore( override fun clear(tag: String) { val localList = models.toList() - models.clear() + synchronized(models) { + models.clear() persist() - + } for (item in localList) { // no longer listen for changes to this model item.unsubscribe(this) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/subscriptions/SubscriptionManagerTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/subscriptions/SubscriptionManagerTests.kt index 3b6a5459d5..68375146a9 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/subscriptions/SubscriptionManagerTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/subscriptions/SubscriptionManagerTests.kt @@ -1,12 +1,9 @@ package com.onesignal.user.internal.subscriptions import com.onesignal.common.modeling.IModelChangedHandler -import com.onesignal.common.modeling.ISingletonModelStoreChangeHandler import com.onesignal.common.modeling.ModelChangeTags import com.onesignal.common.modeling.ModelChangedArgs import com.onesignal.core.internal.application.IApplicationService -import com.onesignal.core.internal.config.ConfigModel -import com.onesignal.extensions.RobolectricTest import com.onesignal.mocks.MockHelper import com.onesignal.session.internal.session.ISessionService import com.onesignal.user.internal.subscriptions.impl.SubscriptionManager @@ -26,7 +23,6 @@ import io.mockk.verify import junit.framework.TestCase import org.junit.runner.RunWith -@RobolectricTest @RunWith(KotestTestRunner::class) class SubscriptionManagerTests : FunSpec({ @@ -56,9 +52,10 @@ class SubscriptionManagerTests : FunSpec({ t1.start() t2.start() + // Give some time for the thread to complete the task Thread.sleep(1000) - + // verify if the thread has been successfully terminated TestCase.assertEquals(Thread.State.TERMINATED, t1.state) }