Proper handling of closable resources after #896

[Tolriq]

This is a follow up of #896 that started on slack.

Let's take the following example.

    private suspend fun Call.await(): WorkerResult {
        return suspendCancellableCoroutine { continuation ->
            continuation.invokeOnCancellation {
                cancel()
            }
            enqueue(object : Callback {
                override fun onResponse(call: Call, response: Response) {
                    continuation.resume(WorkerResult.Success(response))
                }

                override fun onFailure(call: Call, e: IOException) {
                    if (call.isCanceled || !isActive) {
                        continuation.resume(WorkerResult.Error(CancellationException("Cancelled")))
                    } else {
                        continuation.resume(WorkerResult.Error(e))
                    }
                }
            })
        }
    }

How can we ensure that the resume from onResponse()is either successful or that we can close the response.
Adding isActive tests is race condition prone, and I can't think of a way to avoid a race condition leading to leaks of the response.

cc: @elizarov

[elizarov]

Interesting use-case. Good that we have it now. There is only non-public (internal) APIs for that purpose now and they are not quite convenient to use.

First internal API is tryResume/completeResume. However, even if you use it, there is no guarantee that you are have not lost your resource, since it resumes in cancellable mode. Here is what can happen:

1. You invoke resume successfully (continuation was not cancelled yet).
2. The continuation is scheduled for execution (for example, onto the main thread).
3. While continuation is scheduled, job gets cancelled.
4. Now it resumes with CancellationException, instead of resuming with your result, so if it needed to be closed bad luck - you've lost it.

So, there's another internal API that solves that problem. Called suspendAtomicCancellableCoroutine. So, to "solve" your problem you'll need to use all three internal functions. Too error prone, obscure. That is why they are all internal.

We definitely need some public API. But what kind of API -- that is the question.

For example, we can add a separate resume function. Something like CancellableContinuation.resumeAtomic(value): Boolean which returns Boolean (success/fail) and if it returned true then it guarantees that the result is going to be delivered despite cancellation.

Would it help?

(The name is not helpful, though. I wonder how to name it so that it is clear what it does).

[elizarov]

Now, to continue discussion. The problem with resumeAtomic is that it places burden on the users of your API. Suppose we have resumeAtomic and Call.await() is implemented via resumeAtomic. Now somewhere in a code of some Android app:

activityScope.launch { 
    updateUIBefore() // (1)
    val result = someCall().await() // resumes atomically
    updateUIAfter(result) // (2)
    result.close() // close resource
}

Since resume is atomic, then updateUIAfter at (2) can happen when the activity is already destroyed (job cancelled). This is extremely non-obvious and error-prone. So we get one convenience (close resources), loose another convenience. Now, we have to always remember that we need to check if the job is still active after await() if we want to touch UI:

activityScope.launch { 
    updateUIBefore() // (1)
    val result = someCall().await() // resumes atomically
    if (isActive) { // must not forget it!!! Very error-prone
        updateUIAfter(result) 
    }
    result.close() // close resource
}

So alternative design would be a two argument version of resume that takes value and a lambda to close this value in case of cancellation:

continuation.resumeCloseable(resource) { 
    resource.close()
}

How to name this resumeCloseable one? Also a tricky question.

[Tolriq]

I was answering previous post and you confirmed that the atomic or a forced delivery was not good as it's easy to forget the cancellation check on caller side.

Love the last proposal, maybe continuation.cancelableResume ? As it's actually that, a resume that can be cancelled and call something in that case.

[elizarov]

It sends a wrong message that other, plainresume is non cancellable, which is not the case. One solution would be just to overload CancellableContinuation.resume:

fun resume(value: T, onCancellation: (cause: Throwable) -> Unit)

[Tolriq]

Yep simple and clear,

On what context / scope would the callback be called? The resume side, the await side or something else?

[elizarov]

Here's the catch. Just like the family of invokeOnXxx { ... } functions this would be a low-level and fully synchronous mechanism that gives no guarantees on execution context and could be only used for thread-safe and non-blocking resource-closing methods (like closing a file or closing a socket).

[Tolriq]

Fine for me as my usage is mainly for database cursors and okHttp response.

Just need the doc to clearly state that, and I suppose one can dispatch the closing to Dispatchers.IO if necessary for a blocking close.

[elizarov]

If you'll need to something long blocking op, you can always do GlobalScope.launch(Dispatcher.IO) { ... } from inside of there. This advice definitely need to go to the docs, too.

[JakeWharton]

Wouldn't using use

someCall().await().use { result ->
  updateUIAfter(result)
}

at the consumption site ensure close() is always called? Special casing Closeable in the API doesn't feel right to me.

[Tolriq]

The resources does not reach the await if there's cancellation during resume.
That's the whole issue here.

Last proposal is not closeable specific, it just allows to ensure that either the await is successfully receiving the result and can actually do call use or that a callback is called to do whatever may be needed to do if the await receive a cancelled. (And in this case call the close but could also be anything else like notifying something waiting elsewhere or whatever).

[JakeWharton]

I'm addressing #1044 (comment). I'm aware of the problem as you know.

[Tolriq]

Ok you missed the last proposal: #1044 (comment)

[JakeWharton]

I didn't. That addresses the problem at the publish site only, not the consumption site where there's a section suspension and cancelation.

[Tolriq]

Then I do not understand your

Special casing Closeable in the API doesn't feel right to me.

There's nothing related to that now, and the cancellation after await was always possible even before #896, Once the await success it have always been the consumer job to properly check for cancellation and do needed stuff there's was no magic ignoring/changing messages unless you call another suspending mechanism that automatically check for cancellation.

So really don't understand your comments.

[JakeWharton]

There were comments suggesting special casing closeable to which I expressed my distaste for. There were comments about the consumption site needing to be careful about suspension between receiving the resource and closing it to which I expressed that use solves it.

[Tolriq]

Ok, but I thought those proposal where out of choice after the previous exchanges, that's why I did not see any correlation with the last proposal.

The consumption side issue was not about the closing of the ressources when cancellation occurs but the fact that await could return a value that you may want to use when the coroutine is cancelled and so the context could be invalid like destroyed activity / fragment.
Usage of use would not make the activity present and so the isActive call would have still be necessary, hence the last proposal that keep current situation when await succeed the context is still valid (assuming you await on the mainThread of course) so resolve nearly all the issues.

[Tolriq]

@elizarov with 1.2 in the work and this not tagged for 1.3 can you expand a little on your first explanation of the 3 internals functions so that I can use that in the meantime?

I'm not really sure how I should merge the 3 in the example from first post.