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

CoroutineScope.cancel() extension to reduce boiler-plate in a typical UI app #671

Closed
elizarov opened this issue Oct 6, 2018 · 8 comments

Comments

@elizarov
Copy link
Contributor

elizarov commented Oct 6, 2018

I'm reading Android codelabs on coroutines here https://codelabs.developers.google.com/codelabs/kotlin-coroutines/index.html#4

It gave me a fresh look on the pattern we are advertising for CoroutineScope. There it is being cast without inheritance:

private val viewModelJob = Job()
private val uiScope = CoroutineScope(Dispatchers.Main + viewModelJob)

override fun onCleared() {
    super.onCleared()
    viewModelJob.cancel()
}

This does look like bit a boilerplate, even if you have to write it once in your app. What we can do, is to have a cancel() extension function directly on CoroutineScope.

PRO: It is going to simplify the above code (note that CoroutineScope(...) constructor always creates a Job object):

private val uiScope = CoroutineScope(Dispatchers.Main)

override fun onCleared() {
    super.onCleared()
    uiScope.cancel()
}

CON: We have a convention of declaring "coroutine builder" functions as extensions on CoroutineScope. It mean, that if we have cancel() extension on CoroutineScope, then it will become available as cancel() in the scope of those coroutine builder functions, which could be confusing and is completely useless (at least, I don't see a use-case for cancel() in the scope of coroutine builder). I don't have a nice solution for this downside.

Any comments?

@fvasco
Copy link
Contributor

fvasco commented Oct 6, 2018

Something like coroutineContext property

val CoroutineScope.currentJob : Job
    get() = TODO()

Or a better name

@elizarov
Copy link
Contributor Author

elizarov commented Oct 8, 2018

@fvasco Thanks. Something like currentJob might be good solution for the CON side. If we do it this way, then the resulting code will look like this:

private val uiScope = CoroutineScope(Dispatchers.Main)

override fun onCleared() {
    super.onCleared()
    uiScope.currentJob.cancel() // currentJob ???
}

Maybe currentJob does not clearly show an intent in this case and may be confusing, though. As an alternative we can name it coroutineJob. There is also a coroutineContext property in CoroutineScope, so it plays nicely with it.

@fvasco
Copy link
Contributor

fvasco commented Oct 8, 2018

I agree with you, @elizarov

In the CoroutineScope using currentJob works well, outside should be used a better name.

You may consider to deprecate val CoroutineScope.isActive: Boolean and use a more explicit coroutineJob.isActive.

@objcode
Copy link
Contributor

objcode commented Oct 8, 2018

This is an interesting proposal, and I think it's a good idea to help minimize this boilerplate.

Update: Replaced my reply after considering da16bc2.

CoroutineScope.cancel() shows up in too many places

The blocks for launch, async, and coroutineScope will all have cancel() available in a way that's going to be confusing for new developers. Calling cancel() from inside a launch {} block will cancel all other coroutines started in the scope, which is confusing.

Job as nullable property on CoroutineScope

The API currently available for cancellation via implicit jobs is

fun onCleared() {
     super.onCleared()
     uiScope.coroutineContext.cancel()
}

Reviewing the updated API with coroutineJob (or similar generic names) I'm not sure it'll improve readability in the absence of further abstractions:

fun onCleared() {
     super.onCleared()
     uiScope.coroutineJob.cancel()
}

Subclass CoroutineScope

This is what I expect most developers to do if they follow the composition pattern shown in the code lab. Without modifying the API of CoroutineScope, one can create a CancelableCoroutineScope class that's a subclass and use that in any place that needs a Joblike scope. This class might look like this:

class CancelableCoroutineScope(context: CoroutineContext): CoroutineScope {
    
    private val job = context[Job] ?: Job()
    override val coroutineContext: // ...

    fun cancel() {
        job.cancel()
    }
}

usage:

val uiScope = CancelableCoroutineScope(Dispatchers.Main)

fun onCleared()  {
    super.onCleared()
    uiScope.cancel() // only available in the subclass
}

@objcode
Copy link
Contributor

objcode commented Oct 8, 2018

I wonder, can inline classes allow for extending an API like this?

@elizarov
Copy link
Contributor Author

elizarov commented Oct 8, 2018

@objcode CancellableCoroutineScope can be an inline class, indeed. However, it kind of misnomer, since every instance of CoroutineScope should be cancellable, since we already have code to create a Job() in a regular CoroutineScope(...) constructor function.

I've looked at the usage via coroutineContext that you've proposed and it already seems quite good for me. Maybe we shall just update codelab to use this pattern and stop trying to further improve it:

private val uiScope = CoroutineScope(Dispatchers.Main)

override fun onCleared() {
    super.onCleared()
    uiScope.coroutineContext.cancel() // is it confusing or Ok? 
}

@objcode
Copy link
Contributor

objcode commented Oct 8, 2018

After considering this for a while I think any new APIs here are not going to improve the code above coroutineContext.cancel() The current coroutineContext.cancel() API is a reasonable shortcut and any new API shows up in strange places. I'll add a callout in the codelab to make sure that this is clear.

For ViewModels specifically, I think we should be able to improve the API inside the ViewModel library code. The codelab is intentionally avoiding introducing any abstractions not shipped in a library (including the obvious abstraction of introducing a common ViewModel parent that manages the CoroutineScope).

Taking a look at the code, I think the explicit viewModelJob currently in the codelab is the easiest currently available option for someone new to coroutines/scopes. As stable libraries are introduced that encapsulate cancellation for ViewModel, I'll update the codelab to use them!

How does this sound to you:

  • Adding a API is not likely to help, scope.coroutineContext.cancel() is reasonable for one off scopes (e.g. in a BroadcastReceiver)
  • I'll add a callout explaining the implicit parent job
  • When stable libraries support this for ViewModel will update the codelab again

@elizarov
Copy link
Contributor Author

elizarov commented Oct 8, 2018

LGTM. I'll close this issue.

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

No branches or pull requests

3 participants