Early return ComposeRuntimeError, not fixed in compiler 1.4.4 (Missed recording an endGroup) [274889428]

Fixed

Bug

Status Update

No update yet.

Description

ub...@gmail.com

created issue #1

Mar 23, 2023 04:38PM

Jetpack Compose version: Compose BOM 2023.03.00, Compose Compiler 1.4.4

Jetpack Compose component used: compiler

Android Studio Build: N/A

Kotlin version: 1.8.10

Steps to Reproduce or Code Sample to Reproduce:

Minimal repro project at https://github.com/bubenheimer/composeearlyinlinereturnbug

@Composable
fun Foo() {
    getOrElse { return }
    getOrElse { return }
}

@Composable
inline fun getOrElse(block: () -> Boolean): Boolean {
    val valid = produceState(Bar(false)) { value = Bar(true) }.value.data
    return if (valid) true else block()
}

class Bar(val data: Boolean)

This looks a little funny only because it's a minimal repro.

Stack trace (if applicable):

androidx.compose.runtime.ComposeRuntimeError: Compose Runtime internal error. Unexpected or incorrect use of the Compose internal runtime API (Missed recording an endGroup). Please report to Google or use https://goo.gle/compose-feedback
at androidx.compose.runtime.ComposerKt.composeRuntimeError(Composer.kt:4501)
at androidx.compose.runtime.ComposerImpl.recordEndGroup(Composer.kt:4663)
at androidx.compose.runtime.ComposerImpl.end(Composer.kt:2395)
at androidx.compose.runtime.ComposerImpl.endRestartGroup(Composer.kt:2884)
at androidx.compose.runtime.internal.ComposableLambdaImpl.invoke(ComposableLambda.jvm.kt:108)
at androidx.compose.runtime.internal.ComposableLambdaImpl.invoke(ComposableLambda.jvm.kt:34)
at androidx.compose.runtime.CompositionLocalKt.CompositionLocalProvider(CompositionLocal.kt:228)
at androidx.compose.ui.platform.CompositionLocalsKt.ProvideCommonCompositionLocals(CompositionLocals.kt:194)
at androidx.compose.ui.platform.AndroidCompositionLocals_androidKt$ProvideAndroidCompositionLocals$3.invoke(AndroidCompositionLocals.android.kt:123)
at androidx.compose.ui.platform.AndroidCompositionLocals_androidKt$ProvideAndroidCompositionLocals$3.invoke(AndroidCompositionLocals.android.kt:122)
at androidx.compose.runtime.internal.ComposableLambdaImpl.invoke(ComposableLambda.jvm.kt:107)
at androidx.compose.runtime.internal.ComposableLambdaImpl.invoke(ComposableLambda.jvm.kt:34)
at androidx.compose.runtime.CompositionLocalKt.CompositionLocalProvider(CompositionLocal.kt:228)
at androidx.compose.ui.platform.AndroidCompositionLocals_androidKt.ProvideAndroidCompositionLocals(AndroidCompositionLocals.android.kt:114)
at androidx.compose.ui.platform.AndroidCompositionLocals_androidKt$ProvideAndroidCompositionLocals$4.invoke(Unknown Source:12)
at androidx.compose.ui.platform.AndroidCompositionLocals_androidKt$ProvideAndroidCompositionLocals$4.invoke(Unknown Source:10)
at androidx.compose.runtime.RecomposeScopeImpl.compose(RecomposeScopeImpl.kt:162)
at androidx.compose.runtime.ComposerImpl.recomposeToGroupEnd(Composer.kt:2465)
at androidx.compose.runtime.ComposerImpl.skipCurrentGroup(Composer.kt:2733)
at androidx.compose.runtime.ComposerImpl$doCompose$2$5.invoke(Composer.kt:3364)
at androidx.compose.runtime.ComposerImpl$doCompose$2$5.invoke(Composer.kt:3342)
at androidx.compose.runtime.SnapshotStateKt__DerivedStateKt.observeDerivedStateRecalculations(DerivedState.kt:341)
at androidx.compose.runtime.SnapshotStateKt.observeDerivedStateRecalculations(Unknown Source:1)
at androidx.compose.runtime.ComposerImpl.doCompose(Composer.kt:3342)
at androidx.compose.runtime.ComposerImpl.recompose$runtime_release(Composer.kt:3307)
at androidx.compose.runtime.CompositionImpl.recompose(Composition.kt:772)
at androidx.compose.runtime.Recomposer.performRecompose(Recomposer.kt:1047)
at androidx.compose.runtime.Recomposer.access$performRecompose(Recomposer.kt:124)
at androidx.compose.runtime.Recomposer$runRecomposeAndApplyChanges$2$2.invoke(Recomposer.kt:541)
at androidx.compose.runtime.Recomposer$runRecomposeAndApplyChanges$2$2.invoke(Recomposer.kt:510)
at androidx.compose.ui.platform.AndroidUiFrameClock$withFrameNanos$2$callback$1.doFrame(AndroidUiFrameClock.android.kt:34)
at androidx.compose.ui.platform.AndroidUiDispatcher.performFrameDispatch(AndroidUiDispatcher.android.kt:109)
at androidx.compose.ui.platform.AndroidUiDispatcher.access$performFrameDispatch(AndroidUiDispatcher.android.kt:41)
at androidx.compose.ui.platform.AndroidUiDispatcher$dispatchCallback$1.doFrame(AndroidUiDispatcher.android.kt:69)
at android.view.Choreographer$CallbackRecord.run(Choreographer.java:1229)
at android.view.Choreographer$CallbackRecord.run(Choreographer.java:1239)
at android.view.Choreographer.doCallbacks(Choreographer.java:899)
at android.view.Choreographer.doFrame(Choreographer.java:827)

