oppia / oppia-android

A free, online & offline learning platform to make quality education accessible for all.
https://www.oppia.org
Apache License 2.0
315 stars 517 forks source link

Retrofit needs to have proper threading controls #2951

Open BenHenning opened 3 years ago

BenHenning commented 3 years ago

Retrofit needs:

  1. To use our threading dispatchers
  2. Needs to have proper adaption with DataProvider (preferably in a way where no one ever interacts with Retrofit directly to avoid accidentally introducing synchronous calls)
BenHenning commented 3 years ago

/cc @jcqli

BenHenning commented 3 years ago

I dug into this a bit more last night and the findings are mixed.

Good news Retrofit apparently leans on OkHttp for asynchronous threading management, but does perform callback invocation in background threads (which is configurable by passing in a custom ExecutorService). OkHttp lets us configure the threads used for asynchronous processing by passing in a custom dispatcher class (which uses ExecutorService under the hood). Fortunately, we also leverage ExecutorServices as the base concurrency primitives for all of our coroutine dispatchers making it easy to centralize our threading across OkHttp, Retrofit, and the rest of the app (only Glide won't be part of this since they intentionally use their own fine-tuned ExecutorServices).

Not-so-good news We expect all threading controls to go through Kotlin coroutines which means we have no way currently to synchronize threading across the Java world with ExecutorServices and the Kotlin world with coroutines. There are two options to fix this:

  1. Introduce a means to use coroutines in Java (e.g. make our existing CoroutineExecutorService used for Glide interop production-ready)
  2. Change our test synchronization mechanism to instead synchronize on ExecutorService and allow the rest of the app to have access to the underlying ExecutorServices used in the coroutine dispatchers

(1) seems particularly scary since coroutines are difficult to make block, and ExecutorService has blocking APIs that must be implemented (see warning at the top of CoroutineExecutorService & the implementation for a lot more details on this). This has resulted in very challenging situations that aren't obvious to fix. Moving toward an ExecutorService-based synchronization model will make a lot more sense (since then we're coordinating all threading to a single ExecutorService rather than via a wrapper ExecutorService through corotouines to an ExecutorService). This will also nicely let us replace the existing CoroutineExecutorService when testing Glide.

While (2) seems nice, it has some drawbacks: