Skip to content
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

Common usage of CoroutineScope Job functions are difficult to understand #934

Closed
ZakTaccardi opened this issue Jan 14, 2019 · 9 comments
Closed
Labels

Comments

@ZakTaccardi
Copy link

One problem we have on our team when onboarding developers to Coroutines is the complex access to the Job in a CoroutineScope - specifically scope.coroutineContext[Job]!!. Access to methods like .invokeOnCompletion, cancel, etc are unwieldy to use because of the Job nullability and violation of the Law of Demeter.

// easy to use/understand
scope.invokeOnCompletion { }
scope.cancel {  }

// difficult to understand
scope.coroutineContext[Job]!!.invokeOnCompletion { }
scope.coroutineContext[Job]!!..cancel { }

The latter is more difficult because you have to understand that a CoroutineScope is just a wrapper around a CoroutineContext that will always have a Job (even though the API says a Job can be null). And once it is undestood, it's a pretty verbose API.

Possible Solutions

I see two possible solutions going forward (though both could be implemented).

  1. Put commonly used functions like .cancel(), invokeOnCompletion{ }, etc as extension functions on CoroutineScope.
  2. Add an extension function scope.job that returns an non-null Job - because a Job should never be null for a CoroutineScope.

note: scope.cancel() was added in #866

@qwwdfsad
Copy link
Member

We have been thinking about this for a while.

The problem with a lot of extensions of CoroutinesScope is its mental model for users that a new to kotlinx.coroutines. When scope API surface duplicates Job one, it becomes a source of confusion "why should I use both" and unclear semantics. While standalone .job extension may be useful for advanced usages, we are still not sure. Could you please elaborate what Job API are you using apart from invokeOnCompletion?

This issue is more about design of the mental model and clear understanding of Job and CoroutineScope concepts in the library, implementation is straightforward and trivial, so if you need it right now you can introduce an extension property/method in your project.

@ZakTaccardi
Copy link
Author

what Job API are you using apart from invokeOnCompletion?

We are using:

job.cancel()
job.cancelAllChildren()
job.invokeOnCompletion()
job.isCompleted // rare
job.isActive // rare
job.cancelAndJoin() // rare

Could you please elaborate what Job API are you using apart from invokeOnCompletion

I'm currently using it to signal a ViewModel.onCleared() event, instead of relying on extending the android arch components ViewModel

While standalone .job extension may be useful for advanced usages, we are still not sure.

Ultimately, I think it's important to understand that the handle on coroutines created from a CoroutineScope is a Job. Is it safe to assume that all CoroutineScope instances should have a Job associated with this?

I don't think it's very important (for beginners) to understand that a Job lives inside a CoroutineContext. In most common usage - the interesting bits of using a CoroutineScope are what dispatcher is used, and being able to control the scope's lifecycle via Job. I think this is an argument in favor of a standalone .job extension.

val scope = CoroutineScope(Dispatchers.Main) // specify the dispatcher
// control lifecycle
scope.job.invokeOnCompletion { }

The above is simple to onboard beginner devs to. The following is not:

val scope = CoroutineScope(Dispatchers.Main)
scope.coroutineContext[Job]!!.invokeOnCompletion {  }

It requires a dev to understand that:

  1. A Job is a special part of a CoroutineContext
  2. Use the Job.Companion as a Key<Job> for accessing the Job instance of a CoroutineContext
  3. Job should never be null in a CoroutineScope, so it is safe to force unwrap it (!!)
  4. a Job will automatically be created for you if you don't specify it

Not requiring a dev to understand those things at the very beginning for the very basics would be a big win

@qwwdfsad
Copy link
Member

Even if we ignore the mental model and discoverability issues, I am afraid that introducing job property will be a source-breaking change.

E.g.

class Activity() : CoroutineScope by CoroutineScope(EmptyCoroutineContext) {
   
  val job = Job()

  suspend fun foo() {
    withContext(Dispatchers.IO) {
      println("Cancelling $job")
      job.cancel()
    }
  }

}

If we introduce job extension, foo semantics will silently change.

We can workaround it with LowPriorityInOverloadResolution, but it's still a subtle topic

@elizarov
Copy link
Contributor

