Skip to content

Commit c5456cd

Browse files
Shahroz16Shahroz16
authored
chore: added thread safety in the inapp message controller (#570)
1 parent c4090cf commit c5456cd

File tree

5 files changed

+267
-22
lines changed

5 files changed

+267
-22
lines changed

‎messaginginapp/api/messaginginapp.api‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -419,3 +419,7 @@ public final class io/customer/messaginginapp/ui/InlineInAppMessageView : androi
419419
public final fun setProgressTint (I)V
420420
}
421421

422+
public abstract interface annotation class io/customer/messaginginapp/ui/controller/ThreadSafeProperty : java/lang/annotation/Annotation {
423+
public abstract fun reason ()Ljava/lang/String;
424+
}
425+

‎messaginginapp/src/main/java/io/customer/messaginginapp/ui/controller/InAppMessageViewController.kt‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,21 +26,22 @@ internal abstract class InAppMessageViewController<ViewCallback : InAppMessageVi
2626
protected val type: String,
2727
protected val platformDelegate: InAppPlatformDelegate,
2828
val viewDelegate: InAppHostViewDelegate
29-
) : EngineWebViewListener {
29+
) : ThreadSafeController(), EngineWebViewListener {
3030
private val logger = SDKComponent.logger
3131
protected val inAppMessagingManager = SDKComponent.inAppMessagingManager
3232
protected var shouldDispatchDisplayEvent: Boolean = true
3333

3434
@VisibleForTesting(otherwise = VisibleForTesting.PROTECTED)
35-
var engineWebViewDelegate: EngineWebViewDelegate? = null
35+
var engineWebViewDelegate: EngineWebViewDelegate? by threadSafe()
3636

3737
@VisibleForTesting(otherwise = VisibleForTesting.PROTECTED)
38-
var currentMessage: Message? = null
38+
@ThreadSafeProperty("Accessed from UI thread writes and background WebView callbacks")
39+
var currentMessage: Message? by threadSafe()
3940

40-
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
41-
var currentRoute: String? = null
41+
@ThreadSafeProperty("Accessed from UI and background threads during route changes")
42+
var currentRoute: String? by threadSafe()
4243

43-
var viewCallback: ViewCallback? = null
44+
var viewCallback: ViewCallback? by threadSafe()
4445

4546
/**
4647
* Listener to handle action clicks from inline in-app messages.

‎messaginginapp/src/main/java/io/customer/messaginginapp/ui/controller/InlineInAppMessageViewController.kt‎

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -31,22 +31,20 @@ internal constructor(
3131
elapsedTimer = ElapsedTimer()
3232
)
3333

34-
internal var elementId: String? = null
35-
set(value) {
36-
val oldValue = field
37-
field = value
38-
if (oldValue != value) {
39-
logViewEvent("Element ID changed from $oldValue to $value")
40-
// Fetch current state to ensure we have latest message for given element ID
41-
onElementIdChanged()
42-
}
34+
@ThreadSafeProperty("Accessed from UI thread and background state subscriptions")
35+
internal var elementId: String? by threadSafeWithNotification { old, new ->
36+
if (old != new) {
37+
logViewEvent("Element ID changed from $old to $new")
38+
// Fetch current state to ensure we have latest message for given element ID
39+
onElementIdChanged()
4340
}
41+
}
4442

45-
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
46-
internal var contentWidthInDp: Double? = null
43+
@ThreadSafeProperty("Accessed during layout changes and size callbacks")
44+
internal var contentWidthInDp: Double? by threadSafe()
4745

48-
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
49-
internal var contentHeightInDp: Double? = null
46+
@ThreadSafeProperty("Accessed during layout changes and size callbacks")
47+
internal var contentHeightInDp: Double? by threadSafe()
5048

5149
init {
5250
viewDelegate.isVisible = false
@@ -56,7 +54,7 @@ internal constructor(
5654
private fun subscribeToStore() {
5755
inAppMessagingManager.subscribeToState(
5856
areEquivalent = { oldState, newState ->
59-
val viewElementId = this.elementId ?: return@subscribeToState true
57+
val viewElementId = elementId ?: return@subscribeToState true
6058

6159
val oldMessage = oldState.queuedInlineMessagesState.getMessage(viewElementId)
6260
val newMessage = newState.queuedInlineMessagesState.getMessage(viewElementId)
@@ -76,9 +74,10 @@ internal constructor(
7674

7775
internal fun onDetachedFromWindow() {
7876
val message = currentMessage ?: return
77+
val currentElementId = elementId
7978

8079
if (platformDelegate.shouldDestroyViewOnDetach()) {
81-
logViewEvent("View detached from window, dismissing inline message view for $elementId ")
80+
logViewEvent("View detached from window, dismissing inline message view for $currentElementId ")
8281
inAppMessagingManager.dispatch(
8382
InAppMessagingAction.DismissMessage(
8483
message = message,
@@ -87,7 +86,7 @@ internal constructor(
8786
)
8887
)
8988
} else {
90-
logViewEvent("Skipping destroy for inline message view for $elementId — likely config change or temporary detach")
89+
logViewEvent("Skipping destroy for inline message view for $currentElementId — likely config change or temporary detach")
9190
}
9291
}
9392

‎messaginginapp/src/main/java/io/customer/messaginginapp/ui/controller/ThreadSafeController.kt‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
package io.customer.messaginginapp.ui.controller
2+
3+
import kotlin.properties.ReadWriteProperty
4+
import kotlin.reflect.KProperty
5+
6+
/**
7+
* Base class that enforces thread-safe property patterns for controller classes.
8+
* All controller properties that might be accessed cross-thread should use these delegates.
9+
*/
10+
internal abstract class ThreadSafeController {
11+
12+
/**
13+
* Property delegate that automatically makes properties thread-safe with @Volatile.
14+
* Use this for simple properties that don't need complex state management.
15+
*
16+
* Usage:
17+
* var elementId: String? by threadSafe()
18+
* var isVisible: Boolean by threadSafe(false)
19+
*/
20+
protected fun <T> threadSafe(initialValue: T? = null): ReadWriteProperty<Any?, T?> {
21+
return VolatileProperty(initialValue)
22+
}
23+
24+
/**
25+
* Property delegate with change notifications for properties that trigger actions.
26+
*
27+
* Usage:
28+
* var elementId: String? by threadSafeWithNotification { old, new ->
29+
* if (old != new) onElementIdChanged(old, new)
30+
* }
31+
*/
32+
protected fun <T> threadSafeWithNotification(
33+
initialValue: T? = null,
34+
onChange: (old: T?, new: T?) -> Unit
35+
): ReadWriteProperty<Any?, T?> {
36+
return VolatilePropertyWithNotification(initialValue, onChange)
37+
}
38+
}
39+
40+
/**
41+
* Thread-safe property delegate using @Volatile
42+
*/
43+
private class VolatileProperty<T>(initialValue: T?) : ReadWriteProperty<Any?, T?> {
44+
@Volatile
45+
private var value: T? = initialValue
46+
47+
override fun getValue(thisRef: Any?, property: KProperty<*>): T? = value
48+
override fun setValue(thisRef: Any?, property: KProperty<*>, value: T?) {
49+
this.value = value
50+
}
51+
}
52+
53+
/**
54+
* Thread-safe property delegate with change notifications
55+
*/
56+
private class VolatilePropertyWithNotification<T>(
57+
initialValue: T?,
58+
private val onChange: (old: T?, new: T?) -> Unit
59+
) : ReadWriteProperty<Any?, T?> {
60+
@Volatile
61+
private var value: T? = initialValue
62+
63+
override fun getValue(thisRef: Any?, property: KProperty<*>): T? = value
64+
override fun setValue(thisRef: Any?, property: KProperty<*>, value: T?) {
65+
val oldValue = this.value
66+
this.value = value
67+
onChange(oldValue, value)
68+
}
69+
}
70+
71+
/**
72+
* Annotation to mark properties that should be thread-safe
73+
* This helps with code reviews and static analysis
74+
*/
75+
@Target(AnnotationTarget.PROPERTY)
76+
@Retention(AnnotationRetention.SOURCE)
77+
annotation class ThreadSafeProperty(
78+
val reason: String = "Accessed from multiple threads"
79+
)

‎messaginginapp/src/test/java/io/customer/messaginginapp/ui/controller/ThreadSafeControllerTest.kt‎

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,162 @@
1+
package io.customer.messaginginapp.ui.controller
2+
3+
import io.customer.messaginginapp.testutils.core.JUnitTest
4+
import java.util.concurrent.CountDownLatch
5+
import java.util.concurrent.TimeUnit
6+
import java.util.concurrent.atomic.AtomicInteger
7+
import java.util.concurrent.atomic.AtomicReference
8+
import kotlin.concurrent.thread
9+
import org.amshove.kluent.shouldBeEqualTo
10+
import org.amshove.kluent.shouldBeNull
11+
import org.junit.jupiter.api.Test
12+
13+
class ThreadSafeControllerTest : JUnitTest() {
14+
15+
private class TestController : ThreadSafeController() {
16+
var simpleProperty: String? by threadSafe()
17+
var propertyWithDefault: Boolean? by threadSafe(false)
18+
var notificationProperty: String? by threadSafeWithNotification { old, new ->
19+
notificationCallCount.incrementAndGet()
20+
lastOldValue.set(old)
21+
lastNewValue.set(new)
22+
}
23+
val notificationCallCount = AtomicInteger(0)
24+
val lastOldValue = AtomicReference<String?>()
25+
val lastNewValue = AtomicReference<String?>()
26+
}
27+
28+
@Test
29+
fun threadSafe_givenDefaultInitialization_expectNullValue() {
30+
val controller = TestController()
31+
32+
controller.simpleProperty.shouldBeNull()
33+
}
34+
35+
@Test
36+
fun threadSafe_givenDefaultValue_expectDefaultReturned() {
37+
val controller = TestController()
38+
39+
controller.propertyWithDefault shouldBeEqualTo false
40+
}
41+
42+
@Test
43+
fun threadSafe_givenValueSet_expectValueReturned() {
44+
val controller = TestController()
45+
val testValue = "test-value"
46+
47+
controller.simpleProperty = testValue
48+
49+
controller.simpleProperty shouldBeEqualTo testValue
50+
}
51+
52+
@Test
53+
fun threadSafe_givenValueSetToNull_expectNullReturned() {
54+
val controller = TestController()
55+
controller.simpleProperty = "initial"
56+
57+
controller.simpleProperty = null
58+
59+
controller.simpleProperty.shouldBeNull()
60+
}
61+
62+
@Test
63+
fun threadSafeWithNotification_givenValueSet_expectNotificationCalled() {
64+
val controller = TestController()
65+
val testValue = "test-value"
66+
67+
controller.notificationProperty = testValue
68+
69+
controller.notificationCallCount.get() shouldBeEqualTo 1
70+
controller.lastOldValue.get().shouldBeNull()
71+
controller.lastNewValue.get() shouldBeEqualTo testValue
72+
}
73+
74+
@Test
75+
fun threadSafeWithNotification_givenValueChanged_expectNotificationWithOldValue() {
76+
val controller = TestController()
77+
val oldValue = "old-value"
78+
val newValue = "new-value"
79+
controller.notificationProperty = oldValue
80+
controller.notificationCallCount.set(0) // Reset counter
81+
82+
controller.notificationProperty = newValue
83+
84+
controller.notificationCallCount.get() shouldBeEqualTo 1
85+
controller.lastOldValue.get() shouldBeEqualTo oldValue
86+
controller.lastNewValue.get() shouldBeEqualTo newValue
87+
}
88+
89+
@Test
90+
fun threadSafe_givenConcurrentAccess_expectNoDataCorruption() {
91+
val controller = TestController()
92+
val threadCount = 10
93+
val iterationsPerThread = 100
94+
val latch = CountDownLatch(threadCount)
95+
val completionLatch = CountDownLatch(threadCount)
96+
val values = mutableSetOf<String>()
97+
98+
// Create multiple threads that set different values
99+
repeat(threadCount) { threadIndex ->
100+
thread {
101+
latch.countDown()
102+
latch.await() // Wait for all threads to be ready
103+
104+
repeat(iterationsPerThread) { iteration ->
105+
val value = "thread-$threadIndex-iteration-$iteration"
106+
controller.simpleProperty = value
107+
synchronized(values) {
108+
values.add(value)
109+
}
110+
// Small delay to increase chance of interleaving
111+
Thread.sleep(1)
112+
}
113+
completionLatch.countDown()
114+
}
115+
}
116+
117+
// Wait for all threads to complete
118+
completionLatch.await(5, TimeUnit.SECONDS)
119+
120+
// The final value should be one of the values that was set
121+
val finalValue = controller.simpleProperty
122+
if (finalValue != null) {
123+
values.contains(finalValue) shouldBeEqualTo true
124+
}
125+
}
126+
127+
@Test
128+
fun threadSafe_givenConcurrentReadWrite_expectConsistentReads() {
129+
val controller = TestController()
130+
val readCount = 1000
131+
val writeValue = "consistent-value"
132+
val readValues = mutableListOf<String?>()
133+
val readLatch = CountDownLatch(1)
134+
val writeLatch = CountDownLatch(1)
135+
val completionLatch = CountDownLatch(2)
136+
137+
// Writer thread
138+
thread {
139+
writeLatch.countDown()
140+
writeLatch.await()
141+
controller.simpleProperty = writeValue
142+
completionLatch.countDown()
143+
}
144+
145+
// Reader thread
146+
thread {
147+
readLatch.countDown()
148+
readLatch.await()
149+
repeat(readCount) {
150+
readValues.add(controller.simpleProperty)
151+
}
152+
completionLatch.countDown()
153+
}
154+
155+
completionLatch.await(5, TimeUnit.SECONDS)
156+
157+
// All non-null values should be the write value (due to @Volatile visibility)
158+
readValues.filterNotNull().forEach { value ->
159+
value shouldBeEqualTo writeValue
160+
}
161+
}
162+
}

0 commit comments

Comments
 (0)