"cannot collect twice from pager" error after configuration change + Room PagingSource change [158784811]

Fixed

Bug

Status Update

No update yet.

Description

ia...@gmail.com

created issue #1

Jun 12, 2020 03:15AM

Component used: Paging Version used: 3.0.0-alpha01

I was moving my app over to Paging 3.0.0-alpha01. It uses Room 2.3.0-alpha01 for support for PagingSource.

Dao:

internal abstract val chosenPhotosPaged: PagingSource<Int, ChosenPhoto>

ViewModel:

internal val chosenPhotos = Pager(PagingConfig(24, 12)) {
    GalleryDatabase.getInstance(application).chosenPhotoDao().chosenPhotosPaged
}.flow.cachedIn(viewModelScope)

UI:

lifecycleScope.launchWhenStarted {
    viewModel.chosenPhotos.collect {
        // Load into a PagingDataAdapter
        chosenPhotosAdapter.submitData(it)
    }
}

Steps to reproduce:

Checkout the paging3 branch of Muzei
Add 30+ images to your device
Install and Activate Muzei
Go to the Sources screen and select the "My Photos" source
Accept the runtime permission
Go to the Settings screen
Hit the FAB at the bottom, then the image button
Select ~2-5 images
Rotate your device
Hit the FAB at the bottom, image, and add additional images

Expected behavior: The new images are seamlessly added

Actual behavior: I received the following exception:

2020-06-11 19:54:18.401 net.nurik.roman.muzei E/AndroidRuntime: FATAL EXCEPTION: main
    Process: net.nurik.roman.muzei, PID: 21268
    java.lang.IllegalStateException: cannot collect twice from pager
        at androidx.paging.PageFetcherSnapshot$pageEventFlow$1.invokeSuspend(PageFetcherSnapshot.kt:94)
        at androidx.paging.PageFetcherSnapshot$pageEventFlow$1.invoke(Unknown Source:10)
        at androidx.paging.CancelableChannelFlowKt$cancelableChannelFlow$1.invokeSuspend(CancelableChannelFlow.kt:35)
        at androidx.paging.CancelableChannelFlowKt$cancelableChannelFlow$1.invoke(Unknown Source:10)
        at kotlinx.coroutines.flow.ChannelFlowBuilder.collectTo$suspendImpl(Builders.kt:327)
        at kotlinx.coroutines.flow.ChannelFlowBuilder.collectTo(Unknown Source:0)
        at kotlinx.coroutines.flow.internal.ChannelFlow$collectToFun$1.invokeSuspend(ChannelFlow.kt:53)
        at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
        at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:56)
        at kotlinx.coroutines.EventLoop.processUnconfinedEvent(EventLoop.common.kt:69)
        at kotlinx.coroutines.DispatchedTaskKt.resumeUnconfined(DispatchedTask.kt:184)
        at kotlinx.coroutines.DispatchedTaskKt.dispatch(DispatchedTask.kt:108)
        at kotlinx.coroutines.CancellableContinuationImpl.dispatchResume(CancellableContinuationImpl.kt:308)
        at kotlinx.coroutines.CancellableContinuationImpl.resumeImpl(CancellableContinuationImpl.kt:318)
        at kotlinx.coroutines.CancellableContinuationImpl.resumeWith(CancellableContinuationImpl.kt:250)
        at kotlinx.coroutines.channels.AbstractChannel$ReceiveElement.resumeReceiveClosed(AbstractChannel.kt:877)
        at kotlinx.coroutines.channels.AbstractSendChannel.helpClose(AbstractChannel.kt:312)
        at kotlinx.coroutines.channels.AbstractSendChannel.close(AbstractChannel.kt:241)
        at kotlinx.coroutines.channels.ChannelCoroutine.close(Unknown Source:2)
        at kotlinx.coroutines.channels.SendChannel$DefaultImpls.close$default(Channel.kt:102)
        at androidx.paging.CancelableChannelFlowKt$cancelableChannelFlow$1$1.invoke(CancelableChannelFlow.kt:33)
        at androidx.paging.CancelableChannelFlowKt$cancelableChannelFlow$1$1.invoke(Unknown Source:2)
        at kotlinx.coroutines.InvokeOnCompletion.invoke(JobSupport.kt:1386)
        at kotlinx.coroutines.JobSupport.notifyCompletion(JobSupport.kt:1529)
        at kotlinx.coroutines.JobSupport.completeStateFinalization(JobSupport.kt:323)
        at kotlinx.coroutines.JobSupport.finalizeFinishingState(JobSupport.kt:240)
        at kotlinx.coroutines.JobSupport.tryMakeCompletingSlowPath(JobSupport.kt:903)
        at kotlinx.coroutines.JobSupport.tryMakeCompleting(JobSupport.kt:860)
        at kotlinx.coroutines.JobSupport.cancelMakeCompleting(JobSupport.kt:693)
        at kotlinx.coroutines.JobSupport.cancelImpl$kotlinx_coroutines_core(JobSupport.kt:664)
        at kotlinx.coroutines.JobSupport.cancelInternal(JobSupport.kt:629)
        at kotlinx.coroutines.JobSupport.cancel(JobSupport.kt:614)
        at kotlinx.coroutines.Job$DefaultImpls.cancel$default(Job.kt:164)
        at androidx.paging.PageFetcherSnapshot.close(PageFetcherSnapshot.kt:183)
        at androidx.paging.PageFetcher$flow$1$2.invokeSuspend(PageFetcher.kt:75)
        at androidx.paging.PageFetcher$flow$1$2.invoke(Unknown Source:18)
        at kotlinx.coroutines.flow.FlowKt__TransformKt$scan$$inlined$unsafeFlow$1$lambda$1.emit(Collect.kt:133)
        at kotlinx.coroutines.flow.FlowKt__ChannelsKt.emitAllImpl$FlowKt__ChannelsKt(Channels.kt:58)
        at kotlinx.coroutines.flow.FlowKt__ChannelsKt$emitAllImpl$1.invokeSuspend(Unknown Source:11)
        at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
        at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:56)
        at android.os.Handler.handleCallback(Handler.java:883)
        at android.os.Handler.dispatchMessage(Handler.java:100)
        at android.os.Looper.loop(Looper.java:214)
        at android.app.ActivityThread.main(ActivityThread.java:7356)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930)
