`invokeOnCompletion` doesn't provide thread guarantees and doesn't allow to suspend

[dovchinnikov]

val popupVm: StateFlow<PopupData?> = ... // view model
launch(Dispatchers.Swing) {
  popupVm.collectLatest { data ->
    if (data == null) {
      return@collectLatest
    }
    val popup = createPopup(data)
    currentCoroutineContext().job. invokeOnCompletion { 
      // popup should be hidden once popupVm value is changed
      if (it != null) {
        // this can be executed on Event Dispatch Thread only
        popup.hide()
      }
    }
    popup.show()
  }  
}

Currently, invokeOnCompletion does not provide any guarantees about the dispatcher, so hide is invoked on a random thread.

I cannot do the following because show() might block EDT and spin an inner event loop, thus making it to the awaitCancellation call only until after the popup is closed by the user from the UI:

try {
  popup.show()
  awaitCancellation()
}
finally {
  popup.hide()
}

Currently I have to do this:

coroutineScope {
  val popup = createPopup(data)
  launch {
    try {
      awaitCancellation()
    }
    finally {
      popup.hide()
    }
  }
  popup.show()
}

I'd rather have a convenient invokeOnCompletion (#3259) but with convenient threading and with ability to suspend. Basically, I'm asking for an equivalent of:

fun CoroutineScope.invokeOnCompletion(cleanup: suspend () -> Unit) { 
  launch {
    try {
      awaitCancellation()
    }
    finally {
      withContext(NonCancellable) {
        cleanup()
      }
    }
  }
}

[dovchinnikov]

NB the exact invokeOnCompletion variant from the issue description will prevent the calling scope to complete normally. The expected library function should not prevent the parent scope to complete, but it should invoke its lambda only on cancellation.

[qwwdfsad]

Thanks for the detailed explanation!

I have a few questions about your expectations from such an API:

1) Suspendability support

Do you expect the code from invokeOnCompletion be cancellable on itself? How it is going to be cancellable? Note that invokeOnCompletion is invoked when the original job is already completed, so the completion handler cannot be a child. E.g. consider the handler is suspendable:

job.invokeOnCompletion {
    suspendForever()
}

Do you expect this code to prevent any job from completion? Do you expect it to be cancellable and, if so, how to cancel it?

2) Dispatcher support.

It seems like you expect invokeOnCompletion to inherit its dispatcher from the parent. It opens quite a few questions about liveness and default behaviour, let's consider this scenario:

2.1)

val job = Job()
job.invokeOnCompletion {} 

Where this invokeOnCompletion is going to be invoked?

2.2)

Now a bit trickier:

runBlocking {
    val job = Job(coroutineContext.job)
    job.invokeOnCompletion {} 
}

The same question applies.

2.3) Now consider a much more tricker case:

val executor = Executors.newFixedThreadPoolExecutor(...).asCoroutineDispatcher()

val job = launch(executor) {
    ....
}

job.invokeOnCompletion {
    // NB: this code will be invoked when the job is completed!
   // Will be invoked in executor as the request suggests
}

job.join()
executor.close() // Races with invokeOnCompletion

What is the expected behaviour here?

These questions expose some flaws in the design "let invokeOnCompletion be suspendable and inherit the dispatcher", but I do not expect a sound solution for them; I'm rather interested in your expectations in these examples if the proposed solution is implemented as is, so it can help us to shape the API towards more sound form, while still solving the problems you are solving

[dovchinnikov]

Note that invokeOnCompletion is invoked when the original job is already completed, so the completion handler cannot be a child.

I'd expect the lambda to be executed after the job is in Completing or Cancelling state, before it's Completed or Cancelled.

Do you expect it to be cancellable and, if so, how to cancel it?

I'd expect the lambda to be non-cancellable, It's the same as in a regular completion handler one can block the thread forever, so non-cancellable is ok if properly documented. It's the responsibility of the client to not suspend indefinitely.

If a child job of the lambda fails, then the lambda should be cancelled as well (as per normal cancellation propagation to the parent).

2.1, 2.2, 2.3

I'd expect the completion handler to be executed in a scope (not on a job) inheriting the dispatcher from the scope context (or Dispatchers.Default if unspecified).

[elizarov]

A related problem reported via YouTrack (a use-case of cleaning up resources in a cancelled coroutine): https://youtrack.jetbrains.com/issue/KT-60182

[mgroth0]

Note to self: the above-linked YouTrack issue was my own.