Kotlinx.coroutines: Inconsistent cancellation and lost exceptions

[qwwdfsad]

Consider following snippet:

val job = async() {
    barrier.await()
    throw IOException()
}

barrier.await()
job.cancel(TestException())
job.await() // <- 1

1 will always throw IOException and it will have eitherCompletedExceptionally or Cancelled state. TestException is always lost even when cancel call was successful.

Now slightly rewrite it:

val job = async() { 
   barrier.await()
   withContext(wrapperDispatcher(coroutineContext)) { // Just a wrapper to avoid fast paths in implementation
      throw IOException()
   }
}

barrier.await()
job.cancel(TestException())
job.await() // <- 2

Now 2 now can throw JobCancellationException: Job is being cancelled which definitely shouldn't be a terminal exception. Depending on timing it also may lose either TestException or IOException (even when its known that IOException was thrown).

What user may expect
1) When it's known that IOException was thrown, await() should throw it as well. If concurrent cancellation with the cause was successful, IOException should have its cause as suppressed exception
2) When it's known that IOException wasn't thrown (cancellation was "first"), await should throw TestException
3) JobCancellationException should never be thrown if the cause is present on all code paths and the job is in its final state
4) No intermediate JobCancellationException should be present in the final state of Job

We should design, rework and document exception handling mechanism.
We can give up some performance on an exceptional path (especially when multiple exceptions are thrown), but regular code path should stay the same

[qwwdfsad]

As an additional improvement, we can do best-effort stacktrace augmentation via reflection and basic cycle detection (see #305) to provide even better debuggability

[qwwdfsad]

Note that #354 and #356 are very similar.
In both cases, thrown exception is lost and in #354 JCE is not passed to ExceptionHandler, so thrown exception can't be logged even if it was added to suppressed exceptions of JCE.

Current idea is to delay all exception processing until a transition to the final state of the Job, then construct final exception and handle it.

UPD:
https://stackoverflow.com/questions/50191606/why-coroutineexceptionhandler-didnt-catch-handle-my-exception

[ylemoigne]

I think that it was this behavior that I saw in my use case. Context : kotlin-js app with react wrapper.
An action launch a fetch. In case where the react component is dismounted while fetching, in the onComponentWillUnmount is set to cancel the job. After fetching but still in the coroutine, an action is dispatched (through redux) and the page location is change.

The simplified execution flow is like that :

action -> job=launch { fetchUrl.await() ; dispatch(Event) ; change(window.location) }
                                                |                      |
                                                |                      |
                                                |                      |
                                                |               Call Component (WillUnmount)
                                             Exception               job.cancel

Never saw the exception in console (I used a dispatcher with an handler which log exception in console). Very disturbing to understand what is happenning. But running in debug, I finally saw the switch around state of something, wrapped in a try { .... } catch. And in the catch, there is a if, my exception goes in a branch which do nothing with it.

If you decide to don't change the design, please, at least, consider the possibility of adding something like SupressedExceptionHandler so that we have a way to see what is hapenning.

[vadymhimself]

When is this going to be fixed? Is there a workaround to actually force-throw an exception out of the coroutine even if it was already canceled?

[qwwdfsad]

@bolein Hopefully in the next release. Currently, we're trying to define its interoperability with cancelling state

[vadymhimself]

I was able to force-throw an exception out of the coroutine on android using the main thread Handler.

[abdurahmanadilovic]

@bolein can you share a snippet of that force throw?

[vadymhimself]

@abdurahmanadilovic

  launch(UI_CONTEXT) {
    try {
      // do stuff
    } catch (e: Throwable) {
      if (!isActive) {
        Handler(Looper.getMainLooper()).post({
          throw Exception("Exception thrown in a cancelled coroutine $coroutineContext", e)
        })
      }
      throw e
    }
  }

[abdurahmanadilovic]

@bolein
The problem with try and catch is that i need to show an error on the UI inside the catch block, and if an exception happens in the catch block I can't catch it, this is what I came up with

launch(UI + CoroutineExceptionHandler({ context, throwable ->
        throw throwable
    })){
          //body
        }

[vadymhimself]

@qwwdfsad please explain the changes made in 9d0471b and when is it going to go live.

[qwwdfsad]

@bolein I will update our guide before merging it.
The general idea is to make exception handling "synchronous" to make parallel decomposition transparent.

E.g. for code

launch(MyExceptionHandler) {
    launch(coroutineContext) {  throw ChildException() }
    throw ParentException()
}

MyExceptionHandler will receive one of the exceptions (depending on timing) and another one in suppressed exceptions.
For jobs hierarchy exception is reported only once by root job, having all intermediate exceptions as suppressed, exceptions are never swallowed.

Other changes:
1) Custom exception handler can't override parent cancellation behavior
2) JobCancellationException are always ignored and used for internal machinery

Note that it's work in progress and anything can change.

It's going into release as soon as it's ready. It has a lot of corner cases and is breaking change, so we should internally evaluate it first.

[qwwdfsad]

Guide new section (still not merged, but it's more or less its final form)