Replace TimeoutCancellationException with TimeoutException

[qwwdfsad]

In the current version of the library, withTimeout throws TimeoutCancellationException when a timeout was exceeded.

It can lead to very subtle errors, for example:

launch {
    val result = withTimeout(...) {
        // ... some computation ...
    }
    // process result
}

TimeoutCancellationException is CancellationException, thus is never reported.
But in the snippet above, it's likely to be a programmatic error. If it is expected to miss the deadline, then withTimeoutOrNull should be used explicitly.

My proposal is deprecation of TimeoutCancellationException and replacement with TimeoutException that is not CancellationException

[artbez]

Hi, @qwwdfsad !
My team and I also faced this problem. Do you have any expectations about the time this task will be solved?

[qwwdfsad]

Could you please elaborate on the problem you've faced?

[artbez]

Sure, we use withTimeout for restricting our inner functions with deadlines. So, any exceptions except CancellationException will be reported from our business logic code, but TimeoutCancellationException won’t. We want to parse such exceptions as others. And, as you said before, it would be possible if withTimeout threw TimeoutException instead of TimeoutCancellationException. Another case is when we use launch within coroutineScope. Exceded deadline in one job doesn't cancel other jobs. Of course, we can use our own custom launch which catches TimeoutCancellationException inside and throws TimeoutException or we can use withTimeoutOrNull. But, we'd like to know if you plan to change the behavior of withTimeout function.

Thank you!

[qwwdfsad]

The preliminary design decision is to deprecate withTimeout function with @LowPriorityInOverloadResolution and re-introduce it in the brand new package kotlinx.coroutines.time with the same name.

The change is now blocked by the IDE bug that doesn't replace a signature to the same name in a different package

[dkhalanskyjb]

This could be a good time to also avoid this behavior #1914 by checking at the end of the block if the coroutine was canceled and throwing in that case.

[qwwdfsad]

The same applies to the cancellation before withTimeout even start (that is also observed in #1914).

Historically, it was on par with withContext behaviour. Though the latter actively evolved (#962, #785, then #791), while withTimeout has been left as is. Its behaviour is clearly lagging behind what is expected from it by default and is worth changing.

It seems like we already have three potential changes in the function semantics:

• Exception type (more important, it's CE -> non-CE)
• Non-atomic cancellation (it also will help to get rid of all the hacks in our undispatchedResult and startUndispatchedOrReturnIgnoreTimeout) on completion
• Avoid entrance into the body of withTimeout if timeout already kicked in before the coroutine starts

[qwwdfsad]

It can be the case that the behaviour is too breaking and thus we should come up with a better name instead of silently shadowing the original signature.
What concerns me is the fact that timeouts are non-deterministic and are likely to happen under the load or if things go south, meaning that users are likely to learn about this change from production crashes rather than regular testing

[jQrgen]

Relevant: https://betterprogramming.pub/the-silent-killer-thats-crashing-your-coroutines-9171d1e8f79b

[wkornewald]

Relevant: https://betterprogramming.pub/the-silent-killer-thats-crashing-your-coroutines-9171d1e8f79b

The article correctly describes some root causes of the problem, i.e. CancellationException being used in places where an error-indicating exception would be needed instead.

However the article's proposed solution is to treat all abortable cancellations as errors (i.e. where the job wasn't canceled). This can't be correct. Just because >90% of CancellationExceptions use Job cancellation doesn't mean you can turn all other cancellations into errors. Ignoring those abortable cases with "but you shouldn't be catching errors there anyway" or "So far, I haven’t found any major issues" is a shaky argument (and in our codebase it would quickly hit issues). The article's solution is a workaround that breaks fundamental "safe bubbling" behavior of cancellations where you don't want to trigger error handling (like AbortFlowException).

The real solution is to fix the incorrect usages of CancellationException or at least provide alternative APIs where breaking changes wouldn't be acceptable. The bubbling behavior should not be broken.

[jQrgen]

@wkornewald, thanks for the clarification.

[joffrey-bion]

Is there a similar plan to "fix" Deferred.await() / SendChannel.send() / ReceiveChannel.receive() in the same way? i.e. only throw CancellationException when the calling coroutine is cancelled, but NOT in situations where the producer/consumer on the other end is cancelled (in which case some other exception that's not a subtype of CancellationException would be more appropriate, in order to avoid cancelling the calling coroutine silently)

[dkhalanskyjb]

We are yet to decide what to do about this. See #3658

[Legion2]

Also facing this inconsistency, as a workaround I now use withTimeoutOrNull and throw an exception on null value. Took my a long time to figure out why my code got silently canceled instead of getting a real exception which explains the timeout.

[masc3d]

this will also happen when combining flow operators timeout and merge. very confusing and difficult to track down in complex flows.

[scherrsasrf]

Just wasted 4 hours tracking down jobs that were cancelled on timeout but didn't throw an exception that could be logged. We are now using something like this:

class TimeoutException(cause: Exception) : Exception(cause)

suspend fun <T> withDeadline(timeout: Duration, block: suspend () -> T): T = try {
    withTimeout(timeout) { block() }
} catch (e: TimeoutCancellationException) {
    throw TimeoutException(e)
}

[kevincianfarini]

I understand that the maintainers are working on how to properly fix this design flaw, but in the meantime, can we mark the regular withTimeout function as requiring opt in? Acknowledging the design flaw in a way that's highly visible will help prevent future uses of withTimeout when the recommended solution for now is to use withTimeoutOrNull and throw a custom exception.

[eygraber]

I knew about this issue and still got bit by it.

[qwwdfsad]

Here goes the discussion for the potential solution: #4356