Skip to content

1338: Clean up caches after WorkflowNode torn down #1339

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 10, 2025
Merged

1338: Clean up caches after WorkflowNode torn down #1339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions workflow-core/api/workflow-core.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,17 @@ public final class com/squareup/workflow1/StatelessWorkflow$RenderContext : com/
public fun runningSideEffect (Ljava/lang/String;Lkotlin/jvm/functions/Function2;)V
}

public final class com/squareup/workflow1/StatelessWorkflow$StatelessAsStatefulWorkflow : com/squareup/workflow1/StatefulWorkflow {
public fun <init> (Lcom/squareup/workflow1/StatelessWorkflow;)V
public final fun clearCache ()V
public synthetic fun initialState (Ljava/lang/Object;Lcom/squareup/workflow1/Snapshot;)Ljava/lang/Object;
public fun initialState (Ljava/lang/Object;Lcom/squareup/workflow1/Snapshot;)V
public synthetic fun render (Ljava/lang/Object;Ljava/lang/Object;Lcom/squareup/workflow1/StatefulWorkflow$RenderContext;)Ljava/lang/Object;
public fun render (Ljava/lang/Object;Lkotlin/Unit;Lcom/squareup/workflow1/StatefulWorkflow$RenderContext;)Ljava/lang/Object;
public synthetic fun snapshotState (Ljava/lang/Object;)Lcom/squareup/workflow1/Snapshot;
public fun snapshotState (Lkotlin/Unit;)Lcom/squareup/workflow1/Snapshot;
}

public final class com/squareup/workflow1/TypedWorker : com/squareup/workflow1/Worker {
public fun <init> (Lkotlin/reflect/KType;Lkotlinx/coroutines/flow/Flow;)V
public fun doesSameWorkAs (Lcom/squareup/workflow1/Worker;)Z
Expand Down
19 changes: 18 additions & 1 deletion workflow-core/src/commonMain/kotlin/com/squareup/workflow1/StatelessWorkflow.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ public abstract class StatelessWorkflow<PropsT, OutputT, out RenderingT> :
* Class type returned by [asStatefulWorkflow].
* See [statefulWorkflow] for the instance.
*/
private inner class StatelessAsStatefulWorkflow :
inner class StatelessAsStatefulWorkflow :
StatefulWorkflow<PropsT, Unit, OutputT, RenderingT>() {

/**
Expand Down Expand Up @@ -268,6 +268,23 @@ public abstract class StatelessWorkflow<PropsT, OutputT, out RenderingT> :
}

override fun snapshotState(state: Unit): Snapshot? = null

/**
* When we are finished with at least one node that holds on to this workflow instance,
* then we clear the cache. The reason we do that every time is that it *might* be the last
Copy link
Collaborator

@zach-klippenstein zach-klippenstein Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just gonna say, this sounds like a problem for refcounting, although if this is a temporary bandaid while we prep for a bigger, more comprehensive fix, who cares.

* node that is caching this instance, and if so, we do not want to leak these cached
* render contexts.
*
* Yes, that means that it might have to be re-created again when this instance is used
* multiple times. The current design for how we get a [StatefulWorkflow] from the
* [StatelessWorkflow] is a failed compromise between performance (caching) and type-safe
* brevity (erasing the `StateT` type from the concerns of [StatelessWorkflow]). It needs
* to be fixed with a bigger re-write (https://github.com/square/workflow-kotlin/issues/1337).
*/
fun clearCache() {
cachedStatelessRenderContext = null
canonicalStatefulRenderContext = null
}
}

private val statefulWorkflow: StatefulWorkflow<PropsT, Unit, OutputT, RenderingT> =
Expand Down
10 changes: 6 additions & 4 deletions workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowNode.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import com.squareup.workflow1.RuntimeConfig
import com.squareup.workflow1.RuntimeConfigOptions
import com.squareup.workflow1.RuntimeConfigOptions.PARTIAL_TREE_RENDERING
import com.squareup.workflow1.StatefulWorkflow
import com.squareup.workflow1.StatelessWorkflow
import com.squareup.workflow1.TreeSnapshot
import com.squareup.workflow1.Workflow
import com.squareup.workflow1.WorkflowAction
Expand Down Expand Up @@ -234,11 +235,12 @@ internal class WorkflowNode<PropsT, StateT, OutputT, RenderingT>(
* after calling this method.
*/
fun cancel(cause: CancellationException? = null) {
// No other cleanup work should be done in this function, since it will only be invoked when
// this workflow is *directly* discarded by its parent (or the host).
// If you need to do something whenever this workflow is torn down, add it to the
// invokeOnCompletion handler for the Job above.
Copy link
Contributor Author

@steve-the-edwards steve-the-edwards Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment was old. We now cancel anytime we stop actively rendering a node.

coroutineContext.cancel(cause)
lastRendering = NullableInitBox()
(
cachedWorkflowInstance as?
StatelessWorkflow<PropsT, OutputT, RenderingT>.StatelessAsStatefulWorkflow
)?.clearCache()
}

/**
Expand Down