Skip to content

Conversation

@CDRussell
Copy link
Member

@CDRussell CDRussell commented Apr 22, 2020

Task/Issue URL: https://app.asana.com/0/414730916066338/1172307373191458
Tech Design URL:
CC:

Description:
After updating the app dependencies to newest versions of all libraries, we noticed an increase in UndeliverablExceptions. This, it seems, is most likely caused by our sync job being interrupted, so we in turn dispose of the RxJava subscription. However in cases where the networking capabilities have just dropped (a potential cause of the sync job being stopped) we would then get a bunch of IO-related exceptions being called and since we'd already disposed, RxJava would have nowhere to deliver the exception to, hence it would throw an UndeliverableException.

This UndeliverableException wouldn't be caught by a custom RxJava global error handler (as we don't set one) so it would default to the uncaught exception handler for the thread, which would crash the app.

This change catches and logs UndeliverableExceptions but doesn't let the app crash because of them. For all other exceptions, they continue to defer to the uncaught exception handler as before.

Steps to test this PR:
Getting the sync job to fail at the right time is hard, so best to replicate this scenario is to start the sync and then almost immediately dispose of that RxJava subscription. ie, stick this somewhere you can invoke it at your pleasure 👇

    val task = appConfigurationSyncer.scheduleImmediateSync()
        .subscribeOn(Schedulers.io())
        .subscribe({
            Timber.i("Finished downloads")
        }, {
            Timber.w("Failed to download initial app configuration ${it.localizedMessage}")
        })

    Handler().postDelayed(500) { task.dispose() }

ℹ️This might not crash every time, but it should do most of the time it is run.

  1. Verify the above crashes the app on develop
  2. Verify it no longer crashes on this branch.

Internal references:

Software Engineering Expectations
Technical Design Template

After updating the app dependencies to newest versions of all libraries, we noticed an increase in UndeliverablExceptions. This, it seems, is most likely caused by our sync job being interrupted, so we in turn dispose of the RxJava subscription. However in cases where the networking capabilities have just dropped (a potential cause of the sync job being stopped) we would then get a bunch of IO-related exceptions being called and since we'd already disposed, RxJava would have nowhere to deliver the exception to, hence it would throw an UndeliverableException.

This UndeliverableException wouldn't be caught by the RxJava global error handler (as we don't set one) so it would default to the uncaught exception handler for the thread.

This change catches and logs UndeliverableExceptions but doesn't let the app crash because of them. For all other exceptions, they continue to defer to the uncaught exception handler as before.
Copy link
Contributor

@marcosholgado marcosholgado left a comment

Choose a reason for hiding this comment

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

Good job! 👍

@CDRussell CDRussell merged commit 5bfbd93 into develop Apr 22, 2020
@CDRussell CDRussell deleted the feature/craig/add_global_rxjava_error_handler branch April 22, 2020 10:51
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