CompletionHandlerException: Exception in resume onCancellation handler for `CancellableContinuation(DispatchedCont`…

[AliSoftware]

Sentry Url: https://sentry.io/share/issue/86e0889187bf49408f1f7460cdb241a8/
User Count: 281
Count: 617
First Release: 21.0-rc-1
First Seen: 2022-10-18T06:34:51.360000Z
Last Seen: 2022-11-15T08:51:07Z
24 Hours: 71
30 Days: 614

CompletionHandlerException: Exception in resume onCancellation handler for CancellableContinuation(DispatchedContinuation[Dispatchers.IO, Continuation at org.wordpress.android.ui.uploads.UploadStarter$upload$2.invokeSuspend(UploadStarter.kt:148)@cc5a5b9]){Completed}@ebc1dfe

...
(1 additional frame(s) were not displayed)

[ovitrif]

I couldn't reproduce this issue crash, what I've tried is:

1. Open App
2. Disable internet connectivity
3. Create and edit a couple of posts and pages, some to be published some saved as drafts
4. Either
   a. Put app in background
   b. Close app
5. Enable internet connectivity
6. Either
   a. Put app in foreground
   b. Open app

I've tried these steps about 5 times, both using emulator and physical (Samsung) device.

The failure occurs in UploadStarter.kt:148, and it's caused by IllegalStateException on mutex.unlock() based on the stack trace in Sentry.

According to the timing of events reported in Sentry, this happens shortly after the app is started.
Without being able to reproduce it, I can't be sure if this results in data loss (though it might have the potential to), or if the issue recovers automatically with additional attempts to upload their local data.

I traced back the changes to this commit: 43949f1.

@ParaskP7 since you may have more context on this I tagged you on this one 🙇 .

My understanding is that formerly there was an issue on Samsung device with their fork of Android 5.0, but now we don't support that Android version anymore. I've checked to see if this issue is somehow still linked to Samsung devices, while it occured in a few instances on those, there are other devices affected.

I've triaged this to HIGH priority with a bit of skepticism, since it might auto recover and not necessarily result in data loss (this is based on the fact that I didn't see it repeat for the same user in Sentry).

[ParaskP7]

👋 @ovitrif and thanks for this investigation! 🥇

I couldn't reproduce this issue crash

Yea, during testing, I couldn't make it crash as well, that is, when we replaced @Synchronized suspend with suspend mutex.

My understanding is that formerly there was an issue on Samsung device with their fork of Android 5.0, but now we don't support that Android version anymore. I've checked to see if this issue is somehow still linked to Samsung devices, while it occurred in a few instances on those, there are other devices affected.

Yea, but I think the previous issue was different to this one, see issue here and fix PR here. As such I believe this is a different issue. Cc @malinajirka wdyt? 🤔

According to the timing of events reported in Sentry, this happens shortly after the app is started.
Without being able to reproduce it, I can't be sure if this results in data loss (though it might have the potential to), or if the issue recovers automatically with additional attempts to upload their local dat

I've triaged this to HIGH priority with a bit of skepticism, since it might auto recover and not necessarily result in data loss.

Thanks for adding the HIGH priority, I think this is the correct priority as we should make sure that this doesn't results in data loss.

