From 2c940300a33737b5339e7c6437f9200c91f41be3 Mon Sep 17 00:00:00 2001 From: jinliu9508 Date: Tue, 10 Dec 2024 12:21:39 -0500 Subject: [PATCH 1/4] reproduce ANR caused by operationModelStore.load --- .../core/internal/operations/impl/OperationModelStore.kt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationModelStore.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationModelStore.kt index 1fa25fe631..22146f97ef 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationModelStore.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationModelStore.kt @@ -30,6 +30,7 @@ import org.json.JSONObject internal class OperationModelStore(prefs: IPreferencesService) : ModelStore("operations", prefs) { fun loadOperations() { load() + Logging.debug("OperationModelStore finished loading.") } override fun create(jsonObject: JSONObject?): Operation? { @@ -63,6 +64,7 @@ internal class OperationModelStore(prefs: IPreferencesService) : ModelStore throw Exception("Unrecognized operation: $operationName") } + Thread.sleep(5000) // populate the operation with the data. operation.initializeFromJson(jsonObject) From 11b3564f88836ff0b51263b0da0478e5c8a90604 Mon Sep 17 00:00:00 2001 From: jinliu9508 Date: Tue, 10 Dec 2024 12:24:44 -0500 Subject: [PATCH 2/4] add a test unit to simulate the ANR caused by operationRepo.enqueue while loading is not completed --- .../com/onesignal/common/ModelingTests.kt | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/common/ModelingTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/common/ModelingTests.kt index 1ee72ddb3a..1273976364 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/common/ModelingTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/common/ModelingTests.kt @@ -20,6 +20,41 @@ import java.util.UUID class ModelingTests : FunSpec({ + test("ensure prolonged loading in the background thread does not block insertion in the main thread") { + // Given + val prefs = MockPreferencesService() + val operationModelStore = OperationModelStore(prefs) + + // add an arbitrary operation to the cache + val cachedOperation = LoginUserFromSubscriptionOperation() + val newOperation = LoginUserOperation() + cachedOperation.id = UUID.randomUUID().toString() + val jsonArray = JSONArray() + jsonArray.put(cachedOperation.toJSON()) + prefs.saveString(PreferenceStores.ONESIGNAL, PreferenceOneSignalKeys.MODEL_STORE_PREFIX + "operations", jsonArray.toString()) + + // simulate a background thread to load operations + val backgroundThread = + Thread { + operationModelStore.loadOperations() + } + + val mainThread = + Thread { + operationModelStore.add(newOperation) + } + + backgroundThread.start() + mainThread.start() + + mainThread.join(100) + + // Then + // insertion from the main thread is done without blocking + operationModelStore.list().count() shouldBe 1 + operationModelStore.list().first() shouldBe newOperation + } + test("Deadlock related to Model.setOptAnyProperty") { // Given val modelStore = MockHelper.configModelStore() From fecb45dd0e615261fa9b533bb9469f3d7b0f4a6e Mon Sep 17 00:00:00 2001 From: jinliu9508 Date: Tue, 10 Dec 2024 12:23:01 -0500 Subject: [PATCH 3/4] fix ANR caused by operationRepo.enqueue while loading is not finished --- .../onesignal/common/modeling/ModelStore.kt | 49 ++++++++++--------- 1 file changed, 25 insertions(+), 24 deletions(-) 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 f7c41a410b..8651db5295 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 @@ -169,33 +169,34 @@ abstract class ModelStore( val str = _prefs.getString(PreferenceStores.ONESIGNAL, PreferenceOneSignalKeys.MODEL_STORE_PREFIX + name, "[]") val jsonArray = JSONArray(str) - synchronized(models) { - val shouldRePersist = models.isNotEmpty() - for (index in jsonArray.length() - 1 downTo 0) { - val newModel = create(jsonArray.getJSONObject(index)) ?: continue - - /* - * NOTE: Migration fix for bug introduced in 5.1.12 - * The following check is intended for the operation model store. - * When the call to this method moved out of the operation model store's initializer, - * duplicate operations could be cached. - * See https://github.com/OneSignal/OneSignal-Android-SDK/pull/2099 - */ - val hasExisting = models.any { it.id == newModel.id } - if (hasExisting) { - Logging.debug("ModelStore<$name>: load - operation.id: ${newModel.id} already exists in the store.") - continue - } - models.add(0, newModel) - // listen for changes to this model - newModel.subscribe(this) + val shouldRePersist = models.isNotEmpty() + for (index in jsonArray.length() - 1 downTo 0) { + val newModel = create(jsonArray.getJSONObject(index)) ?: continue + + /* + * NOTE: Migration fix for bug introduced in 5.1.12 + * The following check is intended for the operation model store. + * When the call to this method moved out of the operation model store's initializer, + * duplicate operations could be cached. + * See https://github.com/OneSignal/OneSignal-Android-SDK/pull/2099 + */ + val hasExisting = models.any { it.id == newModel.id } + if (hasExisting) { + Logging.debug("ModelStore<$name>: load - operation.id: ${newModel.id} already exists in the store.") + continue } - hasLoadedFromCache = true - // optimization only: to avoid unnecessary writes - if (shouldRePersist) { - persist() + + synchronized(models) { + models.add(0, newModel) } + // listen for changes to this model + newModel.subscribe(this) + } + hasLoadedFromCache = true + // optimization only: to avoid unnecessary writes + if (shouldRePersist) { + persist() } } From dc8cff6a2857f4e4f47ec79989aef4fb532590ee Mon Sep 17 00:00:00 2001 From: jinliu9508 Date: Tue, 10 Dec 2024 12:59:06 -0500 Subject: [PATCH 4/4] fixup: remove the code that manually simulate a prolonged loading process --- .../core/internal/operations/impl/OperationModelStore.kt | 2 -- 1 file changed, 2 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationModelStore.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationModelStore.kt index 22146f97ef..1fa25fe631 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationModelStore.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationModelStore.kt @@ -30,7 +30,6 @@ import org.json.JSONObject internal class OperationModelStore(prefs: IPreferencesService) : ModelStore("operations", prefs) { fun loadOperations() { load() - Logging.debug("OperationModelStore finished loading.") } override fun create(jsonObject: JSONObject?): Operation? { @@ -64,7 +63,6 @@ internal class OperationModelStore(prefs: IPreferencesService) : ModelStore throw Exception("Unrecognized operation: $operationName") } - Thread.sleep(5000) // populate the operation with the data. operation.initializeFromJson(jsonObject)