-
Notifications
You must be signed in to change notification settings - Fork 527
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
Introduce thread-safe MediatorLiveData or alternative #90
Comments
It appears the current MediatorLiveData approach can result in incorrect downstream behavior. Consider the following situation:
This was found during #122 when a partially-complete progress controller implementation tried to notify observers of a data provider change within the data provider. The implementation was able to work around this by avoiding the notification during the data provider callback, however this did expose a real issue with this multi-livedata approach for propagating coroutine results to the UI. There should be a simpler, more performant, and correct way to tie coroutines to LiveData without using MediatorLiveData. |
Correction: the workaround didn't work. It may be that any situation where another scope is combined with a data provider that cancellation is highly likely whenever the mediator live data is notified of a change. This isn't correct: pending jobs should not be cancelled just because a change on the data provider can now be observed. |
AnswerClassificationController, and attempted to make ExplorationProgressController thread-safe. The thread-safety led to significant interface changes in the progress controller, and led to discovering some issues with the mediator live data approach to interop coroutines and LiveData. This locking mechanism will need to change since the optimal solution requires resolving #90.
The workaround in the meantime will be to move execution for the progress controller back to the main thread and use a standard lock for synchronizing thread access for shared state. This issue will block introducing the more performant option. We'll keep the interface fully asynchronous to support this solution in the future. |
#122 also introduced a DataProvider wrapper for Deferred which might be nice to reintroduce, but it needs to be really carefully considered since it may have downstream implications for this mediator mechanism. That provider looked like the following: /**
* Returns an in-memory [DataProvider] which wraps the specified [Deferred] and provides either its successful value
* or its resulting failure.
*/
fun <T> createDeferredDataProviderAsync(id: Any, deferred: Deferred<T>): DataProvider<T> {
return createInMemoryDataProviderAsync(id) {
try {
AsyncResult.success(deferred.await())
} catch (e: Exception ) {
AsyncResult.failed<T>(e)
}
}
} |
with the current MediatorLiveData implementation (see #90 for more context). Fix existing progress controller tests and add a few more. All current progress controller tests are passing.
…nProgressController (#183) * Introduce first pass interface for ExplorationProgressController. * Fill in the stubbed logic for ExplorationProgressController. Still no tests to verify correctness. Also, added a method to facilitate notifying of DataProvider changes on the UI thread. * Fix lateinit issue in ExplorationProgressController due to wrongly ordered initialization. * Fix a variaty of issues in the exp progress controller, properly hook it up to the data controller, and start adding tests. * Created a separate ExplorationRetriever, hooked up AnswerClassificationController, and attempted to make ExplorationProgressController thread-safe. The thread-safety led to significant interface changes in the progress controller, and led to discovering some issues with the mediator live data approach to interop coroutines and LiveData. This locking mechanism will need to change since the optimal solution requires resolving #90. * Change the locking mechanism for ExplorationProgressController to work with the current MediatorLiveData implementation (see #90 for more context). Fix existing progress controller tests and add a few more. All current progress controller tests are passing. * Finish tests for ExplorationProgressController and add test classification support for the second test exploration (about_oppia).
* Introduce first pass interface for ExplorationProgressController. * Fill in the stubbed logic for ExplorationProgressController. Still no tests to verify correctness. Also, added a method to facilitate notifying of DataProvider changes on the UI thread. * Fix lateinit issue in ExplorationProgressController due to wrongly ordered initialization. * Fix a variaty of issues in the exp progress controller, properly hook it up to the data controller, and start adding tests. * Created a separate ExplorationRetriever, hooked up AnswerClassificationController, and attempted to make ExplorationProgressController thread-safe. The thread-safety led to significant interface changes in the progress controller, and led to discovering some issues with the mediator live data approach to interop coroutines and LiveData. This locking mechanism will need to change since the optimal solution requires resolving #90. * Change the locking mechanism for ExplorationProgressController to work with the current MediatorLiveData implementation (see #90 for more context). Fix existing progress controller tests and add a few more. All current progress controller tests are passing. * Finish tests for ExplorationProgressController and add test classification support for the second test exploration (about_oppia). * First iteration at implementing real answer classification for the Oppia prototype. This uses a Dagger-powered solution to make it straightforward to add new rule types and interactions in a way that automatically hooks into the classifier. This only adds TextInput support, so existing tests do not pass. Tests and the app do build. * Bind all text input rules, add numeric input rules, and add support for two bound input values. * Add numbers with units classification support. * Add multiple choice input classification support. Fix some of the exploration progress controller tests for numeric input. * Add item selection classification support. Clean up all rule classifier comments to point to their corresponding Oppia web versions. * Add fractions input classification support. * Add Continue module. Resolve some TODOs, add TODOs to test classifiers, and fix ExplorationProgressController tests (which also included fixing one bug in the TextInput FuzzyEquals and the progress controller's fallback routing logic). * Add thorough tests for AnswerClassificationController. * Consolidate generic rule classifiers into a single class.
* Introduce first pass interface for ExplorationProgressController. * Fill in the stubbed logic for ExplorationProgressController. Still no tests to verify correctness. Also, added a method to facilitate notifying of DataProvider changes on the UI thread. * Fix lateinit issue in ExplorationProgressController due to wrongly ordered initialization. * Fix a variaty of issues in the exp progress controller, properly hook it up to the data controller, and start adding tests. * Created a separate ExplorationRetriever, hooked up AnswerClassificationController, and attempted to make ExplorationProgressController thread-safe. The thread-safety led to significant interface changes in the progress controller, and led to discovering some issues with the mediator live data approach to interop coroutines and LiveData. This locking mechanism will need to change since the optimal solution requires resolving #90. * Change the locking mechanism for ExplorationProgressController to work with the current MediatorLiveData implementation (see #90 for more context). Fix existing progress controller tests and add a few more. All current progress controller tests are passing. * Finish tests for ExplorationProgressController and add test classification support for the second test exploration (about_oppia). * ObservableViewModel * Load exploration and open ExplorationActivity * Test case for load exploration * Connection with StateFragment * Nit changes self review * Refactor home * Resolve merge conflicts * Test case updated
* Introduce first pass interface for ExplorationProgressController. * Fill in the stubbed logic for ExplorationProgressController. Still no tests to verify correctness. Also, added a method to facilitate notifying of DataProvider changes on the UI thread. * Fix lateinit issue in ExplorationProgressController due to wrongly ordered initialization. * Fix a variaty of issues in the exp progress controller, properly hook it up to the data controller, and start adding tests. * Created a separate ExplorationRetriever, hooked up AnswerClassificationController, and attempted to make ExplorationProgressController thread-safe. The thread-safety led to significant interface changes in the progress controller, and led to discovering some issues with the mediator live data approach to interop coroutines and LiveData. This locking mechanism will need to change since the optimal solution requires resolving #90. * Change the locking mechanism for ExplorationProgressController to work with the current MediatorLiveData implementation (see #90 for more context). Fix existing progress controller tests and add a few more. All current progress controller tests are passing. * Finish tests for ExplorationProgressController and add test classification support for the second test exploration (about_oppia). * ObservableViewModel * Load exploration and open ExplorationActivity * Test case for load exploration * Connection with StateFragment * Nit changes self review * Refactor home * Resolve merge conflicts * Test case updated * Drawable for input type edittext background and added items in # app/src/main/res/values/colors.xml # app/src/main/res/values/dimens.xml * Drawable for input type edittext background and added items in # app/src/main/res/values/colors.xml # app/src/main/res/values/dimens.xml * # app/src/main/res/values/colors.xml
* Introduce first pass interface for ExplorationProgressController. * Fill in the stubbed logic for ExplorationProgressController. Still no tests to verify correctness. Also, added a method to facilitate notifying of DataProvider changes on the UI thread. * Fix lateinit issue in ExplorationProgressController due to wrongly ordered initialization. * Fix a variaty of issues in the exp progress controller, properly hook it up to the data controller, and start adding tests. * Created a separate ExplorationRetriever, hooked up AnswerClassificationController, and attempted to make ExplorationProgressController thread-safe. The thread-safety led to significant interface changes in the progress controller, and led to discovering some issues with the mediator live data approach to interop coroutines and LiveData. This locking mechanism will need to change since the optimal solution requires resolving #90. * Change the locking mechanism for ExplorationProgressController to work with the current MediatorLiveData implementation (see #90 for more context). Fix existing progress controller tests and add a few more. All current progress controller tests are passing. * Finish tests for ExplorationProgressController and add test classification support for the second test exploration (about_oppia). * craeted image parser * ObservableViewModel * Load exploration and open ExplorationActivity * Test case for load exploration * fetch exploration * Connection with StateFragment * update * update * working on fetching customization_args * Update StateFragmentPresenter.kt * Separated this in its own PR * Delete UrlImageParser.kt Separated this in its own PR. * Update StateFragmentPresenter.kt * Update build.gradle * Create HtmlParser.kt * Update HtmlParser.kt * Update HtmlParser.kt * Update HtmlParser.kt * working on testcases * Update HtmlParserTestActivty.kt * Update HtmlParserTestActivty.kt * added testcases. * Update AndroidManifest.xml * Update InteractionAdapter.kt * Update InteractionAdapter.kt * Update HtmlParser.kt * corrected variable names. * updated variables. * Update HtmlParser.kt * Update HtmlParser.kt * Update HtmlParserTestActivty.kt * Update HtmlParser.kt * Delete InteractionAdapter.kt * deteted unused xml files * merged html code. * Update HtmlParser.kt * Delete UrlImageParserTest.kt * used dagger injection for UrlImageParser. * Update HtmlParserTestActivty.kt * Update HtmlParserTestActivty.kt * Update HtmlParser.kt * Update HtmlParserTestActivty.kt * updated tests * updated test * moved to testing package * Update HtmlParserTestActivty.kt * Update HtmlParserTestActivty.kt * fix issues * fix issues. * fixed issues * fixed issues. * Update activity_test_html_parser.xml * fixes * Update HtmlParserTest.kt * Update HtmlParserTest.kt * fixed issues. * fixing issues. * made use of factory * Update HtmlParserTest.kt * Update UrlImageParser.kt * Update AndroidManifest.xml * Update HtmlParserTestActivity.kt * nit * Update AudioFragmentTest.kt * updated * fixes * Update UrlImageParser.kt * Not used in this. * fixes * Not used. * Update CellularDataDialogFragmentTest.kt * reverted * Update CellularDataDialogFragmentTest.kt * fixes * Update ImageLoader.kt * Update HtmlParser.kt * Update HtmlParser.kt * Update build.gradle * fixed issues. * Update UrlImageParser.kt * Update UrlImageParser.kt * Update UrlImageParser.kt * Update ImageParsingAnnotations.kt * working on image loader * Injected ImageLoader * updated image loader. * Update ActivityComponent.kt * Update HtmlParserTestActivity.kt * Update GlideImageLoader.kt * removed annotation * Update HtmlParser.kt * line break * fix * fix comments * Update UrlImageParser.kt * Update UrlImageParser.kt * injected context * working on testcase. * testcase added * added test case * testcases * Update build.gradle * Update AndroidManifest.xml * fixes * Update UrlImageParserTest.kt * changed to CutomTarget * Update UrlImageParserTest.kt * updated. * Update UrlImageParserTest.kt * Update HtmlParserTestActivity.kt * Update misc.xml * Update GlideImageLoaderModule.kt * Update HtmlParserTest.kt * deleted test class for loading image and added todo * updated * Update build.gradle * Update GlideImageLoader.kt * Update build.gradle * Update UrlImageParser.kt
* Introduce first pass interface for ExplorationProgressController. * Fill in the stubbed logic for ExplorationProgressController. Still no tests to verify correctness. Also, added a method to facilitate notifying of DataProvider changes on the UI thread. * Fix lateinit issue in ExplorationProgressController due to wrongly ordered initialization. * Fix a variaty of issues in the exp progress controller, properly hook it up to the data controller, and start adding tests. * Created a separate ExplorationRetriever, hooked up AnswerClassificationController, and attempted to make ExplorationProgressController thread-safe. The thread-safety led to significant interface changes in the progress controller, and led to discovering some issues with the mediator live data approach to interop coroutines and LiveData. This locking mechanism will need to change since the optimal solution requires resolving #90. * Change the locking mechanism for ExplorationProgressController to work with the current MediatorLiveData implementation (see #90 for more context). Fix existing progress controller tests and add a few more. All current progress controller tests are passing. * Finish tests for ExplorationProgressController and add test classification support for the second test exploration (about_oppia). * craeted image parser * ObservableViewModel * Load exploration and open ExplorationActivity * Test case for load exploration * fetch exploration * Connection with StateFragment * update * update * working on fetching customization_args * Update StateFragmentPresenter.kt * Separated this in its own PR * Delete UrlImageParser.kt Separated this in its own PR. * Update StateFragmentPresenter.kt * Update build.gradle * Create HtmlParser.kt * Update HtmlParser.kt * Update HtmlParser.kt * Update HtmlParser.kt * working on testcases * Update HtmlParserTestActivty.kt * Update HtmlParserTestActivty.kt * added testcases. * Update AndroidManifest.xml * Update InteractionAdapter.kt * Update InteractionAdapter.kt * Update HtmlParser.kt * corrected variable names. * updated variables. * Update HtmlParser.kt * Update HtmlParser.kt * Update HtmlParserTestActivty.kt * Update HtmlParser.kt * Delete InteractionAdapter.kt * deteted unused xml files * merged html code. * Update HtmlParser.kt * Delete UrlImageParserTest.kt * used dagger injection for UrlImageParser. * Update HtmlParserTestActivty.kt * Update HtmlParserTestActivty.kt * Update HtmlParser.kt * Update HtmlParserTestActivty.kt * updated tests * updated test * moved to testing package * Update HtmlParserTestActivty.kt * Update HtmlParserTestActivty.kt * fix issues * fix issues. * fixed issues * fixed issues. * Update activity_test_html_parser.xml * fixes * Update HtmlParserTest.kt * Update HtmlParserTest.kt * fixed issues. * fixing issues. * made use of factory * Update HtmlParserTest.kt * Update UrlImageParser.kt * Update AndroidManifest.xml * Update HtmlParserTestActivity.kt * nit * Update AudioFragmentTest.kt * updated * fixes * Update UrlImageParser.kt * Not used in this. * fixes * Not used. * Update CellularDataDialogFragmentTest.kt * reverted * Update CellularDataDialogFragmentTest.kt * fixes * Update ImageLoader.kt * Update HtmlParser.kt * Update HtmlParser.kt * Update build.gradle * fixed issues. * Update UrlImageParser.kt * Update UrlImageParser.kt * Update UrlImageParser.kt * Update ImageParsingAnnotations.kt * working on image loader * Injected ImageLoader * updated image loader. * Update ActivityComponent.kt * Update HtmlParserTestActivity.kt * Update GlideImageLoader.kt * removed annotation * Update HtmlParser.kt * line break * fix * fix comments * Update UrlImageParser.kt * Update UrlImageParser.kt * injected context * working on testcase. * testcase added * added test case * testcases * Update build.gradle * Update AndroidManifest.xml * fixes * Update UrlImageParserTest.kt * changed to CutomTarget * Update UrlImageParserTest.kt * updated. * Update UrlImageParserTest.kt * Update HtmlParserTestActivity.kt * Update misc.xml * Update GlideImageLoaderModule.kt * updated with image parser * Update StateFragmentContentCardTest.kt * Update HtmlParserTest.kt * deleted test class for loading image and added todo * updated * Update build.gradle * Update GlideImageLoader.kt * Update build.gradle * Update UrlImageParser.kt * updated. * update * update * Update StateFragmentPresenter.kt * ContentCardTestActivity introduced * updated test. * Update AndroidManifest.xml * Update AndroidManifest.xml * Update StateFragmentContentCardTest.kt * renamed file. * Update state_fragment.xml * moved test case to stateFragmentTest * Update StateFragmentTest.kt * changes. * Update content_item.xml * Update ContentViewModel.kt * Update StateFragmentTest.kt * KDoc added
* Introduce first pass interface for ExplorationProgressController. * Fill in the stubbed logic for ExplorationProgressController. Still no tests to verify correctness. Also, added a method to facilitate notifying of DataProvider changes on the UI thread. * Fix lateinit issue in ExplorationProgressController due to wrongly ordered initialization. * Fix a variaty of issues in the exp progress controller, properly hook it up to the data controller, and start adding tests. * Created a separate ExplorationRetriever, hooked up AnswerClassificationController, and attempted to make ExplorationProgressController thread-safe. The thread-safety led to significant interface changes in the progress controller, and led to discovering some issues with the mediator live data approach to interop coroutines and LiveData. This locking mechanism will need to change since the optimal solution requires resolving #90. * Change the locking mechanism for ExplorationProgressController to work with the current MediatorLiveData implementation (see #90 for more context). Fix existing progress controller tests and add a few more. All current progress controller tests are passing. * Finish tests for ExplorationProgressController and add test classification support for the second test exploration (about_oppia). * craeted image parser * ObservableViewModel * Load exploration and open ExplorationActivity * Test case for load exploration * fetch exploration * Connection with StateFragment * update * update * working on fetching customization_args * Update StateFragmentPresenter.kt * Separated this in its own PR * Delete UrlImageParser.kt Separated this in its own PR. * Update StateFragmentPresenter.kt * Update build.gradle * Create HtmlParser.kt * Update HtmlParser.kt * Update HtmlParser.kt * Update HtmlParser.kt * working on testcases * Update HtmlParserTestActivty.kt * Update HtmlParserTestActivty.kt * added testcases. * Update AndroidManifest.xml * Update InteractionAdapter.kt * Update InteractionAdapter.kt * Update HtmlParser.kt * corrected variable names. * updated variables. * Update HtmlParser.kt * Update HtmlParser.kt * Update HtmlParserTestActivty.kt * Update HtmlParser.kt * Delete InteractionAdapter.kt * deteted unused xml files * merged html code. * Update HtmlParser.kt * Delete UrlImageParserTest.kt * used dagger injection for UrlImageParser. * Update HtmlParserTestActivty.kt * Update HtmlParserTestActivty.kt * Update HtmlParser.kt * Update HtmlParserTestActivty.kt * updated tests * updated test * moved to testing package * Update HtmlParserTestActivty.kt * Update HtmlParserTestActivty.kt * fix issues * fix issues. * fixed issues * fixed issues. * Update activity_test_html_parser.xml * fixes * Update HtmlParserTest.kt * Update HtmlParserTest.kt * fixed issues. * fixing issues. * made use of factory * Update HtmlParserTest.kt * Update UrlImageParser.kt * Update AndroidManifest.xml * Update HtmlParserTestActivity.kt * nit * Update AudioFragmentTest.kt * updated * fixes * Update UrlImageParser.kt * Not used in this. * fixes * Not used. * Update CellularDataDialogFragmentTest.kt * reverted * Update CellularDataDialogFragmentTest.kt * fixes * Update ImageLoader.kt * Update HtmlParser.kt * Update HtmlParser.kt * Update build.gradle * fixed issues. * Update UrlImageParser.kt * Update UrlImageParser.kt * Update UrlImageParser.kt * Update ImageParsingAnnotations.kt * working on image loader * Injected ImageLoader * updated image loader. * Update ActivityComponent.kt * Update HtmlParserTestActivity.kt * Update GlideImageLoader.kt * removed annotation * Update HtmlParser.kt * line break * fix * fix comments * Update UrlImageParser.kt * Update UrlImageParser.kt * injected context * working on testcase. * testcase added * added test case * testcases * Update build.gradle * Update AndroidManifest.xml * fixes * Update UrlImageParserTest.kt * changed to CutomTarget * Update UrlImageParserTest.kt * updated. * Update UrlImageParserTest.kt * Update HtmlParserTestActivity.kt * Update misc.xml * Update GlideImageLoaderModule.kt * updated with image parser * updated * Update StateRetriever.kt * Create StateSelectionInteractionTest.kt * Configuration change save state * Update StateSelectionInteractionTest.kt * Update StateSelectionInteractionTest.kt * Update StateSelectionInteractionTest.kt * Update StateFragmentContentCardTest.kt * Update HtmlParserTest.kt * deleted test class for loading image and added todo * updated * Update build.gradle * Update GlideImageLoader.kt * Update build.gradle * Update UrlImageParser.kt * updated. * update * update * Update StateFragmentPresenter.kt * ContentCardTestActivity introduced * updated test. * Update AndroidManifest.xml * Update AndroidManifest.xml * Update StateFragmentContentCardTest.kt * Changed test activity. * renamed file. * Update state_fragment.xml * moved test case to stateFragmentTest * working on test * changes in activity. * Update StateFragmentTest.kt * changes. * Update content_item.xml * Update ContentViewModel.kt * Update StateFragmentTest.kt * updated tests. * Update HomeFragmentPresenter.kt * Update SelectionInteractionCustomizationArgsViewModel.kt * Update home_fragment.xml * Update build.gradle * Update state_fragment.xml * Update StateFragmentTest.kt * KDoc added * Test cases optimised * nit change * Update StateFragmentTest.kt * Update ItemClickListener.kt * Update StateRetriever.kt * Update ExplorationRetriever.kt * Update ExplorationRetriever.kt * fixed issues.
I think #928 resolves this, though the deferred cases will probably need to be considered later once we have an opportunity to test those (there are other issues related to that that may cause issues, like coroutine cancellation). |
* Initial introduction of test coroutine dispatchers to replace Kotlin's test coroutine dispatcher. This includes introducing a test-only module to contain testing dependencies. * Introduce a new LiveData mechanism to bridge coroutines from DataProviders in a way that doesn't have the same limitations as the previous MutableLiveData-based bridge. * Early work at introducing FakeSystemClock tests (not yet complete). * Remove infeasible testing structures, add documentation, and clean up implementation to prepare for code review. * Add notice that the dispatchers utility is temporary. * Cleanup new LiveData bridge, add tests for it, and migrate other DataProviders tests to using TestCoroutineDispatchers utility. * Add AsyncResult tests, fix & re-enable an earlier test in PersistentCacheStoreTest, and fix FakeSystemClock so that it works properly in test environments. * Use ktlint to reformat TestCoroutineDispatchers per reviewer comment thread. * Reformat files failing linter check. * Address reviewer comment.
See AsyncDataSubscriptionManager and NotifiableAsyncLiveData for context. NotifiableAsyncLiveData currently relies on MediatorLiveData to chain CoroutineLiveDatas together to properly pipe coroutine-fed data to the UI. However, adding and removing sources from MediatorLiveData must be done on the main thread since the internal sources data structure is not thread-safe, and it calls through to LiveData's observeForever() method which must be called on the main thread.
This requires triggering DataProvider changes on the main thread, which is definitely not ideal. There are two possible alternatives:
The latter solution seems preferable since it minimizes UI thread hops, but it does result in creating a custom long-lived CoroutineLiveData.
The text was updated successfully, but these errors were encountered: