https://kotlinlang.org logo
Title
s

Stefan Oltmann

02/10/2022, 1:36 PM
I'm a bit confused... since when does
cancel()
throw a
JobCancellationException
to the calling thread?
kotlinx.coroutines.JobCancellationException: StandaloneCoroutine was cancelled; job="coroutine#2444":StandaloneCoroutine{Cancelling}@6b6879d8
	at kotlinx.coroutines.JobSupport.cancel(JobSupport.kt:1605)
	at kotlinx.coroutines.Job$DefaultImpls.cancel$default(Job.kt:189)
1
j

Joffrey

02/10/2022, 1:37 PM
Are you cancelling that job from a coroutine running in that job? This behaviour would not surprise me in that situation
s

Stefan Oltmann

02/10/2022, 1:39 PM
This code used to not throw back an exception before Kotlin 1.6.0 ...
fun sync(photoSourceId: PhotoSourceId): Flow<PhotoAction> = channelFlow {
    
    val existingJob = jobMap[photoSourceId]
    
    if (existingJob != null && existingJob.isActive)
        return@channelFlow

    val job = backgroundCoroutineScope.launch {

        // the work
    }

    jobMap[photoSourceId] = job

    job.join()
}
fun cancelSync(photoSourceId: PhotoSourceId) {


    val job = jobMap[photoSourceId]

    job?.let {

        it.cancel()

        jobMap.remove(photoSourceId)
    }
}
Both methods are called from my main thread
Maybe I got something wrong elsewhere, not sure
j

Joffrey

02/10/2022, 1:44 PM
Are you calling
cancelSync
from inside the coroutine launched in
sync
?
s

Stefan Oltmann

02/10/2022, 1:46 PM
No, I call both from the outside - separate
launch
on the main dispatcher
🤔 1
j

Joffrey

02/10/2022, 1:55 PM
How did you determine that this exception is launched on the thread that calls cancel? Do you have a reproducible example?
s

Stefan Oltmann

02/10/2022, 1:56 PM
Because the stacktrace above starts in my cancelSync() (I snipped that)
j

Joffrey

02/10/2022, 1:58 PM
This might only show where the exception is built, but not which thread crashes as a result
Here: https://pl.kotl.in/d7FDktnkr You can see that the stacktrace shows
cancel()
as the first line, and yet if you remove this catch, the main thread doesn't throw. I think you might have a try-catch-all somewhere, misleading you into thinking your main thread crashes
s

Stefan Oltmann

02/10/2022, 2:09 PM
Oh, you are right. Thanks for that hint. I added a try catch block around that and indeed it does not come from
cancel()
... that's confusing. My internal sync() logic is wrapped with a try-catch like this:
} catch (ignore: CancellationException) {


    send(PhotoAction.SyncCanceled(photoSourceId))

}
I still land in there, but the exception is not ignored but now printed to my command line and even recognized by Sentry as uncaught. That's a new behaviour.
Maybe that's something Sentry changed. Don't know. I updated a lot of dependencies.
Thanks for your help. 🙏 I will find a way around this.
j

Joffrey

02/10/2022, 2:13 PM
It is usually a bad idea to try and catch
CancellationException
. It's meant for the coroutines framework. If you catch it, you should at least rethrow it so the machinery can work properly. Also, if the coroutine is cancelled, you likely won't be able so send anything on the channel because
send
is a suspend function and respects cancellation. You might work around this by using a
finally
block and
withContext(NonCancellable)
, but I don't know your use case enough to know whether that would be a good idea
:thank-you: 1
s

Stefan Oltmann

02/10/2022, 2:14 PM
Yeah, I was thinking it would be good to have that mechanic, call yield() sometimes and ignore the cancellelationException
But maybe I should just use an atomatic boolean as a latch to signal the user wants to cancel the sync
j

Joffrey

02/10/2022, 2:15 PM
You don't need
yield
if you already call some suspend functions (which you likely do in order to send items through the
channelFlow
s

Stefan Oltmann

02/10/2022, 2:16 PM
I noticed that without calling yield() at certain points I can stop the sync never cancels
because it's cooperative cancellation
j

Joffrey

02/10/2022, 2:17 PM
Yes, what I mean is that calling suspend functions has the same effect as yield (built-in cooperation on cancellation, and opportunity to dispatch other coroutines). If you have loops without any suspend function calls, indeed it's better to add
yield()
s

Stefan Oltmann

02/10/2022, 2:18 PM
Yes, I use it in a for loop
But now as I can't ignore the exception because some mechanic still picks it up (I suspect the Sentry update) I need to change it anyway to prevent my logs from being spammed
j

Joffrey

02/10/2022, 2:19 PM
It would be interesting to see the body of the
// work
here
s

Stefan Oltmann

02/10/2022, 2:21 PM
private suspend fun downloadMissingThumbnailsForCloudService(
    photoRepository: PhotoRepository,
    cloudClient: CloudClient,
    photoSourceId: PhotoSourceId,
) {

    val photoIdsWithoutThumbnail =
        photoRepository.findAllPhotoIdsWithoutThumbnail(photoSourceId)

    for (photoId in photoIdsWithoutThumbnail) {

        /*
         * Give a chance to cancel.
         */
        yield()

        try {
            
            val thumbnailBytes = cloudClient.getThumbnailImageItemBytes(photoId)

            photoRepository.saveThumbnailBytes(
                photoId = photoId,
                bytes = thumbnailBytes
            )
            
        } catch (ex: TimeoutCancellationException) {
            Log.error("Receiving thumbnail for $photoId timed out.", ex)
        }
    }
}
Downloading 1000+ thumbnails from a cloud takes a while 😉
I could surely remove the
yield()
by making the called methods
suspend
if I understood you correctly 🤔
j

Joffrey

02/10/2022, 2:24 PM
Yes that would be an option, but if the methods are not suspending then yes you need yield. But wait where do you emit things to the channelFlow? I don't see anything about it
s

Stefan Oltmann

02/10/2022, 2:26 PM
Yeah, I removed that here for better visibility. I have lots of those all over the place.
send(
    PhotoAction.ThumbnailStatusUpdate(
        photoIds = setOf(photoId),
        thumbnailStatus = ThumbnailStatus.LOADED
    )
)
I use a Redux pattern like the official kmm-production-sample does and fire back status updates for the UI to update trough this channel
j

Joffrey

02/10/2022, 2:31 PM
The whole point I was making was that those
send
calls were suspending. If they are present in your loop, you don't need
yield
. But I don't see how they could be present in
downloadMissingThumbnailsForCloudService
since you don't have the producer scope of the channel flow there. Sorry I guess the whole thing might be too entangled to share the idea without dumping the whole source
🙏 1
:thank-you: 1
s

Stefan Oltmann

02/10/2022, 2:32 PM
Oh wow, thank you a lot. Absolutely. I might no longer need the yield()
I added it as there were no send() calls in place and never gave that a second thought 🙈
l

louiscad

02/10/2022, 2:36 PM
BTW, in place of
yield()
, you can also call
currentCoroutineContext().ensureActive()
if starving the dispatcher while no cancellation happened is not a concern (often, in infinite loops, it is though).
👍 1
s

Stefan Oltmann

02/10/2022, 2:57 PM
It's a bit annoying to replace that mechanic, because together with Kotlin Native I don't know how to do it... how to have a thing like Javas AtomicBoolean and communicate cancellation between different threads.