Eagerly cancel async bindings

[Munzey]

After some of the discussion here: https://github.com/michaelbull/kotlin-result/pull/36#discussion_r514505228 I looked into verifying whether all binds will finish executing when ran asynchrounously if a failed binding is encountered and how we should verify the behaviour in our tests. I realised it could be useful to have for compute heavy binding functions ran asynchronously to be able to cancel them eagerly.

Just a suggestion, let me know what you think.

[michaelbull]

Just trying to wrap my head around this. So the existing behaviour has no gaurantee that if one fails the others will cancel? Or is my current understanding of the existing behaviour wrong?

[Munzey]

Just trying to wrap my head around this. So the existing behaviour has no gaurantee that if one fails the others will cancel? Or is my current understanding of the existing behaviour wrong?

I think the two new tests in AsyncSuspendableBindingTest.kt help clarify the most.

If your binding function contains

{
  async{functionA.bind()}
  async{functionB.bind()}
}

where functionB is some long running suspendable function and functionA fails to bind, then with:

• binding - functionB is guaranteed to continue to run till the async job completes, or its own .bind throws the bind cancellation exception (this is the existing logic).

•eagerlyCancelBinding - the deferred job that functionB is running in is cancelled and will stop executing at next suspend point.

I also added documentation to the function which should help clarify what exactly its doing.

[michaelbull]

In that example, if functionB continues to run but functionA already failed, surely the result will still be the Err of functionA whether we cancel it early or not, right? If so, then I think the correct behaviour should not be to offer both but rather to actually just change our existing implementation to always cancel eagerly.

This would follow the example docs and look at as such:

functionA(): Result<Int, Err> = ...
functionB(): Result<Int, Err> = ...

suspend fun example(): Result<Int, Err> = binding {
    val one = async { functionA().bind() }
    val two = async { functionB().bind() }
    one.await() + two.await()
}

I can't really imagine an example with the binding function call where you'd want functionB to keep running if functionA fails, because it's always going to make the example function return the same thing and just waste time.

[Munzey]

In that example, if functionB continues to run but functionA already failed, surely the result will still be the Err of functionA whether we cancel it early or not, right?

You are correct in your analysis!

I can't really imagine an example with the binding function call where you'd want functionB to keep running if functionA fails, because it's always going to make the example function return the same thing and just waste time.

I think this is a very good argument and why I wanted to emphasize the pr is a suggested change 😅

Now that you say it maybe that is the better course of action here - my thinking was that if someone did have some side effecting function, that at least this way we give them the option to let it attempt to complete. Perhaps that is simply a bad idea though and we should not encourage that haha

[michaelbull]

Am I right in thinking that the behaviour of eager cancelling can be achieved using this example and our current implementation?-

functionA(): Result<Int, Err> = ...
functionB(): Result<Int, Err> = ...

suspend fun example(): Result<Int, Err> = binding {
    return coroutineScope {
        val one = async { functionA().bind() }
        val two = async { functionB().bind() }
        one.await() + two.await()
    }
}

[Munzey]

just tested thereturnsStateChangedForOnlyTheFirstAsyncBindFailWhenEagerlyCancellingBindingand replaced it with the block above and no, that wouldnt fix the problem. This would never cancel that coroutineScope, in our current implementation we jsut throw a cancellation exception which cancels the specific child

[Munzey]

great article on coroutine cancellations: https://medium.com/androiddevelopers/cancellation-in-coroutines-aa6b90163629

[michaelbull]

Then I think it makes more sense to make our binding function explicitly set a coroutineScope, as you've done in the eager one, and not offer two options here.

[Munzey]

Then I think it makes more sense to make our binding function explicitly set a coroutineScope, as you've done in the eager one, and not offer two options here.

Agreed I think that's the best call here, will update the pr shortly. If someone does end up wishing for non eager cancellations, we can address that in a separate issue.