Skip to content
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

CoroutineExceptionHandler installed in top(-most) scope not always used. #1157

Open
streetsofboston opened this issue Apr 29, 2019 · 12 comments
Labels
docs KDoc and API reference question

Comments

@streetsofboston
Copy link

streetsofboston commented Apr 29, 2019

I have this example code-snippet:

val xHandlerParent = CoroutineExceptionHandler { c, e ->
    println("Parent Handled Crash")
}

val xHandlerLeaf = CoroutineExceptionHandler { c, e ->
    println("Leaf Handled Crash")
}

fun test1() {
    // Exception throw up the call-stack. Expected
    CoroutineScope(Job()).launch {
        delay(1000)
        throw Exception("Some Exception 1")
    }

    Thread.sleep(2000)
}

fun test2() {
    // Exception handled by xHandlerParent: Expected
    CoroutineScope(xHandlerParent).launch {
        delay(1000)
        throw Exception("Some Exception 1")
    }

    Thread.sleep(2000)
}

fun test3() {
    // Exception handled by xHandlerLeaf: Expected?
    // Yes, since there is a top scope with a CoroutineExceptionHandler.
    // No,  since the top-most scope does not have CoroutineExceptionHandler.
    CoroutineScope(Job()).launch(xHandlerLeaf) {
        delay(1000)
        throw Exception("Some Exception 1")
    }

    Thread.sleep(2000)
}

fun test4() {
    // Exception handled by xHandlerLeaf. Unexpected.
    // Shouldn't it be handled by xHandlerParent instead?
    CoroutineScope(xHandlerParent).launch(xHandlerLeaf) {
        delay(1000)
        throw Exception("Some Exception 1")
    }

    Thread.sleep(2000)
}

fun test5() {
    // Exception handled by xHandlerParent: Expected
    CoroutineScope(xHandlerParent).launch {
        withContext(xHandlerLeaf) {
            delay(1000)
            throw Exception("Some Exception 1")
        }
    }

    Thread.sleep(2000)
}

fun test6() {
    // Exception handled by xHandlerParent: Expected?
    // Yes, since there is a top scope with a CoroutineExceptionHandler.
    // No,  since the top-most scope does not have CoroutineExceptionHandler.
    CoroutineScope(Job()).launch(xHandlerParent) {
        launch(xHandlerLeaf) {
            delay(1000)
            throw Exception("Some Exception 1")
        }
    }

    Thread.sleep(2000)
}

fun test7() {
    // Exception thrown up the call-stack. Expected?
    // Yes, since the top-most scope does not have CoroutineExceptionHandler.
    // No, since there is a top scope with a CoroutineExceptionHandler.
    CoroutineScope(Job()).launch {
        withContext(xHandlerLeaf) {
            delay(1000)
            throw Exception("Some Exception 1")
        }
    }

    Thread.sleep(2000)
}

My questions are:

  • Is the top-most scope always used and if it has CoroutineExceptionHandler, it will be used, otherwise, the exception is thrown up the call-stack?
  • Or is the top scope that has a CoroutineExceptionHandler used instead? Only if no such top scope can be found, the exception is thrown up the call-stack.
  • Regardless of the answer to the previous question, why does the example of test4() not use the xHandlerParent, since that is the CoroutineExceptionHandler of the top and top-most scope?
@streetsofboston streetsofboston changed the title CoroutineExceptionHandler not called in top-most parent-scope. CoroutineExceptionHandler installed in top(-most) scope not always used. Apr 29, 2019
@streetsofboston
Copy link
Author

@elizarov or @qwwdfsad Is there an issue with the CoroutineExceptionHandler or do I misunderstand how exceptions are handled when using launch?

@MartinDevi
Copy link

When you call CoroutineScope.launch you're launching the coroutine within a context which is the sum of the scope's context and the one passed as argument, where the argument's attributes overwrite those of the scope.

I think your test cases therefore make sense. In test3, the resulting context has a job and an exception handler. In test4, the exception handler in the argument overwrites the one in the scope, therefore it's invoked,

However, if a coroutine fails and has a parent in its scope, then it never invokes its uncaught exception handler, it propagates to its scope. I think that explains test5 and test6.

For withContext, it never invokes its uncaught exception handler. That only happens for launch. Hence the propagation in test7.

I think it's interesting to conceptually think that the coroutine propagates its exception to its scope rather than to its parent. It's the scope that can either have a parent capable of handling the exception or not, which determines the behavior.

@streetsofboston
Copy link
Author

streetsofboston commented May 13, 2019

@MartinDevi Thank you for your answer.

Reading your answer, I think my misunderstanding was based on mixing up 'parent-child relationship' and 'overwriting coroutine-context element'.

For example, In test4(), I thought the launch-call established a 'parent-child relationship', but it is overwriting the coroutine-scope's CoroutineExceptionHandler instead.

I must say, it is tricky to understand it all... easy to forget and make mistakes.... :)

@pakoito
Copy link

pakoito commented May 13, 2019

Could you please add some more examples using try/catch to capture some of the errors inside nested coroutines? We've been trying to understand the story about async error handling for a while too.

@streetsofboston
Copy link
Author

streetsofboston commented May 14, 2019

@pakoito
I wrote a set of unit-tests to figure out the handling of exceptions thrown by launch:

import kotlinx.coroutines.CoroutineExceptionHandler
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.delay
import kotlinx.coroutines.launch
import kotlinx.coroutines.test.TestCoroutineDispatcher
import kotlinx.coroutines.test.UncaughtExceptionCaptor
import kotlinx.coroutines.withContext
import org.hamcrest.MatcherAssert.assertThat
import org.hamcrest.Matchers.`is`
import org.hamcrest.Matchers.nullValue
import org.junit.jupiter.api.AfterEach
import org.junit.jupiter.api.BeforeEach
import org.junit.jupiter.api.Test
import kotlin.coroutines.AbstractCoroutineContextElement
import kotlin.coroutines.CoroutineContext

val xHandlerTopScope = TestCoroutineExceptionHandler()

val xHandlerOverride = TestCoroutineExceptionHandler()

val xHandlerChildScope = TestCoroutineExceptionHandler()

private object ThrownException : Throwable("ThrownException")

class ExceptionHandlingForLaunchTest {
    private val dispatcher = TestCoroutineDispatcher()

    private var uncaughtException: Throwable? = null

    @BeforeEach
    fun setup() {
        Thread.setDefaultUncaughtExceptionHandler { _, e -> uncaughtException = e }
    }

    @AfterEach
    fun tearDown() {
        xHandlerTopScope.cleanupTestCoroutines()
        xHandlerOverride.cleanupTestCoroutines()
        xHandlerChildScope.cleanupTestCoroutines()

        Thread.setDefaultUncaughtExceptionHandler(null)
        uncaughtException = null
    }

    @Test
    fun `When no CoroutineExceptionHandler is installed at all, exception is uncaught`() {
        CoroutineScope(dispatcher).run {
            launch {
                delay(1000)
                throw ThrownException
            }

            dispatcher.advanceTimeBy(1000)
            assertThat(uncaughtException, `is`(ThrownException as Throwable))
        }
    }

    @Test
    fun `When CoroutineExceptionHandler is installed in top-scope, exception is handled by top-scope handler`() {
        CoroutineScope(xHandlerTopScope + dispatcher).run {
            launch {
                delay(1000)
                throw ThrownException
            }

            dispatcher.advanceTimeBy(1000)

            assertThat(uncaughtException, nullValue())
            assertThat(xHandlerTopScope.uncaughtExceptions, `is`(listOf<Throwable>(ThrownException)))
        }
    }

    @Test
    fun `When CoroutineExceptionHandler is *added* to top-scope, exception is handled by the added handler`() {
        CoroutineScope(dispatcher).run {
            launch(xHandlerOverride) {
                delay(1000)
                throw ThrownException
            }

            dispatcher.advanceTimeBy(1000)

            assertThat(uncaughtException, nullValue())
            assertThat(xHandlerOverride.uncaughtExceptions, `is`(listOf<Throwable>(ThrownException)))
        }
    }

