I'm doing migrate to coroutines refactoring in and...

# coroutines

[plastiv]

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?

[gildor]

I think it doesn’t make a lot of sense to use callbacks and join for launch

[gildor]

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

[gildor]

> request.loadDataFromNetwork() // Blocking retrofit network call.

This is really bad idea

[gildor]

I really would recommend to use async API of Retrofit + coroutines adapter

[gildor]

you current code has one really bad thing: it’s not cancellable because you use blocking API and you do not handle coroutine cancellation

[gildor]

Intead you can remove all this code and just call this in your CoroutineScope:

launch {
  try {
     val result = request.await() 
  } catch (…) {
     // handle error
  }
}

[plastiv]

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?

[gildor]

Yes, use coroutines-retrofit adapter

[gildor]

From Jake or from me 🙈

[gildor]

But my works only with Call, not with Deferred, just up to you what you use in your project

[gildor]

https://github.com/JakeWharton/retrofit2-kotlin-coroutines-adapter
https://github.com/gildor/kotlin-coroutines-retrofit

Both of them will be deprecated when this PR will be merged
https://github.com/square/retrofit/pull/2886

[plastiv]

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?

[gildor]

Okay, let’s split this to a few separate problems

[gildor]

This class is just some sort of adapter between callbacks and coroutines

[gildor]

I think it’s wrong strategy for migration

[gildor]

and you should use coroutines directly

[gildor]

“directly” I mean that you launch coroutine and call suspend functions, such as await() on you networking

[gildor]

Of course, you can move this from Activity to Presenter, Interactor, ViewModel or any other business logic abstraction

[gildor]

but my point, that this abstraction should support lifecycle if you want to use any aync operation

[gildor]

and if something have lifecycle, just implement CoroutineScope by this abstraction or pass it to constructor (scope has lifecycle itself)

[gildor]

And, finally, if you have scope you can safely start coroutine using launch and use any coroutines API

[plastiv]

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 😉)

[gildor]

I just think this migration is just bad idea, I would use old implementation for existing screens

[gildor]

(yes it’s not fully performant of course, like cancellation is not propagated to okhttp

But you can easily fix this

[gildor]

this is not only about perfomance, your code is leaking