Concurrent failing binds causes uncaught BindException

[Munzey]

I was adding tests to check which error is returned when multiple binds fail for suspendable binding. I suspected that for async calls that finish closely there could be a race condition and you end up with the error from say, the second failed call instead of the first.

Turns out there is a much bigger problem with implementation for async suspendable functions:

runBlocking {
            val result = binding<Int, BindingError> {
                val x = async { success().bind() }
                val y = async { fails().bind() } // weird stuff happens here and an uncaught BindException occurs
                val z = async { fails().bind() } 
                x.await() + y.await() + z.await()
            }
}

Basically its not behaving with the different coroutines running concurrently. If you step through the test I've added returnsFirstErrIfBindingFailed() you'll see what I mean. Struggled to google a solution to this that could contain this to inside binding. This thread with an answer from Roman I thought would do the trick but still not working. CompletableDeferred might also be a solution but not sure how to incorporate it for this case.

I tried a bunch of solutions. Mainly around seeing was there a hack I could implement creating a second ResultBinding implementation for SuspendableBinding. No luck. All out of ideas right now so hoping others can help jump on this 😄

[michaelbull]

If we made binding wrap the operation in it's own CoroutineScope, as per this comment, would it help?

[Munzey]

@michaelbull ok got something working - will post soon

[Munzey]

@michaelbull so the trick was to force the job to cancel once an error is encountered. i was experimenting and tried

coroutineScope {
    this.cancel()
}

and i was finally seeing a differnet exception thrown (job cancellation exception)! at this point i realised i could get rid of throwing bind exception entirely and just switch to directly throwing our own cancellation exception.

The one downside to this is that CancellationException requires pulling kotlinx-coroutines-core-native into commonMain 😬

[michaelbull]

The one downside to this is that CancellationException requires pulling kotlinx-coroutines-core-native into commonMain

We're going to need a different solution then, as I think it's unacceptable to make this library inherently dependent on kotlin-coroutines. Maybe we have to split out the coroutines behaviour to a separate subproject?

[Munzey]

Maybe we have to split out the coroutines behaviour to a separate subproject?

yep sounds like its the only way then to ensure the correct behaviour for suspendable version of binding while not tying users of this lib to that dependency. ccing @ditn and @sottti as they may be interested in this

[Munzey]

@michaelbull if we went that route, would you be willing to host the repo and publish it? If so I'd happily open a pr to get this work moved over to it.

[michaelbull]

By a separate sub-project I mean a project within the same Gradle project, i.e. the same repository

[Munzey]

ah fair enough ! right that makes sense, just a separate folder with its own gradle build script?

[michaelbull]

The example directory is already it's own subproject (but isn't published), so along the same lines as that subproject.

Here's the Gradle docs on the subject: https://docs.gradle.org/current/userguide/multi_project_builds.html

[Munzey]

@michaelbull thanks for the link! I'm gonna look into putting up a separate pr to get this into a subproject then

[Munzey]

closing this. to be solved via #29