https://kotlinlang.org logo
#coroutines
Title
# coroutines
m

Mark

02/26/2024, 5:33 AM
Recently I’ve had a couple of ANR reports:
Copy code
jdk.internal.misc.Unsafe.getReferenceVolatile
when calling
trySend
on a
BufferedChannel
(actually I’m using SingleShotEventBus). Is this something that can be mitigated?
Copy code
main (runnable):tid=1 systid=22340 
#00 pc 0x4bf790 libart.so (art::DumpNativeStack + 108) (BuildId: 02bec5940be704b863f6514fc7d81c41)
#01 pc 0x4b2940 libart.so (art::Thread::DumpStack const + 388) (BuildId: 02bec5940be704b863f6514fc7d81c41)
#02 pc 0x4b2280 libart.so (art::DumpCheckpoint::Run + 164) (BuildId: 02bec5940be704b863f6514fc7d81c41)
#03 pc 0x3c8c9c libart.so (art::Thread::RunCheckpointFunction + 148) (BuildId: 02bec5940be704b863f6514fc7d81c41)
#04 pc 0x5afae0 libart.so (artTestSuspendFromCode + 232) (BuildId: 02bec5940be704b863f6514fc7d81c41)
#05 pc 0x35501c libart.so (art_quick_test_suspend + 156) (BuildId: 02bec5940be704b863f6514fc7d81c41)
       at jdk.internal.misc.Unsafe.getReferenceVolatile(Native method)
       at java.util.concurrent.atomic.AtomicReferenceFieldUpdater$AtomicReferenceFieldUpdaterImpl.get(AtomicReferenceFieldUpdater.java:480)
       at kotlinx.coroutines.JobSupport.getState$kotlinx_coroutines_core(JobSupport.kt:164)
       at kotlinx.coroutines.JobSupport.isActive(JobSupport.kt)
       at kotlinx.coroutines.AbstractCoroutine.isActive(AbstractCoroutine.kt)
       at kotlinx.coroutines.channels.ProducerCoroutine.isActive(Produce.kt)
       at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:96)
       at kotlinx.coroutines.EventLoop.processUnconfinedEvent(EventLoop.java:65)
       at kotlinx.coroutines.DispatchedTaskKt.resumeUnconfined(DispatchedTask.kt:241)
       at kotlinx.coroutines.DispatchedTaskKt.dispatch(DispatchedTask.kt:159)
       at kotlinx.coroutines.CancellableContinuationImpl.dispatchResume(CancellableContinuationImpl.kt:470)
       at kotlinx.coroutines.CancellableContinuationImpl.completeResume(CancellableContinuationImpl.kt:586)
       at kotlinx.coroutines.channels.BufferedChannelKt.tryResume0(BufferedChannel.kt:2904)
       at kotlinx.coroutines.channels.BufferedChannelKt.access$tryResume0(BufferedChannel.kt:1)
       at kotlinx.coroutines.channels.BufferedChannel$BufferedChannelIterator.tryResumeHasNext(BufferedChannel.kt:1694)
       at kotlinx.coroutines.channels.BufferedChannel.tryResumeReceiver(BufferedChannel.kt:642)
       at kotlinx.coroutines.channels.BufferedChannel.updateCellSend(BufferedChannel.kt:458)
       at kotlinx.coroutines.channels.BufferedChannel.access$updateCellSend(BufferedChannel.kt:36)
       at kotlinx.coroutines.channels.BufferedChannel.trySend-JP2dKIU(BufferedChannel.kt:3307)
l

louiscad

02/26/2024, 10:07 AM
Are you catching all exceptions somewhere? You might have an infinite loop if you catch
CancellationException
without rethrowing it.
Other than that, I don't see why you'd have an ANR.
m

Mark

02/26/2024, 10:09 AM
Ok thanks I’ll check
Does that mean every single place in suspending code where I have something like
catch(Exception)
I need to separate out the
CancelationException
and rethrow, or only under certain conditions?
l

louiscad

02/26/2024, 10:13 AM
Always rethrow
That's also documented on d.android.com/kotlin/coroutines
m

Mark

02/26/2024, 10:18 AM
In https://developer.android.com/kotlin/coroutines#handling-exceptions it’s not rethrowing the
CancelationException
. Am I missing something?
l

louiscad

02/26/2024, 10:18 AM
Don't catch it, or rethrow it.
And that example is bad IMO
Looks like they updated the doc and it's no longer showing the gotcha 😠
m

Mark

02/26/2024, 10:19 AM
Oh dear 😞
l

louiscad

02/26/2024, 10:20 AM
So, yeah, avoid preventing cancellation from happening
m

Mark

02/26/2024, 10:29 AM
I wish there was a way in AS to search for try catch blocks but only in suspending code
l

louiscad

02/26/2024, 10:31 AM
You might also want to check inline functions because they can run suspending blocks
👍 1
m

Mark

02/26/2024, 12:49 PM
How about
CancelationException
thrown from a
ContentProvider
implementation to its client? At the moment, I rethrow it as an
OperationCanceledException
which I guess could be a problem.
l

louiscad

02/26/2024, 12:51 PM
Seems like it's not the one from kotlinx.coroutines/java.util.concurrent, so it's up to you to figure out what's happening in those cases exactly, and how you should/want your code to react to that happening.
m

Mark

02/26/2024, 1:05 PM
Do you mean there are some legitimate cases where you would want to catch it without rethrowing it?
l

louiscad

02/26/2024, 1:20 PM
I mean I don't know what's expected for that specific non Kotlin ContentProvider API
m

Mark

02/26/2024, 1:27 PM
Regardless of what is expected of the ContentProvider, I wouldn’t want to leave a coroutine in some inconsistent state. I’ll have a play to see what happens if I just rethrow the CancellationException
Is there a reason why there is no lint check for this rethrowing? It seems like a very easy mistake to make.
l

louiscad

02/26/2024, 1:29 PM
The one from coroutines would propagate until the launch that started the coroutine eats the exception.
👍 1
I don't know, maybe @Vsevolod Tolstopyatov [JB] knows better why there's no warning, IDE intention or "lint" for this very (very very) common pitfall.
m

Mark

02/26/2024, 1:32 PM
It almost feels like it should be a sibling of
Exception
and
Error
l

louiscad

02/26/2024, 1:32 PM
Oh yeah, that's a very good idea!
Somehow never thought about that.
m

Mark

02/26/2024, 1:33 PM
Not gonna happen haha
l

louiscad

02/26/2024, 1:34 PM
I think it actually could happen
s

Sam

02/26/2024, 1:34 PM
l

louiscad

02/26/2024, 1:34 PM
Would break things, but could be mitigated to break mostly "bad" code
v

Vsevolod Tolstopyatov [JB]

02/26/2024, 1:37 PM
There is no such inspection mostly because we haven't seen a lot of user-facing issues caused by accidental catching of
CancellationException
(NB: it's not like "there is no issues" but rather "We haven't seen much internally and we don't have enough external reports"). If you could report us things like "hey, here is my code that was really hard to troubleshoot. It's because I've caught
CE
and the child component got stuck", I would really appreciate it 😊
l

louiscad

02/26/2024, 1:40 PM
I'm pretty sure you don't see it much internally because you're not doing much Android development where exceptions are commonplace and where you often need to do defensive programming to some extent, only to be shot back in a sneaky way that some people probably don't even ever understand the root causes of because they're not coroutines experts like I am (humbly)
v

Vsevolod Tolstopyatov [JB]

02/26/2024, 1:42 PM
Definitely, we are kind of biased in that matter (and many others as well!), that's why external feedback is always appreciated. So any prior experiences and problems caused are welcome
m

Mark

02/26/2024, 1:42 PM
I would never have guessed an ANR was caused by catching and not rethrowing a CE
v

Vsevolod Tolstopyatov [JB]

02/26/2024, 1:45 PM
This part is a bit unclear to me. The ANR stacktrace looks completely innocuous from the first glance. Potentially, there might be some loop in the form of
while (job.isActive) doWorkUnconditionally
, but other from that it's hard to pinpoint the potential problem
l

louiscad

02/26/2024, 1:48 PM
If I was asked this:
What do you believe is the worst pitfall of the ones remaining in Modern Android Development?
I would reply: "this issue with unintended breakage of cancellation and structured concurrency", because: • It's very sneaky • Hard to debug • Affects the whole app • You expect kotlinx.coroutines to be all about safety • It changes the way one needs to think about exceptions, without that being properly documented (and who reads doc? Well I do, but you know…)
thank you color 1
1
👌 1
I myself was bitten by this numerous times while writing greenfield "coroutines first". Last time it happened to me was probably in 2022, so 4-5 years after I considered myself very familiar with coroutines, and using them almost daily on my day job back then. Can't speak for others, that's just my experience.
I should probably come up with some snippets that show the problem one day, but I might never take the time to recreate those. What happened often is that some suspending code would be run inside a block that would catch any Exception (or worst, any Throwable), to avoid crashing everything when something called in the suspending code fails with an unpredictable exception. That code, including the part that would catch exceptions (or worst, throwables), would be located inside a loop (e.g. while(true)). On cancellation, you now have an infinite loop that will be extremely hard to debug, especially in release mode (and therefore, in production).
that loop might be there just to handle retries on error, or to restart whatever process once a previous iteration is done
I believe there are other use cases that can lead to something similar
v

Vsevolod Tolstopyatov [JB]

02/26/2024, 2:04 PM
Thanks! I'll see how we can incorporate this knowledge further
🙏🏼 1
l

louiscad

02/26/2024, 4:55 PM
There's this issue in Android Studio Jellyfish Canary 11 where it eats 300% CPU without a valid reason. I see
kotlinx.coroutines
there… Could this be related to that issue? blob thinking fast
Could be from a third party IDE plugin also of course, or could be another problem
k

kevin.cianfarini

02/27/2024, 11:25 PM
It's worth noting that the less experienced engineers in my team, particularly ones on the iOS side working on our KMP code, dont realize that catching
Exception
without rethrowing CE is dangerous. We've had many, many issues regarding this. It's really hard to enforce on a team that isn't solely composed of senior engineers
☝🏼 1
And another example recently, I got a notification from davx5 specifying that the job was canceled on my phone. This leads me to believe that this issue is prevalent in popular OSS repos too
🫠 1
l

louiscad

02/28/2024, 8:03 AM
Copy code
@Deprecated
expect class CancellationException : Exception

@Deprecated
actual typealias CancellationException

class CancellationSignal : Throwable()
Copy code
@Deprecated
fun withTimeout(…)

@Deprecated
class TimeoutCancellationException
🫡
k

kevin.cianfarini

02/28/2024, 8:32 PM
At the very least, I think that CancellationException shouldn’t inherit from IllegalStateException. I get why they did this for alias reasons with the JVM’s CancellationException, but now checks like this are error prone. It’s not just
catch (e: Exception)
.
Copy code
try {
  yield()
  checkNotNull(someField) { "It was null! Scary!" }
} catch (e: IllegalStateException) {
  logger.log(e)
  // BUG, we didn't rethrow CancellationException
}
I suspect we definitely wouldn’t be able to make any change like this until coroutines 2.0.0+
Perhaps a more graceful migration would be:
Copy code
// Coroutines < 2.0.0

@Deprecated("Use CancellationSignal") 
open class CancellationException : IllegalStateException

class CancellationSignal : CancellationException()

// Coroutines >= 2.0.0 

class CancellationSignal : Throwable()
👍 1
l

louiscad

02/28/2024, 8:44 PM
Your snippet with "BUG" misses a suspend call to make it plausible, maybe asd
delay(something)
or
awaitWhathever()
?
k

kevin.cianfarini

02/28/2024, 8:45 PM
yeah sorry assume suspend because we’re in the coroutines channel. Lemme update.
m

Mark

02/29/2024, 5:31 AM
Devs still need to be aware of this
CancellationException
curiosity because sometimes you want to catch everything in order to do some tidy-up (that is only required when something goes wrong) for example deleting a file that has been partially written to. So I’m not sure how much moving the Exception in the class hierarchy will help. I suppose if there was some kind of
finallyForThrowable
block (or if the
finally
block was passed a nullable
Throwable
), then that would work.
l

louiscad

02/29/2024, 10:34 AM
throwing null leads to a
NullPointerException
being thrown instead, which is ambiguous, so not great, best to just have a custom
Throwable
type IMO
m

Mark

02/29/2024, 1:35 PM
Although getting OT, I was meaning it would be useful (in this case and others) to have a kind of finally for when a throwable is thrown (that’s the
finallyForThrowable
bit). Alternatively, the
finally
block could receive a
Throwable?
arg to indicate what was thrown (in the case of non-null), or null for when the
try
finished normally. Like with
finally
normally, you wouldn’t be throwing anything from there. It’s messy though because is the throwable the one thrown from the
try
or one potentially thrown from a
catch
?
k

kevin.cianfarini

02/29/2024, 7:08 PM
catch everything in order to do some tidy-up (that is only required when something goes wrong) for example deleting a file that has been partially written to.
In a scenario like this you should probably be rethrowing the exception, regardless of its type.
Copy code
try {
  aThing()
} catch (e: Throwable) {
  doPartialTidyUp()
  throw e
}
m

Mark

03/01/2024, 2:42 AM
Sometimes, but you may also be returning null, for example
l

louiscad

03/01/2024, 10:25 AM
You might, but should you? Personally, `try`/`finally` and
use { }
extensions do all I need. Then I need to catch `Exception`s general or specific. I can't think of any case where I would want to catch all
Error
subclasses or a random
Throwable
subclass that is not an
Exception
.
catch (e: Throwable)
is not something I'd want to see in my codebases, because it's almost certain it's not handling errors properly.
☝️ 1
m

Mark

03/02/2024, 12:16 PM
How would you deal with a partially written file that needs to be deleted on any throwable?
k

kevin.cianfarini

03/02/2024, 1:32 PM
With the code that I shared two messages ago
m

Mark

03/02/2024, 2:40 PM
But catching
Throwable
is exactly what Louis said he doesn’t want.
l

louiscad

03/02/2024, 4:07 PM
Unless you rethrow I meant
m

Mark

03/03/2024, 3:32 AM
This is the point of the alternative
finally
I was suggesting. You don’t need to think about rethrowing. It’s done for you.
k

kevin.cianfarini

03/03/2024, 7:21 PM
Such a niche use case doesn't warrant a language feature IMO when it can be built with existing language features as a function like
use
.
m

Mark

03/04/2024, 2:45 AM
How would you use
use
in this case? You still need to either use a try/catch and rethrow or use a messy boolean to indicate whether the block finished normally or not. I think the cleanest solution would be for the
finally
block to be passed a boolean to indicate whether the
try
completed normally or not.
l

louiscad

03/04/2024, 9:42 AM
It'd just save one line of code (
throw t
). If you find a proper name for a function taking 2 lambdas, running the second one on Throwable before rethrowing, or a name for a
Result<T>
extension that calls a block with the Throwable then rethrows (to be used after
runCatching
, then go for it!
m

Mark

03/04/2024, 9:53 AM
It’s really not about saving one line of code, it’s more about enforcement. We could make a similar argument on why
finally
is not necessary. It can easily be achieved by calling a local tidyup function and rethrowing as necessary.
l

louiscad

03/04/2024, 9:55 AM
I think you'd want something like that (not possible with current Kotlin though):
Copy code
try {
    doPossiblyThrowingStuff()
} intercept (t: Throwable) {
    cleanup() // Won't suffice in case of process death!
}
This can work:
Copy code
runCatching {
    doPossiblyThrowingStuff()
}.intercept<Throwable> {
    cleanup() // Won't suffice in case of process death!
}
You'd need to document that
intercept
rethrows after running its block
And you can make it return
T
(from
Result<T>
)
m

Mark

03/04/2024, 9:59 AM
I don’t even think it needs the Throwable generic because it’s very much about if anything goes wrong which is why I was thinking of a boolean.
l

louiscad

03/04/2024, 9:59 AM
I kept it because without it comes the question: "What is it intercepting exactly?"
m

Mark

03/04/2024, 10:09 AM
Like this?
Copy code
fun <T> Result<T>.interceptFailure(failureBlock: () -> Unit): T {
    if (this.isFailure) {
        failureBlock()
        throw exceptionOrNull()!!
    }
    return getOrThrow()
}
l

louiscad

03/04/2024, 10:21 AM
Yes, and you can (should) make it inline
👍 1
k

kevin.cianfarini

03/19/2024, 1:12 PM
Commenting here again — just reviewed a PR from one of my senior coworkers that catches
IllegalStateException
but did not rethrow CancellationException. The existence of utilities like
checkNotNull
in Kotlin popularized the use of general exceptions like ISE, and that directly conflicts with how cancellation should propagate through a system. Perhaps we should raise this as an issue on the repo?
3 Views