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

Improve episode sync error handling #711

Merged
merged 3 commits into from
Jan 17, 2023
Merged

Conversation

mchowning
Copy link
Contributor

@mchowning mchowning commented Jan 17, 2023

Description

This PR should improve the reliability of our episode sync.

In my testing, all the calls to sync episode data were failing and a NoSuchElementException was getting logged. I think there are two separate causes of this, and fixing either of them solves the issue. This PR includes both fixes.

The first fix is that although we were calling onErrorReturnItem with our sync poller (which syncs the episode status every 60 seconds), that handler would complete the polling, causing sync to stop. The better approach is to handle the error inside the concatMap call because that way the polling doesn't stop if there is an error.

The second fix is that, in my testing, every time I call the sync/update_episode endpoint, I would get the NoSuchElementException. It sounds like this can be an issue with using a Single without a response body. Changing this to be a Completable resolved the problem.

The problem is that I feel pretty confident that I'm missing something there because as far as I can tell, this code hasn't changed recently, and I find it unlikely that sync has been as broken as I'm experiencing that entire time, so maybe there is something I'm doing that is uniquely making this problem appear.

Basically, please look at these changes with a skeptical eye, particularly the changes to the sync/update_episode endpoint, since the additional error handling on its own is sufficient to fix the bug. I just made the second change to avoid throwing and catching an error on every API call to that endpoint, but we can easily and safely remove that change (I put it in a separate commit for exactly that reason).

Testing Instructions

Verify the problem

  1. Using a build that does not have the changes from this PR
  2. (recommended, but optional) update the code here to force the sync to happen every 10 seconds (default is 60 seconds, but who has that kind of patience 😄 )
  3. Log into the app and begin playing a podcast
  4. Observe that after 10 seconds a call is made to the sync API endpoint, and it returns an empty response
  5. Wait at least 10 seconds and observe that no other calls are made to the sync API endpoint. (This is what I was observing at least, but I would like to confirm that it's not only me who is seeing this).
  6. If the response doesn't error on it's own, it may be interesting to see if changing the response code to 404 causes the sync polling to stop (it does for me without the changes in this PR).

Verify the fix

  1. Using a build that includes the changes from this PR
  2. Again, update the frequency of the sync polling
  3. Log into the app and begin playing a podcast
  4. Observe that after 10 seconds a call is made to the sync API endpoint, and it returns an empty response
  5. Observe that polling continues every 10 seconds
  6. Intercept one of the responses and change the status code to be an error (I used 404)
  7. Observe that the polling continues every 10 seconds

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • I have considered whether it makes sense to add tests for my changes
  • All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
  • Any jetpack compose components I added or changed are covered by compose previews

I have tested any UI changes...

  • with different themes
  • with a landscape orientation
  • with the device set to have a large display and font size
  • for accessibility with TalkBack

Related issue: #616

The Single<Void> return was throwing NoSuchElementExceptions in my
testing
@mchowning mchowning marked this pull request as ready for review January 17, 2023 22:10
@mchowning mchowning requested a review from a team as a code owner January 17, 2023 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants