Skip to content

Fix lost exception from dispatched withContext on cancelled job #681

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 8, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions common/kotlinx-coroutines-core-common/src/Builders.common.kt
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ public suspend fun <T> withContext(
}
}
// SLOW PATH -- use new dispatcher
val coroutine = DispatchedCoroutine(newContext, uCont) // MODE_DISPATCHED
val coroutine = DispatchedCoroutine(newContext, uCont) // MODE_ATOMIC_DEFAULT
coroutine.initParentJob()
block.startCoroutineCancellable(coroutine, coroutine)
coroutine.getResult()
Expand Down Expand Up @@ -292,7 +292,7 @@ private class DispatchedCoroutine<in T>(
context: CoroutineContext,
uCont: Continuation<T>
) : ScopeCoroutine<T>(context, uCont) {
override val defaultResumeMode: Int get() = MODE_CANCELLABLE
override val defaultResumeMode: Int get() = MODE_ATOMIC_DEFAULT

// this is copy-and-paste of a decision state machine inside AbstractionContinuation
// todo: we may some-how abstract it via inline class
Expand Down
24 changes: 24 additions & 0 deletions common/kotlinx-coroutines-core-common/test/CoroutineScopeTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,30 @@ class CoroutineScopeTest : TestBase() {
}
}

@Test
fun testCoroutineScopeCancellationVsException() = runTest {
expect(1)
var job: Job? = null
job = launch(start = CoroutineStart.UNDISPATCHED) {
expect(2)
try {
coroutineScope {
expect(3)
yield() // must suspend
expect(5)
job!!.cancel() // cancel this job _before_ it throws
throw TestException1()
}
} catch (e: TestException1) {
// must have caught TextException
expect(6)
}
}
expect(4)
yield() // to coroutineScope
finish(7)
}

@Test
fun testScopePlusContext() {
assertSame(EmptyCoroutineContext, scopePlusContext(EmptyCoroutineContext, EmptyCoroutineContext))
Expand Down
24 changes: 24 additions & 0 deletions common/kotlinx-coroutines-core-common/test/SupervisorTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,30 @@ class SupervisorTest : TestBase() {
assertTrue(parent.isCancelled)
}

@Test
fun testSupervisorScopeCancellationVsException() = runTest {
expect(1)
var job: Job? = null
job = launch(start = CoroutineStart.UNDISPATCHED) {
expect(2)
try {
supervisorScope {
expect(3)
yield() // must suspend
expect(5)
job!!.cancel() // cancel this job _before_ it throws
throw TestException1()
}
} catch (e: TestException1) {
// must have caught TextException
expect(6)
}
}
expect(4)
yield() // to coroutineScope
finish(7)
}

private class TestException1 : Exception()
private class TestException2 : Exception()
}
54 changes: 54 additions & 0 deletions common/kotlinx-coroutines-core-common/test/WithContextTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,60 @@ class WithContextTest : TestBase() {
}
}

@Test
fun testRunCancellationUndispatchedVsException() = runTest {
expect(1)
var job: Job? = null
job = launch(start = CoroutineStart.UNDISPATCHED) {
expect(2)
try {
// Same dispatcher, different context
withContext(CoroutineName("testRunCancellationUndispatchedVsException")) {
expect(3)
yield() // must suspend
expect(5)
job!!.cancel() // cancel this job _before_ it throws
throw TestException()
}
} catch (e: TestException) {
// must have caught TextException
expect(6)
}
}
expect(4)
yield() // to coroutineScope
finish(7)
}

@Test
fun testRunCancellationDispatchedVsException() = runTest {
expect(1)
var job: Job? = null
job = launch(start = CoroutineStart.UNDISPATCHED) {
expect(2)
try {
// "Different" dispatcher (schedules execution back and forth)
withContext(wrapperDispatcher(coroutineContext)) {
expect(4)
yield() // must suspend
expect(6)
job!!.cancel() // cancel this job _before_ it throws
throw TestException()
}
} catch (e: TestException) {
// must have caught TextException
expect(8)
}
}
expect(3)
yield() // withContext is next
expect(5)
yield() // withContext again
expect(7)
yield() // to catch block
finish(9)
}

@Test
fun testRunSelfCancellationWithException() = runTest(unhandled = listOf({e -> e is AssertionError})) {
expect(1)
Expand Down