Consider deprecating or changing the behaviour of CoroutineContext.isActive #3300
Closed
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 aJob
. An absence of replacement can be too disturbing as a lot of code rely on a perfectly finectxWithJob.isActive
- Code that relies on
.job.isActive
no longer can be called from such entry points safely
- Its only possible replacement is
- Change the default behaviour -- return
true
. It also "fixes" such patterns asGlobalScope.isActive
but basically is a breaking change
Activity
joffrey-bion commentedon May 23, 2022
GlobalScope.isActive
is not the same property, and already defaults totrue
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 thanCoroutineContext.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).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 theCoroutineScope.isActive
property from that scope (which would now have a job). Or just useyield()
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.Change the contract of CoroutineContext.isActive to return 'true' for…
Change the contract of CoroutineContext.isActive to return 'true' for…