https://kotlinlang.org logo
Title
p

plastiv

11/07/2018, 3:32 PM
I'm doing migrate to coroutines refactoring in android application. Can someone help me with reviewing next implementation, please? To see if I has any obvious and nonobvious issues. The current idea (I don't want to rewrite whole app and every activity in one MR) is that it will allow to run multiple network requests in parallel between activity onCreate/onDestroy. Results are delivered through callbacks, yes. I know that this is not idiomatic, but I have to start conversion somewhere 😉
open class RetrofitCallManager( // Open because its mocked at some unit tests. 
    val uiDispatcher: CoroutineDispatcher = Dispatchers.Main, // To be able to switch to Unconfined at jvm tests.
    val ioDispatcher: CoroutineDispatcher = <http://Dispatchers.IO|Dispatchers.IO>
) : CoroutineScope {

    private lateinit var parentJob: Job

    override val coroutineContext: CoroutineContext
        get() = parentJob + uiDispatcher

    fun onCreate() {
        parentJob = SupervisorJob() // Job is supervised so one failed child coroutine wouldn't cancel other running coroutines.
    }

    open fun <T> execute(request: NetworkRequest<Result<T>>, listener: ResultListener<T>) = launch { // Launch returns Job which could be joined inside test runBlocking.
        try {
            val result = withContext(ioDispatcher) {
                request.loadDataFromNetwork() // Blocking retrofit network call.
            }
            if (result.isSuccess) {
                listener.onResult(result.data()!!) // Update ui.
            } else {
                listener.onError(result.error()!!) // Update ui.
            }
        } catch (e: CancellationException) {
            throw e // No need to propagate to UI since execution was cancelled by UI.
        } catch (e: Exception) {
            listener.onError(LocalError.create(ERR_CODE, e)) // Update ui.
        }
    }

    fun onDestroy() {
        parentJob.cancel()
    }
}
Is this something you would expect to see in android app?
g

gildor

11/08/2018, 1:34 AM
I think it doesn’t make a lot of sense to use callbacks and join for launch
Even not sure that you need this class in general I don’t know details of your architecture of course, but I think it doesn’t help with migration and instead make sense to implement coroutine scope on your UI component where you already have onCreate/onDestroy or LifecycleProvider, so no need to write all this boilerplate And instead of execute write
await()
extension fro your NetworkRequest
> request.loadDataFromNetwork() // Blocking retrofit network call.
This is really bad idea
I really would recommend to use async API of Retrofit + coroutines adapter
you current code has one really bad thing: it’s not cancellable because you use blocking API and you do not handle coroutine cancellation
Intead you can remove all this code and just call this in your CoroutineScope:
launch {
  try {
     val result = request.await() 
  } catch (…) {
     // handle error
  }
}
p

plastiv

11/08/2018, 9:31 AM
Thank you for checking! Just to make things clear for me which retrofit async API are you mentioning? Afaik, okhttp is still using blocking io. You mean just to delegate managing of thread pools to okhttp itself by using
call.enqueue(Callback
through coroutines-retrofit adapter from Jake?
g

gildor

11/08/2018, 9:36 AM
Yes, use coroutines-retrofit adapter
From Jake or from me 🙈
But my works only with Call, not with Deferred, just up to you what you use in your project
👍 1
p

plastiv

11/08/2018, 10:01 AM
Nice 👍 Haven't seen MR for the retrofit before.
Even not sure that you need this class in general
I don’t know details of your architecture of course, but I think it doesn’t help with migration and instead make sense to implement coroutine scope on your UI component where you already have onCreate/onDestroy or LifecycleProvider, so no need to write all this boilerplate
Actually I was wondering about this. I'm just starting with coroutines but if I learned anything in 8 years of doing android development its is that putting everything into activities is really bad idea. Leave ui for the ui and extract everything else. Why UI component should control/know what execution mechanism app is using? Is it really considered to be a best practice?
g

gildor

11/08/2018, 10:02 AM
Okay, let’s split this to a few separate problems
This class is just some sort of adapter between callbacks and coroutines
I think it’s wrong strategy for migration
and you should use coroutines directly
“directly” I mean that you launch coroutine and call suspend functions, such as await() on you networking
Of course, you can move this from Activity to Presenter, Interactor, ViewModel or any other business logic abstraction
but my point, that this abstraction should support lifecycle if you want to use any aync operation
and if something have lifecycle, just implement CoroutineScope by this abstraction or pass it to constructor (scope has lifecycle itself)
And, finally, if you have scope you can safely start coroutine using launch and use any coroutines API
p

plastiv

11/08/2018, 10:09 AM
I agree about moving to coroutines directly. Wasn't explicit enough with my disclaimer that that's the plan and current impl is a step#1 which allows me to use coroutines without changing any of 30+ screens or up to 50 network calls (not that I'm not using retrofit call directly but through my "requests" classes which are command pattern with some post process logic). That is the beauty of not coupling executing mechanism with UI/activities previously. Now I can switch it with one class only. (yes it's not fully performant of course, like cancellation is not propagated to okhttp, but it was the case with previous impl as well 😉)
g

gildor

11/08/2018, 10:18 AM
I just think this migration is just bad idea, I would use old implementation for existing screens
(yes it’s not fully performant of course, like cancellation is not propagated to okhttp
But you can easily fix this
this is not only about perfomance, your code is leaking