    @Test
    fun `When CoroutineExceptionHandler is *overridden* in top-scope, exception is handled by overriding handler`() {
        CoroutineScope(xHandlerTopScope + dispatcher).run {
            launch(xHandlerOverride) {
                delay(1000)
                throw ThrownException
            }

            dispatcher.advanceTimeBy(1000)

            assertThat(uncaughtException, nullValue())
            assertThat(xHandlerTopScope.uncaughtExceptions, `is`(emptyList()))
            assertThat(xHandlerOverride.uncaughtExceptions, `is`(listOf<Throwable>(ThrownException)))
        }
    }

    @Test
    fun `When CoroutineExceptionHandler is added to child-scope, exception is still handled by top-scope`() {
        CoroutineScope(xHandlerTopScope + dispatcher).run {
            launch {
                withContext(xHandlerChildScope) {
                    delay(1000)
                    throw ThrownException
                }
            }

            dispatcher.advanceTimeBy(1000)

            assertThat(uncaughtException, nullValue())
            assertThat(xHandlerTopScope.uncaughtExceptions, `is`(listOf<Throwable>(ThrownException)))
            assertThat(xHandlerChildScope.uncaughtExceptions, `is`(emptyList()))
        }
    }

    @Test
    fun `When CoroutineExceptionHandler is added to child-scope, exception is still handled by overriding top-scope`() {
        CoroutineScope(dispatcher).run {
            launch(xHandlerOverride) {
                launch(xHandlerChildScope) {
                    delay(1000)
                    throw ThrownException
                }
            }

            dispatcher.advanceTimeBy(1000)

            assertThat(uncaughtException, nullValue())
            assertThat(xHandlerOverride.uncaughtExceptions, `is`(listOf<Throwable>(ThrownException)))
            assertThat(xHandlerChildScope.uncaughtExceptions, `is`(emptyList()))
        }
    }

    @Test
    fun `When no top-scope CoroutineExceptionHandler is installed, added or overridden, exception always remains uncaught`() {
        CoroutineScope(dispatcher).run {
            launch {
                launch(xHandlerChildScope) {
                    delay(1000)
                    throw ThrownException
                }
            }

            dispatcher.advanceTimeBy(1000)

            assertThat(uncaughtException, `is`(ThrownException as Throwable))
            assertThat(xHandlerChildScope.uncaughtExceptions, `is`(emptyList()))
        }
    }
    
    @Test
    fun `When top-scope CoroutineExceptionHandler is installed, added or overridden, exception is always handled by top-scope`() {
        CoroutineScope(xHandlerTopScope + dispatcher).run {
            launch {
                launch(xHandlerChildScope) {
                    delay(1000)
                    throw ThrownException
                }
            }

            dispatcher.advanceTimeBy(1000)

            assertThat(uncaughtException, nullValue())
            assertThat(xHandlerTopScope.uncaughtExceptions, `is`(listOf<Throwable>(ThrownException)))
            assertThat(xHandlerChildScope.uncaughtExceptions, `is`(emptyList()))
        }
    }
}

class TestCoroutineExceptionHandler :
    AbstractCoroutineContextElement(CoroutineExceptionHandler), UncaughtExceptionCaptor, CoroutineExceptionHandler {
    private val _exceptions = mutableListOf<Throwable>()

    override fun handleException(context: CoroutineContext, exception: Throwable) {
        synchronized(_exceptions) {
            _exceptions += exception
        }
    }

    override val uncaughtExceptions
        get() = synchronized(_exceptions) { _exceptions.toList() }

    override fun cleanupTestCoroutines() {
        synchronized(_exceptions) {
            _exceptions.clear()
        }
    }
}

Hope this helps.
Next one up is figuring out how async handles exceptions. :-)

@qwwdfsad
Copy link
Member

This part of coroutines machinery is not well documented in details. We are currently working internally on the advanced exception handling mechanisms that will be easier to use and reason about.
The rule of thumb: CoroutineExceptionHandler should be treated similarly to Thread.uncaughtExceptionHandler that is invoked as the last-ditch effort to report an exception and then crash (e.g. default EH on Android just crashes an application). Thus it is better not to rely on handlers hierarchy and/or particular order.

