Skip to content
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.

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

Have RxPagedListBuilder emit errors in RxJava chain #419

Open
wants to merge 1 commit into
base: androidx-main
Choose a base branch
from

Conversation

veyndan
Copy link
Contributor

@veyndan veyndan commented Jun 20, 2022

Proposed Changes

From https://issuetracker.google.com/issues/233703110:

In Paging 2.1.2, when an error is thrown from the DataSource.Factory, the error is propagated through the rx chain, so we could use things like onErrorResumeNext(…) when there's an error so we can fail safely. In Paging 3, this error is no longer propagated through the rx chain, so our callback in onErrorResumeNext(…) is never invoked.

I've just wrapped the invalidate job contents with a try-catch. This is a greatly simplified version of what rxObservable does.

Note: the diff in GitHub looks like many things changed in RxPagedListBuilder, but checking out the diff in Android Studio shows that I did in fact just wrap it all in a try-catch.

Testing

Test: ./gradlew paging:paging-rxjava2:test paging:paging-rxjava2:test

Issues Fixed

Fixes: 233703110

@google-cla
Copy link

google-cla bot commented Jun 20, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@claraf3
Copy link
Member

claraf3 commented Jun 27, 2022

Hi, thanks for the PR. Would you be able to update the original bug https://issuetracker.google.com/issues/233703110 with a sample project so we can repro your use case? It would help us evaluate whether it is a bug and how we might resolve it.

Closing this PR in the meantime but we'll keep an eye on the bug for when you update it. Thank you.

@claraf3 claraf3 closed this Jun 27, 2022
@veyndan
Copy link
Contributor Author

veyndan commented Jun 28, 2022

Hi @claraf3,

In https://issuetracker.google.com/issues/233703110#comment1 I'd already linked a failing test which showcases the problem. The link is to this file, mainly because the only substantive changes in the repo is that file, but you should be able to clone https://github.com/veyndan/androidx-paging-2-to-3-regressions and run the tests locally. By toggling between Paging 2 (e.g., v2.1.2) and Paging 3 (e.g., v3.1) in the build.gradle.kts file, you'll be able to see that the tests pass when using Paging 2, but fail when using Paging 3.

Please let me know if there were troubles recreating the issue on your end.

@veyndan
Copy link
Contributor Author

veyndan commented Jul 12, 2022

Hey @claraf3, is there anything else that I can do to move this along?

@dlam dlam reopened this Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants