retryResult signature makes it easy to use wrong context

[vrendina]

It seems that issue #4 was closed, but I have also inadvertently run into this situation today with similar code:

    withTimeout(1000) {
        retryResult {
            // This crashes because coroutineContext is obtained from the withTimeout scope
//            println("My retryStatus is ${coroutineContext.retryStatus}")

            // This is okay (naming conflict with coroutineContext)
            println("My retryStatus is ${kotlin.coroutines.coroutineContext.retryStatus}")

            // This is also okay
            coroutineScope {
                println("My retryStatus is ${coroutineContext.retryStatus}")
            }

            Ok(Unit)
        }
    }

I was able to workaround this issue fairly easily, but it is easy to shoot yourself in the foot. I understand it would be a breaking api change to update the type signature of ResultProducer. Is there any other reason why you wouldn't want to provide the proper scope to the retryResult block?

[michaelbull]

I'm not against breaking the API if it avoids such a problem. Could you explain what change you'd make and what the calling code would end up looking like? I think there's a lot to be said for the terseness & simplicity of the current retry(policy) signature and it's not clear to me why the retry function needs a scope passed to it.

[vrendina]

I believe this would resolve the issue:

private typealias ResultProducer<V, E> = suspend CoroutineScope.() -> Result<V, E>
private typealias Producer<T> = suspend CoroutineScope.() -> T

But is probably not the correct solution because of the breaking changes.

[michaelbull]

I actually don't think that will be a breaking change, because if you look here the call to withContext has a second argument which is the block. According to the coroutines docs, the block of withContext actually has the following signature: block: suspend CoroutineScope.() -> T. So I think such a change would actually align us more closely with what the call to withContext expects.

If you'd like to submit a PR with this change I'll review and merge it, if not I'll make the change soon.

[vrendina]

I actually don't think that will be a breaking change

The current type signature for retryResult (without the typealias) is:

suspend fun <V, E> retryResult(
    policy: RetryPolicy<E> = constantDelay(50) + limitAttempts(5),
    block: suspend () -> Result<V, E>
): Result<V, E> 

Updating the signature to include the CoroutineScope as the lambda receiver would break any clients that have explicitly defined the block type. Including those clients which are using method references for example ::someFunctionThatReturnsResult.

suspend fun <V, E> retryResult(
    policy: RetryPolicy<E> = constantDelay(50) + limitAttempts(5),
    block: suspend CoroutineScope.() -> Result<V, E>
): Result<V, E> 

Need to think a little more about the most appropriate solution.

[michaelbull]

I've gone ahead and made a rather significant refactoring that would be part of a v2 in 2babe2a1f91308d642b66df4646aacb9eb0dcb78. The main benefit is that it removes the burden on the caller of interfacing with the coroutineContext. Instead, both the policies and the calling code are written in context of a RetryScope (analogous to a CoroutineScope or a SequenceScope from the stdlib), which gives readonly access to the attempts, previous delay, cumulative delay, etc.

This results in a lot less boilerplate in both the calling code and the policy implementations:

fun main() = runBlocking {
    retryPrint()
}

suspend fun retryPrint() = retry(constantDelay(10)) {
    if (attempt < 3) {
        throw Exception()
    } else {
        println("success after $attempt, took $cumulativeDelay milliseconds")
    }
}

fun limitAttempts(limit: Int): RetryPolicy<*> {
    require(limit > 0) { "limit must be positive: $limit" }

    return {
        if (attempt + 1 >= limit) {
            StopRetrying
        } else {
            ContinueRetrying
        }
    }
}

[sureshg]

@michaelbull any plans to release these changes (with RetryScope). It's there in main branch for a very long time :-)