Skip to content

Commit a1d2b3b

Browse files
Fix workflow freezing thread safety.
1 parent daff58b commit a1d2b3b

File tree

6 files changed

+92
-6
lines changed

6 files changed

+92
-6
lines changed

workflow-runtime/src/appleMain/kotlin/com/squareup/workflow1/internal/Synchronization.apple.kt

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,12 @@
11
package com.squareup.workflow1.internal
22

3+
import kotlinx.cinterop.CPointer
4+
import kotlinx.cinterop.ExperimentalForeignApi
5+
import platform.Foundation.NSCopyingProtocol
36
import platform.Foundation.NSLock
7+
import platform.Foundation.NSThread
8+
import platform.Foundation.NSZone
9+
import platform.darwin.NSObject
410

511
internal actual typealias Lock = NSLock
612

@@ -12,3 +18,35 @@ internal actual inline fun <R> Lock.withLock(block: () -> R): R {
1218
unlock()
1319
}
1420
}
21+
22+
/**
23+
* Implementation of [ThreadLocal] that works in a similar way to Java's, based on a thread-specific
24+
* map/dictionary.
25+
*/
26+
internal actual class ThreadLocal<T>(
27+
private val initialValue: () -> T
28+
) : NSObject(), NSCopyingProtocol {
29+
30+
private val threadDictionary
31+
get() = NSThread.currentThread().threadDictionary
32+
33+
actual fun get(): T {
34+
@Suppress("UNCHECKED_CAST")
35+
return (threadDictionary.objectForKey(aKey = this) as T?)
36+
?: initialValue().also(::set)
37+
}
38+
39+
actual fun set(value: T) {
40+
threadDictionary.setObject(value, forKey = this)
41+
}
42+
43+
/**
44+
* [Docs](https://developer.apple.com/documentation/foundation/nscopying/copy(with:)) say [zone]
45+
* is unused.
46+
*/
47+
@OptIn(ExperimentalForeignApi::class)
48+
override fun copyWithZone(zone: CPointer<NSZone>?): Any = this
49+
}
50+
51+
internal actual fun <T> threadLocalOf(initialValue: () -> T): ThreadLocal<T> =
52+
ThreadLocal(initialValue)

workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/RealRenderContext.kt

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ internal class RealRenderContext<PropsT, StateT, OutputT>(
5252
* - prevent modifications to this object after [freeze] is called.
5353
* - prevent sending to sinks before render returns.
5454
*/
55-
private var frozen = false
55+
private var frozen by threadLocalOf { true }
5656

5757
override val actionSink: Sink<WorkflowAction<PropsT, StateT, OutputT>> get() = this
5858

@@ -100,7 +100,7 @@ internal class RealRenderContext<PropsT, StateT, OutputT>(
100100
* Freezes this context so that any further calls to this context will throw.
101101
*/
102102
fun freeze() {
103-
checkNotFrozen("freeze") { "freeze" }
103+
// checkNotFrozen("freeze") { "freeze" }
104104
frozen = true
105105
}
106106

@@ -117,7 +117,10 @@ internal class RealRenderContext<PropsT, StateT, OutputT>(
117117
*
118118
* @see checkWithKey
119119
*/
120-
private inline fun checkNotFrozen(stackTraceKey: Any, lazyMessage: () -> Any) =
120+
private inline fun checkNotFrozen(
121+
stackTraceKey: Any,
122+
lazyMessage: () -> Any
123+
) =
121124
checkWithKey(!frozen, stackTraceKey) {
122125
"RenderContext cannot be used after render method returns: ${lazyMessage()}"
123126
}
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,29 @@
11
package com.squareup.workflow1.internal
22

3+
import kotlin.reflect.KProperty
4+
35
internal expect class Lock()
46

57
internal expect inline fun <R> Lock.withLock(block: () -> R): R
8+
9+
internal expect class ThreadLocal<T> {
10+
fun get(): T
11+
fun set(value: T)
12+
}
13+
14+
internal expect fun <T> threadLocalOf(initialValue: () -> T): ThreadLocal<T>
15+
16+
@Suppress("NOTHING_TO_INLINE")
17+
internal inline operator fun <T> ThreadLocal<T>.getValue(
18+
receiver: Any?,
19+
property: KProperty<*>
20+
): T = get()
21+
22+
@Suppress("NOTHING_TO_INLINE")
23+
internal inline operator fun <T> ThreadLocal<T>.setValue(
24+
receiver: Any?,
25+
property: KProperty<*>,
26+
value: T
27+
) {
28+
set(value)
29+
}

workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/internal/RealRenderContextTest.kt

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -220,8 +220,10 @@ internal class RealRenderContextTest {
220220

221221
val child = Workflow.stateless<Unit, Nothing, Unit> { fail() }
222222
assertFailsWith<IllegalStateException> { context.renderChild(child) }
223-
assertFailsWith<IllegalStateException> { context.freeze() }
224223
assertFailsWith<IllegalStateException> { context.remember("key", typeOf<String>()) {} }
224+
225+
// Freeze is the exception, it's idempotent and can be called again.
226+
context.freeze()
225227
}
226228

227229
private fun createdPoisonedContext(): RealRenderContext<String, String, String> {
@@ -234,7 +236,9 @@ internal class RealRenderContextTest {
234236
eventActionsChannel,
235237
workflowTracer = null,
236238
runtimeConfig = emptySet(),
237-
)
239+
).apply {
240+
unfreeze()
241+
}
238242
}
239243

240244
private fun createTestContext(): RealRenderContext<String, String, String> {
@@ -247,6 +251,8 @@ internal class RealRenderContextTest {
247251
eventActionsChannel,
248252
workflowTracer = null,
249253
runtimeConfig = emptySet(),
250-
)
254+
).apply {
255+
unfreeze()
256+
}
251257
}
252258
}

workflow-runtime/src/jsMain/kotlin/com/squareup/workflow1/internal/Synchronization.js.kt

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,3 +5,13 @@ package com.squareup.workflow1.internal
55
internal actual typealias Lock = Any
66

77
internal actual inline fun <R> Lock.withLock(block: () -> R): R = block()
8+
9+
internal actual class ThreadLocal<T>(private var value: T) {
10+
actual fun get(): T = value
11+
actual fun set(value: T) {
12+
this.value = value
13+
}
14+
}
15+
16+
internal actual fun <T> threadLocalOf(initialValue: () -> T): ThreadLocal<T> =
17+
ThreadLocal(initialValue())

workflow-runtime/src/jvmMain/kotlin/com/squareup/workflow1/internal/Synchronization.jvm.kt

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,3 +3,8 @@ package com.squareup.workflow1.internal
33
internal actual typealias Lock = Any
44

55
internal actual inline fun <R> Lock.withLock(block: () -> R): R = synchronized(this, block)
6+
7+
internal actual typealias ThreadLocal<T> = java.lang.ThreadLocal<T>
8+
9+
internal actual fun <T> threadLocalOf(initialValue: () -> T): ThreadLocal<T> =
10+
ThreadLocal.withInitial(initialValue)

0 commit comments

Comments
 (0)