Comments

ch...@google.com <ch...@google.com> Mar 23, 2023 05:02PM

Reassigned to ch...@google.com.

ch...@google.com <ch...@google.com> #2Mar 23, 2023 06:10PM

Related bug -

https://issuetracker.google.com/issues/124511903

Initial loads in LimitOffsetPagingSource run in transaction but subsequent loads do not because of cost of transaction. Loads rely on count query to return correct LoadResult.

If count query is outdated, i.e. large amount of data deleted, LimitOffsetPagingSource can return i.e. empty/wrong data and incorrect itemsBefore/itemsAfter/prevKey/nextKey. This can happen if outdated PagingSource doesn't get invalidated in time.

ub...@gmail.com <ub...@gmail.com> #3Mar 23, 2023 08:34PM

Project: platform/frameworks/support
Branch: androidx-main

commit 6bd2347ff259c28b34789c8569b7a298d2a8fd8d
Author: Clara Fok <clarafok@google.com>
Date: Fri Jul 09 12:53:22 2021

Implement Invalid return type in PagingSource

Added third return type for LoadResult sealed class in PagingSource. It
was added as a support feature to handle race scenarios between data loads
and database invalidations, such as if Room's InvalidationTracker does
not propagate invalidation signal to the PagingSource in time and the
PagingSource continues querying an updated database. This can result in
data being loaded onto a stale PageFetcherSnapshot causing duplicated or
missing data. Related bug regarding race scenario in Room

b/191806126

.

For example, if data was deleted and items shifted positions,
positionally-keyed paging sources can end up loading duplicated items.

Paging handles this return type by discarding the loaded data and
invalidating the paging source. This will trigger a new paging source to
be generated. This return type is also supported for Paging2 APIs
leveraging PagingSource such as LivePagedList and RxPagedList.

Aside from handling invalid data due to races, this return type can be
leveraged in general where the database or network returns potentially
invalid or stale data that needs to be discarded.

Bug: 192013267
Test: ./gradlew :paging:paging-common:test
Relnote: "A third LoadResult return type LoadResult.Invalid is added to
PagingSource. When a PagingSource.load returns
LoadResult.Invalid, paging will discard the loaded data and
invalidate the PagingSource. This return type is designed to
handle potentially invalid or stale data that can be returned
from the database or network.

For example, if the underlying database gets written into but
the PagingSource does not invalidate in time, it may return
inconsistent results if its implementation depends on the
immutability of the backing dataset it loads from (e.g., LIMIT
OFFSET style db implementations). In this scenario, it is
recommended to check for invalidation after loading and to
return LoadResult.Invalid, which causes Paging to discard any
pending or future load requests to this PagingSource and
invalidate it.

This return type is also supported by Paging2 API that leverages
LivePagedList or RxPagedList. When using a PagingSource with
Paging2's PagedList APIs, the PagedList is immediately detached,
stopping further attempts to load data on this PagedList and
triggers invalidation on the PagingSource.

LoadResult is a sealed class, which means this is a
source-incomptabile change such that use cases directly using
PagingSource.load results will have to handle LoadResult.Invalid
at compile time. For example, Kotlin users leveraging
exhaustive-when to check return type will have to add a check
for Invalid type."

Change-Id: Id6bd3f2544f1ba97a95a0d590353438a95fedf2a

