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.

Sign up for GitHub

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

Jump to bottom

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

Closed
qwwdfsad opened this issue May 23, 2022 · 1 comment
Closed

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

qwwdfsad opened this issue May 23, 2022 · 1 comment
Labels
breaking change design enhancement for 1.7

Comments

@qwwdfsad
Copy link
Collaborator

qwwdfsad commented May 23, 2022 •
edited
Loading

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
@joffrey-bion
Copy link
Contributor

joffrey-bion commented May 23, 2022 •
edited
Loading

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.

qwwdfsad added a commit that referenced this issue Feb 23, 2023
@qwwdfsad
Change the contract of CoroutineContext.isActive to return 'true' for…
9643c70 
… contexts with no job in it.

Otherwise, the API is becoming an error prone for being called from jobless entrypoints (i.e. 'suspend fun main' or Ktor handlers), as 'if (ctx.isActive)' is a well-established pattern for busy-wait or synchronous job.
It is now aligned with CoroutineScope.isActive behaviour.

Fixes #3300
@qwwdfsad qwwdfsad mentioned this issue Feb 23, 2023
Merged
@psteiger psteiger mentioned this issue Dec 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
breaking change design enhancement for 1.7
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@joffrey-bion @qwwdfsad