I'd use this opportunity to take a better look at Job use-cases and cover the missing ones with CoroutineScope. I see two main missing pieces that require a Job here:

  • cancelChildren() -- this is easy to add. The only problem with it is that, in general, this function is extremely hard to use right in a race-free manner. Can you elaborate what are use-cases for this particular function?

  • invokeOnCancellation { ... } -- this is quite an advanced low-level function, because, for one thing, it does not specify which context it is invoked in, so it is easy to use incorrectly in a non-thread-safe manner. Can you give a bit more of example of how you use it, please. We might be able to design a safer alternative that would be a better fit for CoroutineScope extension.

@ZakTaccardi
Copy link
Author

ZakTaccardi commented Feb 27, 2019

  • invokeOnCancellation { ... } -- this is quite an advanced low-level function

Note: I'm using job.invokeOnCompletion { }, not invokeOnCancellation { }

Can you give a bit more of example of how you use it, please.

Sure. Consider how the Android ViewModel has the onCleared() callback. This is equivalent to the view model's "scope" being cancelled. Therefore, I have a custom ViewModel implementation that does not have an onCleared() callback. Instead, I leverage the scope it's injected with to handle additional cancellation that isn't already covered by a CoroutineScope being cancelled.

class MyViewModel(private val scope: CoroutineScope) {

    init {
        scope.coroutineContext[Job]!!.invokeOnCompletion {
        // clean up anything that would need to be cleaned up in response to `onCleared()`
    }
}

@ZakTaccardi
Copy link
Author

ZakTaccardi commented Feb 27, 2019

cancelChildren() -- this is easy to add. The only problem with it is that, in general, this function is extremely hard to use right in a race-free manner. Can you elaborate what are use-cases for this particular function?

I actually don't use that function at all for the race condition reasons. Some other members of my team like it because it gives them cleanup at a low level of effort. They use it to cancel network requests that are running at a "global" scope. (think like a CoroutineScope that is scoped to an Android Application lifecycle, which is effectively a singleton)

There's also this example

@elizarov
Copy link
Contributor

Sure. Consider how the Android ViewModel has the onCleared() callback. This is equivalent to the view model's "scope" being cancelled. Therefore, I have a custom ViewModel implementation that does not have an onCleared() callback. Instead, I leverage the scope it's injected with to handle additional cancellation that isn't already covered by a CoroutineScope being cancelled.

What kind of cleanup you'd typically do in a ViewModel using invokeOnCompletion this way? Does it require manipulating UI?

As a side note, the original design was based on architecture in which ViewModel is a scope or owns a scope instead of gets injected with scope. It makes a big different with respect to cleanup, because when view model owns a scope it has its own clear/close/cancel function that does all cleanup, like this:

class MyViewModel {
    // I own the scope, as opposed to somebody else owning the scope and inject it to me
    private val scope = MainScope() 

    fun clear() {
        scope.cancel()
        // clean up anything that would need to be cleaned up in response to `onCleared()`
    }
}

So my other question would be if you have to inject a scope into the view model and why.

@ZakTaccardi
Copy link
Author

What kind of cleanup you'd typically do in a ViewModel using invokeOnCompletion this way?

Because I use the injected CoroutineScope to do all my cleanup, it happens automatically 👍 . The only reason to use scope.job.invokeOnCompletion { .. } would be to do non-coroutine cleanup (.clear() a CompositeDisposable or something).

Does it require manipulating UI?

No. They just expose a ReceiveChannel<State> (soon to be Flow<State> 😄 )

when view model owns a scope it has its own clear/close/cancel function that does all cleanup

onCleared() is a cancellation mechanism, but so is CoroutineScope. I don't see the reason to have both. This has some nice benefits when it comes to testing. For example, take the injected scope view model below:

class MyViewModel(scope: CoroutineScope) : ViewModel, CoroutineScope by scope { .. }

My BaseUnitTest class exposes a CoroutineScope that is cancelled when @After fun tearDown() is called. That is the CoroutineScope that MyViewModel is injected with during testing, so I get the automatic cleanup for free (though it should get GC'd anyway). There becomes no need to manually call viewModel.onCleared(), because it's injected with a scope that gets cancelled at the appropriate time.

// I own the scope, as opposed to somebody else owning the scope and inject it to me

I would actually disagree - it's whoever calls onCleared() who owns the scope. This is effectively the same as injected a CoroutineScope that gets cancelled appropriately.

@qwwdfsad
Copy link
Member

Was fixed in #2159, the other part is coming in #3259

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants