Skip to content

Fix launchMolecule startup cancellation race #761

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

Open
jmalpasuto wants to merge 6 commits into
base: trunk
Choose a base branch
from
Open

Fix launchMolecule startup cancellation race #761

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6ec6833
Fix launchMolecule startup cancellation race
jmalpasuto May 20, 2026
54a47a8
Fix launchMolecule startup cancellation cleanup
jmalpasuto May 26, 2026
3e82207
Move isActive check earlier
jmalpasuto May 29, 2026
ed2e853
Allow initial Molecule value before startup cancellation
jmalpasuto May 29, 2026
d22e552
Simplify launchMolecule startup cleanup
jmalpasuto Jul 14, 2026
e9e70b9
Assert initial Molecule value synchronously
jmalpasuto Jul 15, 2026
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
Changed:
- The deprecated `iosX64`, `macosX64`, `tvosX64`, and `watchosX64` targets have been removed.

Fixed:
- Cancelling a coroutine context during Molecule startup no longer causes `launchMolecule` to call `setContent` on a disposed composition.


## [2.2.0] - 2025-09-24
[2.2.0]: https://github.com/cashapp/molecule/releases/tag/2.2.0
Expand Down
45 changes: 25 additions & 20 deletions molecule-runtime/src/commonMain/kotlin/app/cash/molecule/molecule.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,34 +231,39 @@ public fun <T> CoroutineScope.launchMolecule(
val composition = Composition(UnitApplier, recomposer)

var snapshotHandle: ObserverHandle? = null
launch(finalContext, start = UNDISPATCHED) {
try {
recomposer.runRecomposeAndApplyChanges()
} finally {
composition.dispose()
snapshotHandle?.dispose()
}
Comment on lines -234 to -240

@FletchMcKee FletchMcKee May 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's been a while since I looked through the Recomposer code, but I think this try/finally block needs to wrap recomposer.runRecomposeAndApplyChanges(). With this change if something throws in the when (snapshotNotifier) block, the snapshotHandle would leak and the composition wouldn't be disposed.

I think all you need to do is add the finalContext.ensureActive().

@jingibus jingibus May 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

While I dislike silent failure as a failure mode, checking isActive and returning early would be idiomatic with how other launch methods behave on cancelled scopes. That is what I would do.

@FletchMcKee FletchMcKee May 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yep, looking at some of the tests I wonder if there’s another issue. I remember discovering this snapshotHandle leak when working on #596 as I would see the errorDelayed tests handles leaking into other tests. That’s why I added this check to any of the tests that threw an exception:

// Verify `runRecomposeAndApplyChanges` is no longer active.
assertThat(job.isCompleted).isTrue()

But this didn’t work for the errorImmediately tests and I thought it wasn’t an issue because the exception was thrown in the initial composition so it never had a recomposition. For these tests I added job.cancelAndJoin() at the end and thought it was enough but I wonder if this is wrong.

If an exception occurs in the initial composition, I don’t know if anything ever cleans up the snapshotHandle and runRecomposeAndApplyChanges ends up hanging. Maybe something like this is also needed?

if (!finalContext.isActive) return 
try {
  composition.setContent {
    emitter(body())
  }
} catch (exception: Exception) {
  finalContext.cancel()
  throw exception 
}

There’s gotta be a cleaner way and I may be overthinking this, maybe the job completes on the next frame clock tick, but some food for thought.

@jmalpasuto jmalpasuto May 26, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for reviewing! I put up a follow up fix that hopefully addresses these issues... Let me know what you think!

val recomposerJob = launch(finalContext, start = UNDISPATCHED) {
recomposer.runRecomposeAndApplyChanges()
}

when (snapshotNotifier) {
SnapshotNotifier.External -> {}
try {
when (snapshotNotifier) {
SnapshotNotifier.External -> {}

SnapshotNotifier.WhileActive -> {
var applyScheduled = false
snapshotHandle = Snapshot.registerGlobalWriteObserver {
if (!applyScheduled) {
applyScheduled = true
launch(finalContext) {
applyScheduled = false
Snapshot.sendApplyNotifications()
SnapshotNotifier.WhileActive -> {
var applyScheduled = false
snapshotHandle = Snapshot.registerGlobalWriteObserver {
if (!applyScheduled) {
applyScheduled = true
launch(finalContext) {
applyScheduled = false
Snapshot.sendApplyNotifications()
}
}
}
}
}
}

composition.setContent {
emitter(body())
composition.setContent {
emitter(body())
}
} catch (throwable: Throwable) {
recomposer.cancel()

@jingibus jingibus Jul 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The recomposer, or the recomposerJob?

@jmalpasuto jmalpasuto Jul 15, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The recomposer. Canceling only the job could leave effects running whereas canceling the recomposer shuts down both.

throw throwable
} finally {
recomposerJob.invokeOnCompletion {
composition.dispose()
snapshotHandle?.dispose()
}
}
}

Expand Down
26 changes: 25 additions & 1 deletion molecule-runtime/src/commonTest/kotlin/app/cash/molecule/MoleculeStateFlowTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import app.cash.molecule.RecompositionMode.ContextClock
import app.cash.molecule.RecompositionMode.Immediate
import assertk.assertFailure
import assertk.assertThat
import assertk.assertions.isEmpty
import assertk.assertions.isEqualTo
import assertk.assertions.isSameInstanceAs
import assertk.assertions.isTrue
Expand Down Expand Up @@ -99,10 +100,33 @@ class MoleculeStateFlowTest {
}
}.isSameInstanceAs(runtimeException)

// This exception is processed in `composeInitial` and not `runRecomposeAndApplyChanges`, so the job is still active.
runCurrent()
assertThat(job.children.toList()).isEmpty()
job.cancelAndJoin()
}

@Test fun cancelledContextComposesInitialValueBeforeStopping() = runTest {
for (mode in listOf(ContextClock, Immediate)) {
val job = Job()
val scope = CoroutineScope(coroutineContext + BroadcastFrameClock())

@jingibus jingibus Jul 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Breaks structured concurrency, which is bad — breaking SC can lead to unowned jobs, and concurrency leaks.

Instead, do something like:

(this + BroadcastFrameClock()).launchMolecule<Int>(mode, context = job)

If that hangs, well — good! More coverage from the test.

@jingibus jingibus Jul 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(This is contra the style used in other tests, I now realize, so - Jake will probably be fine with it)

@jmalpasuto jmalpasuto Jul 15, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good call checking this. The scope keeps runTest’s existing job, so no new unowned job is created.

@jingibus jingibus Jul 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ahh, yeah, I missed that. I just automatically scan for CoroutineScope as being problematic. You are right!

var effectRan = false

job.cancel()

val flow = scope.launchMolecule<Int>(mode, context = job) {
LaunchedEffect(Unit) {
effectRan = true
}
1
}
runCurrent()

assertThat(flow.value).isEqualTo(1)

@jingibus jingibus Jul 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Move this before runCurrent() - this should synchronously be the case immediately after launchMolecule.

@jmalpasuto jmalpasuto Jul 15, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Updated!

assertThat(effectRan).isEqualTo(false)
assertThat(job.children.toList()).isEmpty()
}
}

@Test fun errorDelayed() = runTest {
val job = Job()
val clock = BroadcastFrameClock()
Expand Down
25 changes: 23 additions & 2 deletions molecule-runtime/src/commonTest/kotlin/app/cash/molecule/MoleculeTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import app.cash.molecule.SnapshotNotifier.External
import app.cash.molecule.SnapshotNotifier.WhileActive
import assertk.assertFailure
import assertk.assertThat
import assertk.assertions.isEmpty
import assertk.assertions.isEqualTo
import assertk.assertions.isNotSameInstanceAs
import assertk.assertions.isSameInstanceAs
Expand Down Expand Up @@ -110,10 +111,29 @@ class MoleculeTest {
}
}.isSameInstanceAs(runtimeException)

// This exception is processed in `composeInitial` and not `runRecomposeAndApplyChanges`, so the job is still active.
runCurrent()
assertThat(job.children.toList()).isEmpty()
job.cancelAndJoin()
}

@Test fun cancelledContextComposesInitialValueBeforeStopping() = runTest {
for (mode in listOf(ContextClock, Immediate)) {
val job = Job()
val scope = CoroutineScope(coroutineContext + BroadcastFrameClock())

@jingibus jingibus Jul 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same feedback here.

var value = 0

job.cancel()

scope.launchMolecule<Int>(mode, emitter = { value = it }, context = job) {
1
}
runCurrent()

assertThat(value).isEqualTo(1)
assertThat(job.children.toList()).isEmpty()
}
}

@Test fun errorDelayed() = runTest {
val job = Job()
val clock = BroadcastFrameClock()
Expand Down Expand Up @@ -184,7 +204,8 @@ class MoleculeTest {
}
}.isSameInstanceAs(runtimeException)

// This exception is processed in `composeInitial` and not `runRecomposeAndApplyChanges`, so the job is still active.
runCurrent()
assertThat(job.children.toList()).isEmpty()
job.cancelAndJoin()
}

Expand Down