(this is based on the fact that I didn't see it repeat for the same user in Sentry)

I am actually seeing it repeat for the same user, see Discover view here.

I traced back the changes to this commit: 43949f1.

@ParaskP7 since you may have more context on this I tagged you on this one 🙇 .

I am not a Mutex expert, thus, I am not sure what is happening here, this needs a bit of a debugging and further investigation to get to the bottom of it, along with a way to replicate it first. I am seeing Pixel 6 crashes as well, which give me some more hope that we could replicate this somehow. 🤞

Now, I went and read the documentation on Mutex.lock() and I am still trying to get my head around it, for example, I still don't (perfectly) understand this below:

- This suspending function is cancellable.
- If the `Job` of the current coroutine is cancelled or completed while this function is suspended,
this function immediately resumes with `CancellationException`.
- There is a prompt cancellation guarantee.
- If the job was cancelled while this function was suspended, it will not resume successfully.
- This function releases the lock if it was already acquired by this function before the
`CancellationException` was thrown.

Parameters (owner)

Optional owner token for debugging. When owner is specified (non-null value) and
this mutex is already locked with the same token (same identity),
this function throws `IllegalStateException`.

I am a bit confused by all the above... @ovitrif and @malinajirka, what is your take on what documentation is saying? Any ideas on how to progress from here? 🤔

[malinajirka]

Looking at the crash, it mentions Mutex is not locked => I haven't done a proper research, but my personal guess is that the coroutine gets cancelled -> the system automatically unlocks the mutex as per If the Jobof the current coroutine is cancelled or completed while this function is suspended, this function immediately resumes withCancellationException. => we attempt to unlock it again in finally => 💥 . (even though my gut feeling tells me that this shouldn't happen as I'd expect the finally to be processed before the cancellation 🤷 )

I'd personally try to reproduce this by modifying the code - or even writing a dummy code with the same setup (Lock, cancel, unlock in finally) -> observe how it behaves or we could just try wrapping the unlock in if(mutex.isLocked) {...}. Wdyt?

[ParaskP7]

Looking at the crash, it mentions Mutex is not locked => I haven't done a proper research, but my personal guess is that the coroutine gets cancelled -> the system automatically unlocks the mutex as per If the Jobof the current coroutine is cancelled or completed while this function is suspended, this function immediately resumes withCancellationException. => we attempt to unlock it again in finally => 💥 . (even though my gut feeling tells me that this shouldn't happen as I'd expect the finally to be processed before the cancellation 🤷 )

Thank @malinajirka , this is exactly what I am thinking too... 💯

I'd personally try to reproduce this by modifying the code - or even writing a dummy code with the same setup (Lock, cancel, unlock in finally) -> observe how it behaves or we could just try wrapping the unlock in if(mutex.isLocked) {...}. Wdyt?

Yes, I like both suggestions, if it were me, I would first go with adding the if(mutex.isLocked) {...} to guard the mutex.unlock() and include it as quick as possible to the codebase. In the meanwhile, I would have tried to reproduce this somehow, with a dummy code or otherwise. Wdyt @ovitrif ? 🤔

[ovitrif]

Thank you @ParaskP7 & @malinajirka for your feedback and insights on this issue 🙇‍♂️

I also had on my mind that we could use if(mutex.isLocked) { mutex.unlock() } but wasn't sure how much of a proper fix this would be.

As I've understood from your feedback actually the if(mutex.isLocked) is a valid fix if this is indeed correct (bonus points if we can prove it):

the coroutine gets cancelled -> the system automatically unlocks the mutex as per If the Jobof the current coroutine is cancelled or completed while this function is suspended, this function immediately resumes withCancellationException. => we attempt to unlock it again in finally => 💥

---

I am actually seeing it repeat for the same user, see Discover view here.

Thank you for this info @ParaskP7, I could actually see it occur for the same user, and repeatedly while attempting to sync the same post to the server, which increases the likelihood of it causing data loss.

Yes, I like both suggestions, if it were me, I would first go with adding the if(mutex.isLocked) {...} to guard the mutex.unlock() and include it as quick as possible to the codebase. In the meanwhile, I would have tried to reproduce this somehow, with a dummy code or otherwise.

This makes perfect sense to me, thank you once again 🙇 , I'm not a mutex expert myself but I'll give this a try with dummy code and see if I can reproduce it; as for a start I'll already open a PR that guards the mutex.unlock() call.

[ParaskP7]

👋 @ovitrif !

As I've understood from your feedback actually the if(mutex.isLocked) is a valid fix if this is indeed correct (bonus points if we can prove it):

💯

This makes perfect sense to me, thank you once again 🙇 , I'm not a mutex expert myself but I'll give this a try with dummy code and see if I can reproduce it; as for a start I'll already open a PR that guards the mutex.unlock() call.

Awesome, feel free to add me as a reviewer to this PR that you've already opened, I can't seem to find it myself, I am probably lucking some GitHub search skills here... 😅

[ovitrif]

Awesome, feel free to add me as a reviewer to this PR that you've already opened, I can't seem to find it myself, I am probably lucking some GitHub search skills here... 😅

👋 @ParaskP7 I've just opened the PR now 🙇 :

• Guard mutex unlock in isLocked check to avoid errors during local data uploads #17472