2020-06-11 19:54:18.407 net.nurik.roman.muzei I/Process: Sending signal. PID: 21268 SIG: 9

Comments

du...@google.com <du...@google.com> #2Jun 12, 2020 03:35AM

Reassigned to du...@google.com.

Project: platform/frameworks/support
Branch: androidx-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

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

ia...@gmail.com <ia...@gmail.com> #3Jun 12, 2020 03:46AM

Re #2 - no, using collectLatest does not fix the issue.

FWIW, I am always using the suspend submitData and never the non-suspend submitData.

va...@gmail.com <va...@gmail.com> #4Jun 13, 2020 03:26PM

The same happened to me when using findNavController().popBackStack() instead of configuration change with the non-suspend submitData `

ap...@google.com <ap...@google.com> #5Jun 15, 2020 03:29PM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit 310351127da4b970a9c5b5dbaa796d0cf19abac0
Author: Yigit Boyar <yboyar@google.com>
Date: Fri Jun 12 22:52:50 2020

Never collect for Flow<PageEvent> twice

This CL Fixes a bug where the Multicast logic could possibly
restart the Flow<PageEvent> if upsteam closes and a new downstream
arrives before we get the new PagedData.

Since Flow<PageEvent> (PagedData.flow) cannot be collected twice and
we already keep the history of events cached, this CL ensures that
if Multicaster is restarted, we won't connect to the original flow.

Instead, we'll just complete that flow as an empty flow and let
downstream receive history of events, if any exists.

I'm not changing Multicaster because from its perspective, restarting
upstream makes sense if there is a new downstream after previous
upstream closes. This behavior is specifc to CachedPageEventFlow hence
handled there.
We run Multicaster with keepAlive so there is no case where the
collection from upstream would stop unless:
* upstream closes
* we close the CachedPageEventFlow (usually due to a new one)
* scope is cancelled

Bug: 158784811
Test: CachedPageEventFlowTest, muzei
Change-Id: I58bcc4b2bde88b1c78ed1a96cf227e068127b47e

M paging/common/src/main/kotlin/androidx/paging/CachedPageEventFlow.kt
M paging/common/src/test/kotlin/androidx/paging/CachedPageEventFlowTest.kt

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

yb...@google.com <yb...@google.com> Jun 15, 2020 04:17PM

Marked as fixed.

lu...@gmail.com <lu...@gmail.com> #6Jun 29, 2020 01:24PM

i still get this crash in alpha2.
i use simple fragment and non-suspend submit
```
java.lang.IllegalStateException: cannot collect twice from pager
at androidx.paging.PageFetcherSnapshot$pageEventFlow$1.invokeSuspend(PageFetcherSnapshot.kt:94)
at androidx.paging.PageFetcherSnapshot$pageEventFlow$1.invoke(PageFetcherSnapshot.kt)
at androidx.paging.CancelableChannelFlowKt$cancelableChannelFlow$1.invokeSuspend(CancelableChannelFlow.kt:35)
at androidx.paging.CancelableChannelFlowKt$cancelableChannelFlow$1.invoke(CancelableChannelFlow.kt)
at kotlinx.coroutines.flow.ChannelFlowBuilder.collectTo$suspendImpl(Builders.kt:327)
at kotlinx.coroutines.flow.ChannelFlowBuilder.collectTo(Builders.kt)
at kotlinx.coroutines.flow.internal.ChannelFlow$collectToFun$1.invokeSuspend(ChannelFlow.kt:53)
at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:56)
at kotlinx.coroutines.EventLoop.processUnconfinedEvent(EventLoop.common.kt:69)
at kotlinx.coroutines.DispatchedContinuationKt.resumeCancellableWith(DispatchedContinuation.kt:337)
at kotlinx.coroutines.intrinsics.CancellableKt.startCoroutineCancellable(Cancellable.kt:26)
at kotlinx.coroutines.CoroutineStart.invoke(CoroutineStart.kt:109)
at kotlinx.coroutines.AbstractCoroutine.start(AbstractCoroutine.kt:158)
at kotlinx.coroutines.BuildersKt__Builders_commonKt.launch(Builders.common.kt:54)
at kotlinx.coroutines.BuildersKt.launch(Unknown Source)
at kotlinx.coroutines.BuildersKt__Builders_commonKt.launch$default(Builders.common.kt:47)
at kotlinx.coroutines.BuildersKt.launch$default(Unknown Source)
at androidx.paging.AsyncPagingDataDiffer.submitData(AsyncPagingDataDiffer.kt:181)
at androidx.paging.PagingDataAdapter.submitData(PagingDataAdapter.kt:115)
at ds.photosight.view.GalleryFragment$onViewCreated$$inlined$observe$2.onChanged(LiveData.kt:53)
```

du...@google.com <du...@google.com> #7Jun 29, 2020 03:51PM

Are you able to provide a repro?

Make sure you didn't forget to call .cachedIn(scope)?

ch...@gmail.com <ch...@gmail.com> #8Jul 8, 2020 07:59AM

pagerFlow
                .combine(removedItemsFlow) { pagingData, removed ->
                    pagingData.filter { (it as CollectEntity).id !in removed }
                }
                .cachedIn(viewmodel.viewModelScope)
                .collectLatest {
                   ****
                }

i found the same error , if cachedIn is after combine, it will broke up, but it is normal when cachedIn call twice , such as cachedIn().combine().cachedIn() , but it will not cache the combine data, because i want to cache the data after filter

du...@google.com <du...@google.com> #9Jul 8, 2020 05:10PM

It's expected to need to call .cachedIn twice there as you've done since re-collection is unsupported on pagingData, operators like combine cause re-collection so without cachedIn you would be expecting to reload the data from scratch any time a new value is emitted to removedItemsFlow

In your example you are correctly caching the filtered data.

After https://android-review.googlesource.com/c/platform/frameworks/support/+/1356724 lands Paging will call the initial .cachedIn for you (presumably in alpha03).

ch...@gmail.com <ch...@gmail.com> #10Jul 10, 2020 06:22AM

In my example i call .cachedIn twice, but actually it doesn't caching the filtered data, so i have to keep the whole remove data and it will filter all remove data in source data every time.

Such as i want to remove data which id is 1， then i remove id 2 ,but when i remove 2,i have to filter 1 again, so i have to save all remove data, so i think the re-collection doesn't work event i call .cachedIn twice

du...@google.com <du...@google.com> #11Jul 11, 2020 12:40AM

In my example i call .cachedIn twice, but actually it doesn't caching the filtered data, so i have to keep the whole remove data and it will filter all remove data in source data every time.

Such as i want to remove data which id is 1， then i remove id 2 ,but when i remove 2,i have to filter 1 again, so i have to save all remove data, so i think the re-collection doesn't work event i call .cachedIn twice

Sorry - I'm not sure I entirely understand what you're doing here. .cachedIn doesn't affect what REFRESH returns, it simply prevents repeated work on recollection within the specified scope (for example if you change from landscape to portrait).

However on each new instance of PagingData due to invalidation, you would still need to filter the entire stream of events, since it's a new stream.

ch...@gmail.com <ch...@gmail.com> #12Jul 11, 2020 02:41AM

I want to do something like notifyItemRemoved , for example there is a collect list ,i want to uncollect the item1, then uncollect the item2, so when i uncollcet item2 i have to filter item1 again ,

I don’t know if you can understand this example, because i think it's useless when call the .cachedIn after filter, because it can't caching the new filter data , so if i want to remove data,i have to keep all item i want to remove even the data already removed.

du...@google.com <du...@google.com> #13Jul 11, 2020 04:34AM

In this case it's recommended to update the backing dataset directly and call invalidate(). We're investigating a page-level flow api that will allow you to control the items themselves without invalidation which you can track in #9.

Calling .cachedIn after .filter prevents your .filter from needing to run again on the same list. For example on configuration change or when navigating between fragments. It has nothing to do with making your filter operation incremental.

ch...@gmail.com <ch...@gmail.com> #14Jul 13, 2020 02:36AM

Thank you for your reply,I hope you can proceed smoothly and have more powerful api.