Since Arrow 1.1.3 we are getting `ShiftCancellatio...

# arrow

[nacyolsa]

Since Arrow 1.1.3 we are getting ShiftCancellationException(Shifted Continuation) is it some known issue?

[simon.vergauwen]

Hey @nacyolsa,

That is not a known issue but I think it indicates incorrect usage. I've been thinking about some ways to prevent this, and how to make this more easily debuggable.

Can you share any related code?

[simon.vergauwen]

You're either leaking bind or shift somewhere into an outer scope, or you're capturing and logging CancellationException which is not allowed in Kotlin.

CancellationException is a special case exception in Kotlin Coroutines.

[nacyolsa]

Can you share any related code?

At this moment I can't. I will try to prepare steps to reproduce later.

shift I use only in a few places. I checked if any bind is missing by using a new plugin and looks like everything is fine.

[simon.vergauwen]

The problem doesn't come from missing bind, it comes from using it in places where you leak it outside of it's scope. This is a known issue with DSLs like the once we have in Kotlin, where you leak functionality outside of their DSL scopes.

For example:

val res = either<String, () -> Int> {
  { "fail".left().bind() }
} // Either.Right(Function1)

res.orNull()?.invoke() // throws ShiftCancellationException

This happens because bind is leaked outside of the either { } scope. I see it's this part is cut-off by Dokka on the website 😞 Here is a link to the docs in KDoc with some more details, https://github.com/arrow-kt/arrow/blob/99e50bd2b5dfcc452d401fc8acfdffd7172d685a/ar[…]w-core/src/commonMain/kotlin/arrow/core/continuations/Effect.kt

Another problematic usage, but this is also not specific to Arrow but to the Kotlin Coroutine system is that you cannot capture CancellationException. I.e.

either<String, Unit> {
  try {
    shift("fail")
  } catch(e: Throwable) {
     println(e)
  }
}

Here CancellationException is swallowed which violates Kotlin Coroutines. This would print ShiftCancellationException, and would result in Either.Right(Unit).

[simon.vergauwen]

The first case could be improved by throwing RuntimeException instead of CancellationException with a better descriptive message. Which is what I am investigating now.

The second case would be considered a bug in Kotlin, and there is not much we can do about it. Perhaps improve the message of the CancellationException? 🤔

[Stylianos Gakis]

Catching a Throwable is such a classic coroutines mistake, also facilitated by the fact that runCatching also catches Throwable and therefore CancellationException and people assume that this is okay to do.
It’s really nice that Either.catch doesn’t do that and properly handles this case, but obviously most people aren’t gonna use that by default.

[simon.vergauwen]

The reason behind runCatching makes sense... Result & co was made for the internal machinery of Continuation and was never meant for public use.

Since it's now in everyones binary, they cannot simply change its behavior without breaking code. Changing the usages might break the binaries in some cases.

It should've never become public. (Change my mind) 😄

[simon.vergauwen]

They never wanted it to be public, Roman argued against it many times.

[Stylianos Gakis]

I really wonder if Kotlin can do something on the language level to help people avoid this problem. Unless you’ve delved into this problem before, there’s no way you know that catching this exception is a mistake. Like absolutely no way to know that beforehands, it’s not obvious.
And yes, I’ve heard of this story before, super unfortunate that it has been that way. But we need to play with the cards we’re dealt now unfortunately 😅

[simon.vergauwen]

Right, I think with compiler plugins we can inspect this and provide IDEA warnings and auto-refactor fixes. It's something that I am planning to investigate.

Since there is now an Arrow Detekt ruleset somewhere, I hope to contribute this as well there. Since compiler plugins / K2 is still quite a long time. Maybe 1-1,5 years away. IIRC planned for 1.10

[simon.vergauwen]

It's actually worse than just such a classic coroutines mistake. It's not just when inside of suspend, but it also applies to all inline fun because suspend can pass through inline fun.

[Stylianos Gakis]

Yeah damn, it applies to more places that one would first think 😵‍💫

[simon.vergauwen]

Still, it should be possible to build a compile-time inspection for this using compiler plugins. Which I hope to do asap. Perhaps it can even be done before 1.10 lands.

[simon.vergauwen]

@nacyolsa, has this helped you locate the issue? Feel free to ask any additional questions. If you're able to share the problematic code, I'd be happy to try to locate the issue.

I'm working on something to improve debug-ability of this issue, and I'm going to include a larger section in the next iteration of the documentation to explain what we discussed in this thread more clearly.

[CLOVIS]

I created KT-55480 to document that runCatching is incompatible with structured concurrency.

[nacyolsa]

I don't have extracted simple case to show you yet.
I didn't find root cause but it's occurs in this function

internal suspend fun <RESPONSE : Any> invoke(request: DaprRequest, type: KClass<RESPONSE>): Either<MessagingError, RESPONSE?> =
    withContext(<http://Dispatchers.IO|Dispatchers.IO>) {
        Either.catch {
            daprClient.invokeMethod(request.toInvokeMethodRequest(appId), TypeRef.get(type.javaObjectType))
                .awaitSingleOrNull()
        }.mapLeft {
            logger.error(it) { it.message }
            MessagingError(message = "Failed to call Dapr $appId request: $request", cause = it, isRecoverable = false)
        }
    }

It helped me to localize that withContext(<http://Dispatchers.IO|Dispatchers.IO>) is not needed here at all. After removing withContext(<http://Dispatchers.IO|Dispatchers.IO>) { issue doesn't occur. It also doesn't occur when I change IO dispatcher to Dispatchers.Unconfined .

[simon.vergauwen]

There is nothing here in the code related to ShiftCancellationException. There is no shift, bind or either { } within this code 😕

[Wesley Hartford]

I get this ShiftCancellationException when building a Flow inside an either {} and one of the steps in my Flow calls shift or bind . I understand the problem and generally know how to fix it, but I still write it frequently.