Skip to content

Conversation

dbottillo
Copy link
Contributor

Fix for #128 .

When running nested coroutine bindings in parallel, an exception in one will trigger a BindCancellationException in the other, essentially leaking the exception over. Then the exception handler code is crashing because receiver.result may not be ready. A diagram explaining the issue:
465261096-e415fd4f-ae7f-42cc-a62b-813f2c393ccc

@michaelbull
Copy link
Owner

Thanks so much for the hard work on this @dbottillo. The diagram was very helpful.

I have one question: my understanding is that we're testing for single-nested bindings (i.e. one inside of the other), however do we need to test for further levels of nesting to ensure that errors are only propagated upwards by a single level, and don't follow the chain upwards automatically?

e.g.

val a = coroutineBinding {
    val b1 = coroutineBinding {
        val c = coroutineBinding {
            Err("c value").bind()
         }

        assertEquals(c, Err("c value"))

        Ok("b value")
     }

    assertEquals(b1, Ok("b value"))

    val b2 = Err("a value").bind()

    Ok(Unit)
}

assertEquals(a, Err("a value"))

@dbottillo
Copy link
Contributor Author

Thanks so much for the hard work on this @dbottillo. The diagram was very helpful.

I have one question: my understanding is that we're testing for single-nested bindings (i.e. one inside of the other), however do we need to test for further levels of nesting to ensure that errors are only propagated upwards by a single level, and don't follow the chain upwards automatically?

e.g.

val a = coroutineBinding {
    val b1 = coroutineBinding {
        val c = coroutineBinding {
            Err("c value").bind()
         }

        assertEquals(c, Err("c value"))

        Ok("b value")
     }

    assertEquals(b1, Ok("b value"))

    val b2 = Err("a value").bind()

    Ok(Unit)
}

assertEquals(a, Err("a value"))

good shout! I've added a test to cover that scenario :)

@michaelbull
Copy link
Owner

Merged in d952b52

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants