Disallow `Job` passed as context to `withContext`/`launch`/`async`

[dovchinnikov]

What do we have now?

It's possible to pass Job as part of context to the mentioned functions:

withContext(Job()) {
    ...
}
cs.launch(Job()) {
    ...
}
cs.async(Job()) {
    ...
}

What should be instead?

Error should be logged when a Job is detected in the added context.

Why?

All above cases "replace" the context Job and attach the scope Job to the passed Job() as a child. This might be unexpected at best, and plain dangerous because it breaks parent-child relation.

[dovchinnikov]

This is continuation of https://github.com/Kotlin/kotlinx.coroutines/issues/814#issuecomment-1457747716

[dovchinnikov]

So, here is the problem: how do we carefully migrate from today's state to this ideal future state? We need to somehow start giving a warning on any code that tries to pass coroutine context with a job as a parameter to those coroutine builder functions, so that we can later turn this warning into an error. But how do we even identify such code? That is the question.

Let's consider correct use cases, and declare the rest incorrect.

1. NonCancellable

withContext(NonCancellable) {}

NonCancellable is a singleton, withContext can detect and allow this logging a warning with invitation to replace it with nonCancellable {} (TBD name).

2. Orphan jobs x withContext

withContext(Job()) {}
withContext(SupervisorJob()) {}
withContext(CompletableDeferred<Unit>()) {}

Can be detected by checking the parent handle. It is essentially the same as 1 and should be handled as such. withContext does not change the state of the Job, so all three calls behave like nonCancellable {}

3. Orphan jobs x launch/async

cs.launch(Job()) {} 
cs.launch(SupervisorJob()) {} 
... 

launch/async do change the state of the parent Job, so there is a difference between regular and supervisor jobs. This should be offered to be replaced with CoroutineScope(cs.coroutineContext + Job()).launch {} (or supervisor variant), at least this way it's evident that launch-ed coroutine is a child of that new scope.

4. Child of current context x withContext

withContext(Job(parent = currentCoroutineContext().job)) {}

The above example will hang the current coroutine because a child is attached, but never completed.

val job = Job(parent = currentCoroutineContext().job)
try {
  withContext(job) {}
} 
finally {
  job.complete() // or job.cancel()
  job.join()
}

This does not make much sense, coroutineScope {} would be a better alternative. Same for supervisor job.

(To be continued)

[qwwdfsad]

Nice write-up, thanks! I have an idea that should cover most of the reports regarding the inflexibility of structured concurrency. Have to wrap my mind around it to write a proper non-ad-hoc-ish proposal though

[elizarov]

The tricky thing is migration here. I don't think we can just make it into an error one day. It has to start with a warning. Ideally, a compiler-time warning, but it is not obvious how to provide one. A runtime warning can be considered, but it is a tough sell for a core library.

[LouisCAD]

A deprecated error level overload that takes a Job, along with a deprecated warning level that takes NonCancellable only can do for compile time warnings.

It can start with a warning level too, without enforcing it at runtime until after a version raises the warning (for Job) to error level.