Is the top-most scope always used and if it has CoroutineExceptionHandler, it will be used, otherwise, the exception is thrown up the call-stack?
Or is the top scope that has a CoroutineExceptionHandler used instead? Only if no such top scope can be found, the exception is thrown up the call-stack.
Regardless of the answer to the previous question, why does the example of test4() not use the xHandlerParent, since that is the CoroutineExceptionHandler of the top and top-most scope?

  1. There are two types of coroutines: the first type handles its own exception (e.g. launch), the second ignores or rethrows it (async or withContext). Only the former use CoroutineExceptionHandler.

  2. When launch is invoked, it inherits exception handler from the parent scope ("default" hander just invokes Thread.currentThread().uncaughtExceptionHandler.uncaughtException(exception).
    If launch is invoked with an exception handler (launch(xHandlerLeaf)), it overwrites an exception handler from the parent. When launch crashes, it uses an exception handler from its context (overwritten, if it was present, or inherited otherwise)

@qwwdfsad qwwdfsad added docs KDoc and API reference question labels May 14, 2019
@streetsofboston
Copy link
Author

Thank you @qwwdfsad !

About point (2).
If launch always overwrite/overrides the exception-handler, then this test method should fail:
fun 'When CoroutineExceptionHandler is added to child-scope, exception is still handled by overriding top-scope()'

From that test it seems that the CoroutineExceptionHandler in the top-most scope (whether it was directly installed or overwritten/overridden by the arguments to its launch method) is always used.
It seems to be never overwritten/overridden by child-scopes' CoroutineExceptionHandlers.

@qwwdfsad
Copy link
Member

qwwdfsad commented May 15, 2019

'When CoroutineExceptionHandler is added to child-scope, exception is still handled by overriding top-scope()'

Code:

launch(xHandlerOverride) { // (1)
    launch(xHandlerChildScope) { // (2)
        delay(1000)
        throw ThrownException
    }
}

This is a bit more advanced case. (2) knows that it has a parent (1) that is able to handle an exception, so it delegates all exception handling to it. This is indeed the least obvious part of this design, this is why we are working on a more robust mechanism.

You are trying to formulate the single rule like "the handler from the topmost scope is used" and that's where the confusion comes from, the actual rule has more than one variable (parent scopes, their context and whether a parent is able to handle an exception)

@pakoito
Copy link

pakoito commented May 15, 2019

With the Context being a set, as an arbitrary suspend function, could I override the parent handler? or as a child coroutine?

@elizarov
Copy link
Contributor

@pakoito Context is an immutable set. It cannot be affected by children of by anything at all after coroutine is created. If a coroutine is a child, then its CEH does not matter, since it would never use it itself.

@pakoito
Copy link

pakoito commented May 15, 2019

Okay, that makes sense. Now, correct me if I'm wrong.

Only the creator is allowed to install new CEHs and if you want your own handling you have to use withContext to create, launch, and await a new child coroutine inline. But that'd override the current CEH if there's one (i.e. Activity-wide one that logs prod errors), so you have to make sure to have a CEH that can compose both of them together and that whomever calls withContext remembers to compose with the existing one. Same for CoroutineInterceptors or other unique keys in the set.

Also, will any new launch inherit the CEH from the parent, or that's just for withContext?

@elizarov
Copy link
Contributor

elizarov commented May 15, 2019

Only the creator is allowed to install new CEHs and if you want your own handling you have to use withContext to create, launch, and await a new child coroutine inline.

Yes.

But that'd override the current CEH if there's one (i.e. Activity-wide one that logs prod errors), so you have to make sure to have a CEH that can compose both of them together and that whomever calls withContext remembers to compose with the existing one. Same for CoroutineInterceptors or other unique keys in the set.

Yes.

Also, will any new launch inherit the CEH from the parent, or that's just for withContext?

All builders inherit all context keys from the scope.

However, one correction here: If you you do launch(newCEH) { .... } it does not meat that the newCEH is actually going to be used, since first and foremost exception handling is delegated to parent. CEH really only matters for coroutines without parent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs KDoc and API reference question
Projects
None yet
Development

No branches or pull requests

5 participants