Skip to content

Commit 56e6ee0

Browse files
steve-the-edwardssteve-the-edwards
authored
Merge pull request #1263 from square/sedwards/fix-conflate
Fix Conflate Stale Rendering Logic for Short Circuit
2 parents e922f71 + 1b1aa06 commit 56e6ee0

File tree

2 files changed

+266
-22
lines changed

2 files changed

+266
-22
lines changed

‎workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RenderWorkflow.kt

Lines changed: 21 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -161,19 +161,12 @@ public fun <PropsT, OutputT, RenderingT> renderWorkflowIn(
161161
}
162162

163163
/**
164-
* If [runtimeConfig] contains [RuntimeConfigOptions.RENDER_ONLY_WHEN_STATE_CHANGES] then
165-
* send any output, but return true which means restart the runtime loop and process another
166-
* action.
164+
* If [runtimeConfig] contains [RuntimeConfigOptions.RENDER_ONLY_WHEN_STATE_CHANGES] and
165+
* we have not changed state, then return true to short circuit the render loop.
167166
*/
168-
suspend fun shortCircuitForUnchangedState(actionResult: ActionProcessingResult): Boolean {
169-
if (runtimeConfig.contains(RENDER_ONLY_WHEN_STATE_CHANGES) &&
167+
fun shouldShortCircuitForUnchangedState(actionResult: ActionProcessingResult): Boolean {
168+
return runtimeConfig.contains(RENDER_ONLY_WHEN_STATE_CHANGES) &&
170169
actionResult is ActionApplied<*> && !actionResult.stateChanged
171-
) {
172-
// Possibly send output and process more actions. No state change so no re-render.
173-
sendOutput(actionResult, onOutput)
174-
return true
175-
}
176-
return false
177170
}
178171

179172
scope.launch {
@@ -183,34 +176,40 @@ public fun <PropsT, OutputT, RenderingT> renderWorkflowIn(
183176
// launched.
184177
var actionResult: ActionProcessingResult = runner.processAction()
185178

186-
if (shortCircuitForUnchangedState(actionResult)) continue
179+
if (shouldShortCircuitForUnchangedState(actionResult)) {
180+
sendOutput(actionResult, onOutput)
181+
continue
182+
}
187183

188184
// After resuming from runner.processAction() our coroutine could now be cancelled, check so
189185
// we don't surprise anyone with an unexpected rendering pass. Show's over, go home.
190186
if (!isActive) return@launch
191187

188+
// Next Render Pass.
192189
var nextRenderAndSnapshot: RenderingAndSnapshot<RenderingT> = runner.nextRendering()
193190

194191
if (runtimeConfig.contains(CONFLATE_STALE_RENDERINGS)) {
195-
// Only null will allow us to continue processing actions and conflating stale renderings.
196-
// If this is not null, then we had an Output and we want to send it with the Rendering
197-
// (stale or not).
198-
while (actionResult is ActionApplied<*> && actionResult.output == null) {
199-
// We have more actions we can process, so this rendering is stale.
192+
while (isActive && actionResult is ActionApplied<*> && actionResult.output == null) {
193+
// We may have more actions we can process, this rendering could be stale.
200194
actionResult = runner.processAction(waitForAnAction = false)
201195

202-
if (!isActive) return@launch
203-
204-
// If no actions processed, then no new rendering needed.
196+
// If no actions processed, then no new rendering needed. Pass on to UI.
205197
if (actionResult == ActionsExhausted) break
206198

199+
// Skip rendering if we had unchanged state, keep draining actions.
200+
if (shouldShortCircuitForUnchangedState(actionResult)) continue
201+
202+
// Make sure the runtime has not been cancelled from runner.processAction()
203+
if (!isActive) return@launch
204+
207205
nextRenderAndSnapshot = runner.nextRendering()
208206
}
209207
}
210208

211-
// Pass on to the UI.
209+
// Pass on the rendering to the UI.
212210
renderingsAndSnapshots.value = nextRenderAndSnapshot
213-
// And emit the Output.
211+
212+
// Emit the Output
214213
sendOutput(actionResult, onOutput)
215214
}
216215
}

‎workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/RenderWorkflowInTest.kt

Lines changed: 245 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import kotlinx.coroutines.CoroutineExceptionHandler
99
import kotlinx.coroutines.ExperimentalCoroutinesApi
1010
import kotlinx.coroutines.cancel
1111
import kotlinx.coroutines.channels.Channel
12+
import kotlinx.coroutines.flow.MutableSharedFlow
1213
import kotlinx.coroutines.flow.MutableStateFlow
1314
import kotlinx.coroutines.flow.StateFlow
1415
import kotlinx.coroutines.flow.map
@@ -21,6 +22,7 @@ import kotlinx.coroutines.test.StandardTestDispatcher
2122
import kotlinx.coroutines.test.TestScope
2223
import kotlinx.coroutines.test.UnconfinedTestDispatcher
2324
import kotlinx.coroutines.test.advanceUntilIdle
25+
import kotlinx.coroutines.test.runCurrent
2426
import kotlinx.coroutines.test.runTest
2527
import okio.ByteString
2628
import kotlin.test.Test
@@ -1180,6 +1182,249 @@ class RenderWorkflowInTest {
11801182
}
11811183
}
11821184

1185+
@Test
1186+
fun for_render_on_change_only_and_conflate_we_drain_action_but_do_not_render_no_state_changed() {
1187+
runtimeTestRunner.runParametrizedTest(
1188+
paramSource = runtimeOptions.filter {
1189+
it.first.contains(RENDER_ONLY_WHEN_STATE_CHANGES) && it.first.contains(
1190+
CONFLATE_STALE_RENDERINGS
1191+
)
1192+
},
1193+
before = ::setup,
1194+
) { (runtimeConfig: RuntimeConfig, workflowTracer: WorkflowTracer?) ->
1195+
runTest(UnconfinedTestDispatcher()) {
1196+
check(runtimeConfig.contains(CONFLATE_STALE_RENDERINGS))
1197+
check(runtimeConfig.contains(RENDER_ONLY_WHEN_STATE_CHANGES))
1198+
1199+
var renderCount = 0
1200+
var childHandlerActionExecuted = 0
1201+
var workerActionExecuted = 0
1202+
val trigger = MutableSharedFlow<String>()
1203+
1204+
val childWorkflow = Workflow.stateful<String, String, String>(
1205+
initialState = "unchanging state",
1206+
render = { renderState ->
1207+
runningWorker(
1208+
trigger.asWorker()
1209+
) {
1210+
action("") {
1211+
state = it
1212+
setOutput(it)
1213+
}
1214+
}
1215+
renderState
1216+
}
1217+
)
1218+
val workflow = Workflow.stateful<String, String, String>(
1219+
initialState = "unchanging state",
1220+
render = { renderState ->
1221+
renderChild(childWorkflow) { childOutput ->
1222+
action("childHandler") {
1223+
childHandlerActionExecuted++
1224+
state = childOutput
1225+
}
1226+
}
1227+
runningWorker(
1228+
trigger.asWorker()
1229+
) {
1230+
action("") {
1231+
workerActionExecuted++
1232+
state = it
1233+
}
1234+
}
1235+
renderState.also {
1236+
renderCount++
1237+
}
1238+
}
1239+
)
1240+
val props = MutableStateFlow(Unit)
1241+
renderWorkflowIn(
1242+
workflow = workflow,
1243+
scope = backgroundScope,
1244+
props = props,
1245+
runtimeConfig = runtimeConfig,
1246+
workflowTracer = workflowTracer,
1247+
) {}
1248+
1249+
launch {
1250+
trigger.emit("changed state")
1251+
}
1252+
advanceUntilIdle()
1253+
1254+
// 2 renderings (initial and then the update.) Not *3* renderings.
1255+
assertEquals(2, renderCount)
1256+
assertEquals(1, childHandlerActionExecuted)
1257+
assertEquals(1, workerActionExecuted)
1258+
}
1259+
}
1260+
}
1261+
1262+
@Test
1263+
fun for_conflate_we_conflate_stacked_actions_into_one_rendering() {
1264+
runtimeTestRunner.runParametrizedTest(
1265+
paramSource = runtimeOptions
1266+
.filter {
1267+
it.first.contains(CONFLATE_STALE_RENDERINGS)
1268+
},
1269+
before = ::setup,
1270+
) { (runtimeConfig: RuntimeConfig, workflowTracer: WorkflowTracer?) ->
1271+
runTest(StandardTestDispatcher()) {
1272+
check(runtimeConfig.contains(CONFLATE_STALE_RENDERINGS))
1273+
1274+
var childHandlerActionExecuted = false
1275+
val trigger = MutableSharedFlow<String>()
1276+
val emitted = mutableListOf<String>()
1277+
1278+
val childWorkflow = Workflow.stateful<String, String, String>(
1279+
initialState = "unchanging state",
1280+
render = { renderState ->
1281+
runningWorker(
1282+
trigger.asWorker()
1283+
) {
1284+
action("") {
1285+
state = it
1286+
setOutput(it)
1287+
}
1288+
}
1289+
renderState
1290+
}
1291+
)
1292+
val workflow = Workflow.stateful<String, String, String>(
1293+
initialState = "unchanging state",
1294+
render = { renderState ->
1295+
renderChild(childWorkflow) { childOutput ->
1296+
action("childHandler") {
1297+
childHandlerActionExecuted = true
1298+
state = childOutput
1299+
}
1300+
}
1301+
runningWorker(
1302+
trigger.asWorker()
1303+
) {
1304+
action("") {
1305+
// Update the rendering in order to show conflation.
1306+
state = "$it+update"
1307+
}
1308+
}
1309+
renderState
1310+
}
1311+
)
1312+
val props = MutableStateFlow(Unit)
1313+
val renderings = renderWorkflowIn(
1314+
workflow = workflow,
1315+
scope = backgroundScope,
1316+
props = props,
1317+
runtimeConfig = runtimeConfig,
1318+
workflowTracer = workflowTracer,
1319+
) {}
1320+
1321+
launch {
1322+
trigger.emit("changed state")
1323+
}
1324+
val collectionJob = launch(UnconfinedTestDispatcher(testScheduler)) {
1325+
// Collect this unconfined so we can get all the renderings faster than actions can
1326+
// be processed.
1327+
renderings.collect {
1328+
emitted += it.rendering
1329+
}
1330+
}
1331+
advanceUntilIdle()
1332+
runCurrent()
1333+
1334+
collectionJob.cancel()
1335+
1336+
// 2 renderings (initial and then the update.) Not *3* renderings.
1337+
assertEquals(2, emitted.size)
1338+
assertEquals("changed state+update", emitted.last())
1339+
assertTrue(childHandlerActionExecuted)
1340+
}
1341+
}
1342+
}
1343+
1344+
@Test
1345+
fun for_conflate_we_do_not_conflate_stacked_actions_into_one_rendering_if_output() {
1346+
runtimeTestRunner.runParametrizedTest(
1347+
paramSource = runtimeOptions
1348+
.filter {
1349+
it.first.contains(CONFLATE_STALE_RENDERINGS)
1350+
},
1351+
before = ::setup,
1352+
) { (runtimeConfig: RuntimeConfig, workflowTracer: WorkflowTracer?) ->
1353+
runTest(StandardTestDispatcher()) {
1354+
check(runtimeConfig.contains(CONFLATE_STALE_RENDERINGS))
1355+
1356+
var childHandlerActionExecuted = false
1357+
val trigger = MutableSharedFlow<String>()
1358+
val emitted = mutableListOf<String>()
1359+
1360+
val childWorkflow = Workflow.stateful<String, String, String>(
1361+
initialState = "unchanging state",
1362+
render = { renderState ->
1363+
runningWorker(
1364+
trigger.asWorker()
1365+
) {
1366+
action("") {
1367+
state = it
1368+
setOutput(it)
1369+
}
1370+
}
1371+
renderState
1372+
}
1373+
)
1374+
val workflow = Workflow.stateful<String, String, String>(
1375+
initialState = "unchanging state",
1376+
render = { renderState ->
1377+
renderChild(childWorkflow) { childOutput ->
1378+
action("childHandler") {
1379+
childHandlerActionExecuted = true
1380+
state = childOutput
1381+
setOutput(childOutput)
1382+
}
1383+
}
1384+
runningWorker(
1385+
trigger.asWorker()
1386+
) {
1387+
action("") {
1388+
// Update the rendering in order to show conflation.
1389+
state = "$it+update"
1390+
setOutput("$it+update")
1391+
}
1392+
}
1393+
renderState
1394+
}
1395+
)
1396+
val props = MutableStateFlow(Unit)
1397+
val renderings = renderWorkflowIn(
1398+
workflow = workflow,
1399+
scope = backgroundScope,
1400+
props = props,
1401+
runtimeConfig = runtimeConfig,
1402+
workflowTracer = workflowTracer,
1403+
) {}
1404+
1405+
launch {
1406+
trigger.emit("changed state")
1407+
}
1408+
val collectionJob = launch(UnconfinedTestDispatcher(testScheduler)) {
1409+
// Collect this unconfined so we can get all the renderings faster than actions can
1410+
// be processed.
1411+
renderings.collect {
1412+
emitted += it.rendering
1413+
}
1414+
}
1415+
advanceUntilIdle()
1416+
runCurrent()
1417+
1418+
collectionJob.cancel()
1419+
1420+
// 3 renderings because each had output.
1421+
assertEquals(3, emitted.size)
1422+
assertEquals("changed state+update", emitted.last())
1423+
assertTrue(childHandlerActionExecuted)
1424+
}
1425+
}
1426+
}
1427+
11831428
private class ExpectedException : RuntimeException()
11841429

11851430
private fun <T1, T2> cartesianProduct(

0 commit comments

Comments
 (0)