Skip to content

Commit ce54070

Browse files
steve-the-edwardssteve-the-edwards
committed
Fix Conflate Stale to Work with Short Circuit
Short circuit if there is no changed state while conflating.
1 parent 59301c7 commit ce54070

File tree

2 files changed

+265
-22
lines changed

2 files changed

+265
-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: 244 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,248 @@ 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+
assertEquals(2, renderCount)
1255+
assertEquals(1, childHandlerActionExecuted)
1256+
assertEquals(1, workerActionExecuted)
1257+
}
1258+
}
1259+
}
1260+
1261+
@Test
1262+
fun for_conflate_we_conflate_stacked_actions_into_one_rendering() {
1263+
runtimeTestRunner.runParametrizedTest(
1264+
paramSource = runtimeOptions
1265+
.filter {
1266+
it.first.contains(CONFLATE_STALE_RENDERINGS)
1267+
},
1268+
before = ::setup,
1269+
) { (runtimeConfig: RuntimeConfig, workflowTracer: WorkflowTracer?) ->
1270+
runTest(StandardTestDispatcher()) {
1271+
check(runtimeConfig.contains(CONFLATE_STALE_RENDERINGS))
1272+
1273+
var childHandlerActionExecuted = false
1274+
val trigger = MutableSharedFlow<String>()
1275+
val emitted = mutableListOf<String>()
1276+
1277+
val childWorkflow = Workflow.stateful<String, String, String>(
1278+
initialState = "unchanging state",
1279+
render = { renderState ->
1280+
runningWorker(
1281+
trigger.asWorker()
1282+
) {
1283+
action("") {
1284+
state = it
1285+
setOutput(it)
1286+
}
1287+
}
1288+
renderState
1289+
}
1290+
)
1291+
val workflow = Workflow.stateful<String, String, String>(
1292+
initialState = "unchanging state",
1293+
render = { renderState ->
1294+
renderChild(childWorkflow) { childOutput ->
1295+
action("childHandler") {
1296+
childHandlerActionExecuted = true
1297+
state = childOutput
1298+
}
1299+
}
1300+
runningWorker(
1301+
trigger.asWorker()
1302+
) {
1303+
action("") {
1304+
// Update the rendering in order to show conflation.
1305+
state = "$it+update"
1306+
}
1307+
}
1308+
renderState
1309+
}
1310+
)
1311+
val props = MutableStateFlow(Unit)
1312+
val renderings = renderWorkflowIn(
1313+
workflow = workflow,
1314+
scope = backgroundScope,
1315+
props = props,
1316+
runtimeConfig = runtimeConfig,
1317+
workflowTracer = workflowTracer,
1318+
) {}
1319+
1320+
launch {
1321+
trigger.emit("changed state")
1322+
}
1323+
val collectionJob = launch(UnconfinedTestDispatcher(testScheduler)) {
1324+
// Collect this unconfined so we can get all the renderings faster than actions can
1325+
// be processed.
1326+
renderings.collect {
1327+
emitted += it.rendering
1328+
}
1329+
}
1330+
advanceUntilIdle()
1331+
runCurrent()
1332+
1333+
collectionJob.cancel()
1334+
1335+
// 2 renderings (initial and then the update.) Not *3* renderings.
1336+
assertEquals(2, emitted.size)
1337+
assertEquals("changed state+update", emitted.last())
1338+
assertTrue(childHandlerActionExecuted)
1339+
}
1340+
}
1341+
}
1342+
1343+
@Test
1344+
fun for_conflate_we_do_not_conflate_stacked_actions_into_one_rendering_if_output() {
1345+
runtimeTestRunner.runParametrizedTest(
1346+
paramSource = runtimeOptions
1347+
.filter {
1348+
it.first.contains(CONFLATE_STALE_RENDERINGS)
1349+
},
1350+
before = ::setup,
1351+
) { (runtimeConfig: RuntimeConfig, workflowTracer: WorkflowTracer?) ->
1352+
runTest(StandardTestDispatcher()) {
1353+
check(runtimeConfig.contains(CONFLATE_STALE_RENDERINGS))
1354+
1355+
var childHandlerActionExecuted = false
1356+
val trigger = MutableSharedFlow<String>()
1357+
val emitted = mutableListOf<String>()
1358+
1359+
val childWorkflow = Workflow.stateful<String, String, String>(
1360+
initialState = "unchanging state",
1361+
render = { renderState ->
1362+
runningWorker(
1363+
trigger.asWorker()
1364+
) {
1365+
action("") {
1366+
state = it
1367+
setOutput(it)
1368+
}
1369+
}
1370+
renderState
1371+
}
1372+
)
1373+
val workflow = Workflow.stateful<String, String, String>(
1374+
initialState = "unchanging state",
1375+
render = { renderState ->
1376+
renderChild(childWorkflow) { childOutput ->
1377+
action("childHandler") {
1378+
childHandlerActionExecuted = true
1379+
state = childOutput
1380+
setOutput(childOutput)
1381+
}
1382+
}
1383+
runningWorker(
1384+
trigger.asWorker()
1385+
) {
1386+
action("") {
1387+
// Update the rendering in order to show conflation.
1388+
state = "$it+update"
1389+
setOutput("$it+update")
1390+
}
1391+
}
1392+
renderState
1393+
}
1394+
)
1395+
val props = MutableStateFlow(Unit)
1396+
val renderings = renderWorkflowIn(
1397+
workflow = workflow,
1398+
scope = backgroundScope,
1399+
props = props,
1400+
runtimeConfig = runtimeConfig,
1401+
workflowTracer = workflowTracer,
1402+
) {}
1403+
1404+
launch {
1405+
trigger.emit("changed state")
1406+
}
1407+
val collectionJob = launch(UnconfinedTestDispatcher(testScheduler)) {
1408+
// Collect this unconfined so we can get all the renderings faster than actions can
1409+
// be processed.
1410+
renderings.collect {
1411+
emitted += it.rendering
1412+
}
1413+
}
1414+
advanceUntilIdle()
1415+
runCurrent()
1416+
1417+
collectionJob.cancel()
1418+
1419+
// 3 renderings because each had output.
1420+
assertEquals(3, emitted.size)
1421+
assertEquals("changed state+update", emitted.last())
1422+
assertTrue(childHandlerActionExecuted)
1423+
}
1424+
}
1425+
}
1426+
11831427
private class ExpectedException : RuntimeException()
11841428

11851429
private fun <T1, T2> cartesianProduct(

0 commit comments

Comments
 (0)