Skip to content

Commit 42d19e0

Browse files
jinliu9508jinliu9508
authored
Merge pull request #1944 from OneSignal/deadlock-between-model-and-subscriber
Deadlock and concurrent modification related to Model.data
2 parents 52987ab + 4f0d3d1 commit 42d19e0

File tree

3 files changed

+173
-51
lines changed

3 files changed

+173
-51
lines changed

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/Model.kt‎

Lines changed: 44 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,6 @@ open class Model(
5151
* specified, must also specify [_parentModel]
5252
*/
5353
private val _parentProperty: String? = null,
54-
private val initializationLock: Any = Any(),
5554
) : IEventNotifier<IModelChangedHandler> {
5655
/**
5756
* A unique identifier for this model.
@@ -80,33 +79,43 @@ open class Model(
8079
* @param jsonObject The [JSONObject] to initialize this model from.
8180
*/
8281
fun initializeFromJson(jsonObject: JSONObject) {
83-
data.clear()
84-
for (property in jsonObject.keys()) {
85-
val jsonValue = jsonObject.get(property)
86-
if (jsonValue is JSONObject) {
87-
val childModel = createModelForProperty(property, jsonValue)
88-
if (childModel != null) {
89-
data[property] = childModel
90-
}
91-
} else if (jsonValue is JSONArray) {
92-
val listOfItems = createListForProperty(property, jsonValue)
93-
if (listOfItems != null) {
94-
data[property] = listOfItems
95-
}
96-
} else {
97-
val method = this.javaClass.methods.firstOrNull { it.returnType != Void::class.java && it.name.contains(property, true) }
98-
99-
if (method == null) {
100-
data[property] = jsonObject.get(property)
82+
synchronized(data) {
83+
data.clear()
84+
for (property in jsonObject.keys()) {
85+
val jsonValue = jsonObject.get(property)
86+
if (jsonValue is JSONObject) {
87+
val childModel = createModelForProperty(property, jsonValue)
88+
if (childModel != null) {
89+
data[property] = childModel
90+
}
91+
} else if (jsonValue is JSONArray) {
92+
val listOfItems = createListForProperty(property, jsonValue)
93+
if (listOfItems != null) {
94+
data[property] = listOfItems
95+
}
10196
} else {
102-
when (method.returnType) {
103-
Double::class.java, java.lang.Double::class.java -> data[property] = jsonObject.getDouble(property)
104-
Long::class.java, java.lang.Long::class.java -> data[property] = jsonObject.getLong(property)
105-
Float::class.java, java.lang.Float::class.java -> data[property] = jsonObject.getDouble(property).toFloat()
106-
Int::class.java, java.lang.Integer::class.java -> data[property] = jsonObject.getInt(property)
107-
Boolean::class.java, java.lang.Boolean::class.java -> data[property] = jsonObject.getBoolean(property)
108-
String::class.java, java.lang.String::class.java -> data[property] = jsonObject.getString(property)
109-
else -> data[property] = jsonObject.get(property)
97+
val method =
98+
this.javaClass.methods.firstOrNull {
99+
it.returnType !=
100+
Void::class.java &&
101+
it.name.contains(
102+
property,
103+
true,
104+
)
105+
}
106+
107+
if (method == null) {
108+
data[property] = jsonObject.get(property)
109+
} else {
110+
when (method.returnType) {
111+
Double::class.java, java.lang.Double::class.java -> data[property] = jsonObject.getDouble(property)
112+
Long::class.java, java.lang.Long::class.java -> data[property] = jsonObject.getLong(property)
113+
Float::class.java, java.lang.Float::class.java -> data[property] = jsonObject.getDouble(property).toFloat()
114+
Int::class.java, java.lang.Integer::class.java -> data[property] = jsonObject.getInt(property)
115+
Boolean::class.java, java.lang.Boolean::class.java -> data[property] = jsonObject.getBoolean(property)
116+
String::class.java, java.lang.String::class.java -> data[property] = jsonObject.getString(property)
117+
else -> data[property] = jsonObject.get(property)
118+
}
110119
}
111120
}
112121
}
@@ -140,7 +149,7 @@ open class Model(
140149
newData[::id.name] = id
141150
}
142151

143-
synchronized(initializationLock) {
152+
synchronized(data) {
144153
data.clear()
145154
data.putAll(newData)
146155
}
@@ -436,9 +445,8 @@ open class Model(
436445
tag: String = ModelChangeTags.NORMAL,
437446
forceChange: Boolean = false,
438447
) {
448+
val oldValue = data[name]
439449
synchronized(data) {
440-
val oldValue = data[name]
441-
442450
if (oldValue == value && !forceChange) {
443451
return
444452
}
@@ -448,9 +456,8 @@ open class Model(
448456
} else if (data.containsKey(name)) {
449457
data.remove(name)
450458
}
451-
452-
notifyChanged(name, name, tag, oldValue, value)
453459
}
460+
notifyChanged(name, name, tag, oldValue, value)
454461
}
455462

456463
/**
@@ -671,13 +678,14 @@ open class Model(
671678
* @return The resulting [JSONObject].
672679
*/
673680
fun toJSON(): JSONObject {
674-
synchronized(initializationLock) {
675-
val jsonObject = JSONObject()
681+
val jsonObject = JSONObject()
682+
synchronized(data) {
676683
for (kvp in data) {
677684
when (val value = kvp.value) {
678685
is Model -> {
679686
jsonObject.put(kvp.key, value.toJSON())
680687
}
688+
681689
is List<*> -> {
682690
val jsonArray = JSONArray()
683691
for (arrayItem in value) {
@@ -689,13 +697,14 @@ open class Model(
689697
}
690698
jsonObject.put(kvp.key, jsonArray)
691699
}
700+
692701
else -> {
693702
jsonObject.put(kvp.key, value)
694703
}
695704
}
696705
}
697-
return jsonObject
698706
}
707+
return jsonObject
699708
}
700709

701710
override fun subscribe(handler: IModelChangedHandler) = changeNotifier.subscribe(handler)

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/ModelStore.kt‎

Lines changed: 10 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -87,11 +87,8 @@ abstract class ModelStore<TModel>(
8787
args: ModelChangedArgs,
8888
tag: String,
8989
) {
90-
synchronized(models) {
91-
persist()
92-
93-
changeSubscription.fire { it.onModelUpdated(args, tag) }
94-
}
90+
persist()
91+
changeSubscription.fire { it.onModelUpdated(args, tag) }
9592
}
9693

9794
override fun replaceAll(
@@ -108,17 +105,16 @@ abstract class ModelStore<TModel>(
108105
}
109106

110107
override fun clear(tag: String) {
108+
val localList = models.toList()
111109
synchronized(models) {
112-
val localList = models.toList()
113110
models.clear()
114111

115112
persist()
116-
117-
for (item in localList) {
118-
// no longer listen for changes to this model
119-
item.unsubscribe(this)
120-
changeSubscription.fire { it.onModelRemoved(item, tag) }
121-
}
113+
}
114+
for (item in localList) {
115+
// no longer listen for changes to this model
116+
item.unsubscribe(this)
117+
changeSubscription.fire { it.onModelRemoved(item, tag) }
122118
}
123119
}
124120

@@ -138,9 +134,8 @@ abstract class ModelStore<TModel>(
138134
model.subscribe(this)
139135

140136
persist()
141-
142-
changeSubscription.fire { it.onModelAdded(model, tag) }
143137
}
138+
changeSubscription.fire { it.onModelAdded(model, tag) }
144139
}
145140

146141
private fun removeItem(
@@ -154,9 +149,8 @@ abstract class ModelStore<TModel>(
154149
model.unsubscribe(this)
155150

156151
persist()
157-
158-
changeSubscription.fire { it.onModelRemoved(model, tag) }
159152
}
153+
changeSubscription.fire { it.onModelRemoved(model, tag) }
160154
}
161155

162156
protected fun load() {

‎OneSignalSDK/onesignal/core/src/test/java/com/onesignal/common/ModelingTests.kt‎

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
package com.onesignal.common
2+
3+
import com.onesignal.common.events.EventProducer
4+
import com.onesignal.common.modeling.IModelChangedHandler
5+
import com.onesignal.common.modeling.IModelStoreChangeHandler
6+
import com.onesignal.common.modeling.ModelChangedArgs
7+
import com.onesignal.mocks.MockHelper
8+
import com.onesignal.mocks.MockPreferencesService
9+
import com.onesignal.user.internal.subscriptions.SubscriptionModel
10+
import com.onesignal.user.internal.subscriptions.SubscriptionModelStore
11+
import io.kotest.core.spec.style.FunSpec
12+
import io.kotest.runner.junit4.KotestTestRunner
13+
import junit.framework.TestCase
14+
import org.junit.runner.RunWith
15+
16+
@RunWith(KotestTestRunner::class)
17+
class ModelingTests : FunSpec({
18+
19+
test("Deadlock related to Model.setOptAnyProperty") {
20+
// Given
21+
val modelStore = MockHelper.configModelStore()
22+
val model = modelStore.model
23+
24+
val t1 =
25+
Thread {
26+
// acquire "model.data", then trigger the onChanged event
27+
model.setOptAnyProperty("key1", "value1")
28+
}
29+
30+
val t2 =
31+
Thread {
32+
// acquire "model.initializationLock", then wait for "model.data" to be released
33+
model.initializeFromModel("", MockHelper.configModelStore().model)
34+
}
35+
36+
model.subscribe(
37+
object : IModelChangedHandler {
38+
// will be executed in t1
39+
override fun onChanged(
40+
args: ModelChangedArgs,
41+
tag: String,
42+
) {
43+
Thread.sleep(200)
44+
// waiting for "model.initializationLock"
45+
model.toJSON()
46+
}
47+
},
48+
)
49+
50+
t1.start()
51+
t2.start()
52+
53+
// Set 1s timeout for t2 to complete the task
54+
t2.join(1000)
55+
56+
// verify if the thread has been successfully terminated
57+
TestCase.assertEquals(Thread.State.TERMINATED, t2.state)
58+
}
59+
60+
test("Deadlock related to ModelSstore add() or remove()") {
61+
// Given
62+
val modelStore = SubscriptionModelStore(MockPreferencesService())
63+
val event = EventProducer<SubscriptionModel>()
64+
val oldSubscriptionModel = SubscriptionModel()
65+
val newSubscriptionModel = SubscriptionModel()
66+
oldSubscriptionModel.id = "oldModel"
67+
newSubscriptionModel.id = "newModel"
68+
modelStore.add(oldSubscriptionModel)
69+
70+
val t1 =
71+
Thread {
72+
// acquire "ModelStore.models", then trigger the onChanged event
73+
System.out.println("1")
74+
modelStore.add(newSubscriptionModel)
75+
}
76+
77+
val t2 =
78+
Thread {
79+
System.out.println("2")
80+
// acquire "model.data", then wait for "ModelStore.models"
81+
newSubscriptionModel.toJSON()
82+
}
83+
84+
modelStore.subscribe(
85+
object : IModelStoreChangeHandler<SubscriptionModel> {
86+
override fun onModelAdded(
87+
model: SubscriptionModel,
88+
tag: String,
89+
) {
90+
// waiting for "model.data"
91+
model.initializeFromModel("", MockHelper.configModelStore().model)
92+
}
93+
94+
override fun onModelUpdated(
95+
args: ModelChangedArgs,
96+
tag: String,
97+
) {
98+
// left empty in purpose
99+
}
100+
101+
override fun onModelRemoved(
102+
model: SubscriptionModel,
103+
tag: String,
104+
) {
105+
// left empty in purpose
106+
}
107+
},
108+
)
109+
110+
t1.start()
111+
t2.start()
112+
113+
// Set 1s timeout for t2 to complete the task
114+
t2.join(1000)
115+
116+
// verify if the thread has been successfully terminated
117+
TestCase.assertEquals(Thread.State.TERMINATED, t2.state)
118+
}
119+
})

0 commit comments

Comments
 (0)