When dealing with a lifecycle-aware component, such as a Presenter, where you may have repeated `onStart`/`onStop` toggling, is it okay to use

cancelChildren()

while never explicitly cancelling the parent

Job

itself? This way when

onStart()

is called, you can continue reusing the

Job

for launching more coroutines?

seems like a micro optimization to me

I have. But typically we move the logic away from the Activity. New code is using Android's

ViewModel

so it's pretty straightforward to override

onCleared()

and cancel the entire parent

Job

. But we have a lot of older code that is following MVP with our own definition of Presenters. These presenters only have an

onTake

and

onDrop

callback which are tied to the

onStart()

and

onStop()

events of the Activity.

Actually, it is a

val

property, so it get’s set once, unless the activity is destroyed anyways

Just ran the following test:

fun main(args: Array<String>) {
    val person = Person()
	println(person.name)
    println(person.name)
    println(person.name)
}

class Person {
    var counter = 0
    val name: String get() = "User id ${counter++}"
}

And the results were:

User id 0
User id 1
User id 2

Woops, read that wrong

Which is why in your post, I believe

onPause

is creating a new

SupervisorJob

instance and then immediately invoking

cancel

on it, essentially leaking the previous one.

Yep, need to fix that

That's most likely why pausing and then resuming the Activity still seems to be working as far as successfully launching coroutines.

So I did a quick test with the following

email_sign_in_button.onClick(coroutineContext) {
//            clickEventChannel.send(SignInClickEvent.SignInButton)
            val ctx1 = coroutineContext
            val ctx2 = coroutineContext

            println(ctx1)
            println("=======")
            println(ctx2)

        }

Hmm... 🤔

I think I’ve made myself even more confused tbqh

😆

For your actual question, I imagine that if all the children are already canceled then destroying the

Job

is fine

No problem. Essentially, the Activity will be destroyed where the parent

Job

is still

active

, however all of it's children will be cancelled.

Nevermind, I did some more experimenting and every call to

coroutineContext

, using the correct property, returns a new context

Ah ok.

I did a bunch more testing and your solution of

cancelChildren

seems a good way to go, I need to update my examples to use it

I'm still not 100% sure though if it is okay to keep the parent

Job

as active if we never call

cancel

on it though. I'd like to think it is okay. 🤔

I think we could also cancel it in onDestroy

Typically, our presenters don't expose an

onDestroy

. As mentioned before, with RxJava, we use

compositeDisposable.clear()

instead of

compositeDisposable.dispose()

and leave the

CompositeDisposable

as undisposed.

Oh sorry, I didn't realize you're cancelling the jobs in the presenter itself.

Yeah. Trying to integrate Coroutines into legacy MVP code that sorta missed the Jetpack train when it was written 😞

Looking at the structured concurrency example it seems that for each invocation of

coroutineContext

, a new

SupervisorJob

instance will be created.

No, it shouldn’t work like that, you should keep Job until scope cancellation and recreate your job if some onStart method may be called a few times

Because I've been uncertain as to whether it is okay to only

cancelChildren()

, I've been using approach similar to the following:

private var scope by Delegates.notNull<CoroutineScope>()

fun onStart() {
    scope = CoroutineScope(SupervisorJob() + Dispatchers.Main)
    haveFunAndLaunchSomeCoroutines()
}

fun onStop() {
    scope.cancel()
}

yes, you can use this too

I personally prefer the

cancelChildren()

approach since you get to use a

val

and avoid the initialization in the

onStart

.

possible problem with cancelChildren() and with CompositeDisposable.clean() is that there is a chance to leak some operation that started after onStop()

Good point! Fortunately, we avoid performing asynchronous tasks like this after

onStop()

.

Yes, everyone tries to avoid it, but I saw a bunch of error related to this, for example even some actions like click on view are asyncronous so you can get callback after onStop

Yep you're right @gildor I misspoke