Make request() cancellable - #51
Conversation
request() used suspendCoroutine, which does not support cancellation. A caller that timed out stayed suspended until the answer arrived, or forever when the target app died after it received the request. request() now uses suspendCancellableCoroutine. The caller can cancel the wait, and a late answer is ignored.
|
I'm not sure this is a good idea as the underlying request will not get cancelled (there is no mechanism to do so) and will get left dangling. Have you actually experienced infinite suspend here? |
request() now links to the death of the target binder. When the target app dies after it received the request, the caller resumes with null instead of a permanent suspend. The death link is removed when the call completes or the caller cancels.
|
no, I did not see it in production but the case is real. code catches DeadObjectException only around the synchronous call. sp when the target dies after it receiving request, the caller stays suspended forever. sjp4 flagged this in the coredevices/mobileapp#378 review now PR is extended with linkToDeath way, could this work maybe? |
| try { | ||
| return result.await() | ||
| } finally { | ||
| runCatching { binder.unlinkToDeath(deathRecipient, 0) } |
There was a problem hiding this comment.
Please use try catch instead, runCatching without passing the Result is just a try..catch with extra object allocations.
|
This looks fine to me, other than the minor runCatching comment. It also handles the cancellation (which I changed my mind now, is probably a good idea that we allow, even if we cannot propagate it). |
runCatching allocates a Result object for no gain here.
|
thanks! |
2 participants
UniversalRequestResponse.request() used suspendCoroutine, which does not support cancellation. A caller that cancels the wait, for example with a timeout, stays suspended until the answer arrives or forever, when the target app dies after it received the request.
request() now uses suspendCancellableCoroutine.
Cancellation resumes the caller, and a late answer is ignored.
Motivated by review feedback on coredevices/mobileapp#378, which wraps its data logging calls in a timeout that could not fire.