Skip to content

Consider deprecating or changing the behaviour of CoroutineContext.isActive #3300

Closed
@qwwdfsad

Description

According to the doc, isActive has the following property:

The coroutineContext.isActive expression is a shortcut for coroutineContext[Job]?.isActive == true. See Job.isActive.

It means that, if the Job is not present, isActive always returns false.

We have multiple reports that such behaviour can be error-prone when used with non-kotlinx.coroutines entry points, such as Ktor and suspend fun main, because it is inconsistent with the overall contract:

(Job) has not been completed and was not cancelled yet

CoroutineContext.isActive predates both CoroutineScope (which should always have a Job in it, if it's not GlobalScope) and job extension, so it may be the case that it can be safely deprecated.

Basically, we have three options:

  • Do nothing, left things as is. It doesn't solve the original issue, but also doesn't introduce any potentially breaking changes
  • Deprecate CoroutineContext.isActive. Such change has multiple potential downsides
    • Its only possible replacement is this.job.isActive, but this replacement is not equivalent to the original method -- .job throws an exception for contexts without a Job. An absence of replacement can be too disturbing as a lot of code rely on a perfectly fine ctxWithJob.isActive
    • Code that relies on .job.isActive no longer can be called from such entry points safely
  • Change the default behaviour -- return true. It also "fixes" such patterns as GlobalScope.isActive but basically is a breaking change

Activity

joffrey-bion

joffrey-bion commented on May 23, 2022

@joffrey-bion
Contributor

It also "fixes" such patterns as GlobalScope.isActive

GlobalScope.isActive is not the same property, and already defaults to true when there is no job, so this one is not broken:
https://github.com/Kotlin/kotlinx.coroutines/blob/master/kotlinx-coroutines-core/common/src/CoroutineScope.kt#L137

This is why I believe it's often more correct to use CoroutineScope.isActive rather than CoroutineContext.isActive.

Its only possible replacement is this.job.isActive

Why, though? Isn't this[Job]?.isActive == true another perfectly valid replacement that doesn't change behaviour? Or is this what you mean by "absence of replacement" - just inlining the property?

I would also be reluctant in suggesting the .job.isActive replacement. Very often the authors of those pieces of code just didn't think of the jobless case, and here we would push them towards making their code fail even harder in this case (possibly unknowingly).

An absence of replacement can be too disturbing as a lot of code rely on a perfectly fine ctxWithJob.isActive

Without better-looking replacement than the body of the current property getter, users could "fix" their code by wrapping the body of their function with coroutineScope { ... } and accessing the CoroutineScope.isActive property from that scope (which would now have a job). Or just use yield() if they want to cancel a non-suspending loop. Or just remove that condition altogether because they didn't need it in the first place (calling other suspend functions with prompt cancellation guarantees already).

Whether we should deprecate CoroutineContext.isActive is rather a conceptual matter of whether we want to give meaning to this property when there is no job.


TL;DR: I like both the deprecation option and the default change option, but if we deprecate I find the replacement .job.isActive harmful. What I like about the change of default is that even though it's technically breaking, it will probably fix more things than break them, and without any change in the ecosystem but version bumps.

added a commit that references this issue on Feb 23, 2023
9643c70
added a commit that references this issue on Feb 23, 2023
747db9e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

      Consider deprecating or changing the behaviour of CoroutineContext.isActive · Issue #3300 · Kotlin/kotlinx.coroutines