M paging/common/api/current.txt
M paging/common/api/public_plus_experimental_current.txt
M paging/common/api/restricted_current.txt
M paging/common/src/main/kotlin/androidx/paging/PageFetcherSnapshot.kt
M paging/common/src/main/kotlin/androidx/paging/PagedList.kt
M paging/common/src/main/kotlin/androidx/paging/PagingSource.kt
M paging/common/src/test/kotlin/androidx/paging/PageFetcherSnapshotTest.kt
M paging/common/src/test/kotlin/androidx/paging/PageFetcherTest.kt
M paging/common/src/test/kotlin/androidx/paging/PagedListTest.kt
M paging/common/src/test/kotlin/androidx/paging/PagingDataDifferTest.kt
M paging/common/src/test/kotlin/androidx/paging/PagingSourceTest.kt
M paging/integration-tests/testapp/src/main/java/androidx/paging/integration/testapp/v3room/V3RemoteMediator.kt
M paging/integration-tests/testapp/src/main/java/androidx/paging/integration/testapp/v3room/V3RoomViewModel.kt

https://android-review.googlesource.com/1747297

ch...@google.com <ch...@google.com> #4Mar 23, 2023 08:41PM

Project: platform/frameworks/support
Branch: androidx-main

commit 3882ac8364fd905c9935d38c93f84599009226d6
Author: Clara Fok <clarafok@google.com>
Date: Fri Jul 09 14:17:48 2021

Implement LoadResult.Invalid to handle race

For non-initial loads in LimitOffsetPagingSource, loads will run Room
InvalidationTracker's refreshVersionSync to synchronously check if
tables have been invalidated and notify observers. If invalidated,
load will return LoadResult.Invalid.

Paging3 will handle Invalid return type by invalidating the paging
source and closing the page event flow, such that no more loads will be
attempted on this paging source.

Note that refreshVersionSync is a temporary solution for
LimitOffsetPagingSource to check table invalidation. Another solution
that does not require a transaction is in development. Related

b/192269858

.

Bug: 191806126
Test: ./gradlew :room:integration-tests:room-testapp-kotlin:cC
Test: ./gradlew room:room-paging:cC
Change-Id: I48634ab95949bae608374f5156a4edd0500a74a3

M room/integration-tests/kotlintestapp/src/androidTest/java/androidx/room/integration/kotlintestapp/test/PagingSourceTest.kt
M room/room-paging/build.gradle
M room/room-paging/src/androidTest/kotlin/androidx/room/paging/LimitOffsetPagingSourceTest.kt
M room/room-paging/src/main/kotlin/androidx/room/paging/LimitOffsetPagingSource.kt

https://android-review.googlesource.com/1761130

ub...@gmail.com <ub...@gmail.com> #5Mar 23, 2023 08:47PM

Thank you! I just opened yet another issue (regression in 1.4.3/1.4.4), was thinking it may be close to your present changes, too: https://issuetracker.google.com/issues/274786923

ap...@google.com <ap...@google.com> #6Mar 24, 2023 04:06PM

Marked as fixed.

Project: platform/frameworks/support
Branch: androidx-main

commit d71d980734a752e516140dd3ac031af5bab86aa3
Author: Chuck Jazdzewski <chuckj@google.com>
Date: Wed Mar 22 16:20:43 2023

Fix `endToMarker()` when ending node groups.

`endToMarker()` is called when a composable function calls
an inline function that has a non-local return. If the non-local
return would return passed the `endNode()` call of an inline
composable function `endToMarker()` was supposed to call `end()`
of the composer with `true` but it was unconditionally calling
it with `false` causing the runtime to get confused as to when
to generate a node.

Also if the `currentMarker()` was called when recomposing existing
content but the `endToMaker()` was called with new content the
groups in the new content would not correctly be closed if the
current marker is in the direct parent group of the new content.

Fixes: 264467571
Fixes: 274889428
Test: ./gradlew :compose:r:r:tDUT
Change-Id: Ibe7063cf22f4bff6eda5bde05c37c1e665c09167

M compose/runtime/runtime/src/commonMain/kotlin/androidx/compose/runtime/Composer.kt
M compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/CompositionTests.kt
M compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/mock/MockViewValidator.kt
M compose/runtime/runtime/src/commonTest/kotlin/androidx/compose/runtime/mock/Views.kt

https://android-review.googlesource.com/2504059

ch...@google.com <ch...@google.com> #7Mar 24, 2023 05:06PM

The above fix was also requested to be cherry-picked into the 1.4 branch.

ch...@google.com <ch...@google.com> #8Mar 24, 2023 05:09PM

As for #5, b/274786923 was introduced in the 1.4.3 version of the compiler, due to an unrelated change, and is still in 1.4.4. The above change does not address this issue as b/274786923 it will require a fix in the compiler plugin, not the runtime.

na...@google.com <na...@google.com> #9Apr 5, 2023 06:27PM

The following release(s) address this bug.It is possible this bug has only been partially addressed:

androidx.compose.runtime:runtime:1.4.1
androidx.compose.runtime:runtime:1.5.0-alpha02