Add handler for exception that occurs within run?

[bitspittle]

Right now, if an exception occurs within run, the program just exits. A user can themselves wrap everything inside a try / catch block (within the run block), but it might be nice if we had a method, onFailure maybe, similar to onFinished

Edit: Added "within the run block" for clarity

[bitspittle]

Been using Kotter just fine without this, not a 1.0 blocker.

[cedriessen]

Unfortunately you cannot just wrap everything in a try/ catch since exceptions thrown in the run block are not propagated to the main thread.

[bitspittle]

Yes, good point. I'll look at this again and implement it if it's not too tricky.

[bitspittle]

So, looked into this a bit more.

In my local development work, I actually got it so that the run block would propagate its exception to the main thread:

class UniqueException : Exception

try {
   section { }.run { throw UniqueException() }
} catch (ex: UniqueException)

However, exceptions in the section do not work like this (and should not work, by design, more on that below):

class UniqueException : Exception

// DOES NOT WORK
try {
   section { throw UniqueException() }.run()
} catch (ex: UniqueException)

The reason the section version does not work is because some renders happen asynchronously:

var value by liveVarOf(0)
section {
   // This renders immediately when "run" is first called
   textLine(value.toString())
}.run {
   value++ // Causes async render request #1 
   requestRerender() // Causes async rerender request #2
   requestRerender() // Causes async rerender request #3
}

Renders are async so that unnecessary intermediate renders can be ignored. The above example may only rerender once or maybe three times, based on threading timing.

---

So now, we have two blocks backed by their own threads - the section block (runs on a special render thread) and the run block (runs on a background thread).

Maybe I'm wrong, but it seems really confusing that an exception in the run block should propagate to the main thread, while an exception in the section block should not.

In conclusion, I'm thinking the best solution is just to handle exceptions explicitly:

section {
   try { dangerousRenderCode() } catch (ex: ...)
}.run {
   try { dangerousRunCode() } catch (ex: ...)
}

My thinking is, exception handling in Kotter blocks should be fairly rare, and being explicit makes it less confusing and more consistent. Leaving things as is, both blocks would swallow any unhandled exceptions they have.

P.S. It's possible my wording in the original comment was confusing, when I said "A user can themselves wrap everything inside a try / catch block", but I meant "inside a try / catch block within the run block"

[bitspittle]

Closing this issue for now, but feel free to leave more feedback / usecases I hadn't considered if you disagree.

[cedriessen]

Ok, I understand your reasoning. And since it is easy to create an extension function to Section that does the catching it is probably best to leave to api as is.
My use case for this is that first I wanted to get rid of all the try catch boilerplate leading to another round of indentation and second that I can establish one centralized error formatting and output at the top.
Thanks for looking into that.

[bitspittle]

By the way, feel free to share some example code with me. I'd love to see your extension method if you write it and the project is public. It's definitely possible I'm just not seeing something because I'm too close to the code here.

[cedriessen]

Yes sure. Find below a stripped version showcasing only the rethrowing. As I mentioned earlier I need all uncaught exception to bubble up in order to establish some centralized error output. To elaborate a bit: I am using Kotter in conjunction with Clikt to write some CLI tool. But not each and every command uses Kotter for its output, so handling exceptions at the top is the easiest way for me to have that said error output.

fun Section.runThrowing(block: suspend RunScope.() -> Unit) {
    var exception: Throwable? = null
    run {
        try {
            block()
        } catch (e: Throwable) {
            exception = e
        }
        // make sure any aside sections get rendered
        // see https://github.com/varabyte/kotter/issues/86
        delay(500)
    }
    exception?.let { throw it }
}

[cedriessen]

My code actually looks a bit different. I created a replacement extension function for Session.section returning a SectionExtension which in turn feature the above runThrowing function. This way I am able to create and control some common "components" (a progress bar and a timer) passing them via SectionExtension.

This showed me how easily Kotter can be customized :-)

[bitspittle]

Thanks for the context! And are you not worried about exceptions that originate within sections? I can definitely have exception propagation work in just run blocks, I just wasn't sure if that inconsistency would be confusing.

[bitspittle]

This showed me how easily Kotter can be customized :-)

Awesome! I definitely made sure (with a few exceptions using the internal keyword) that I had to implement my features using the exact same tools as all users. Hopefully that paid off here 👍

[cedriessen]

And are you not worried about exceptions that originate within sections?

No. Unlike run where all kinds of exceptions caused be unexpected behavior like IO issues and what not might occur I would consider exceptions in section to be just a bug. I kept my section implementations free from any stuff other than simple output logic.

What I did though is to catch and render exceptions occurring in section but just so that it's easier to fix the bug.

[cedriessen]

Also I didn't find a way from the "outside" to catch and propagate any exception in section to the main thread.

[bitspittle]

I'm thinking about using the coroutine library as an example and adding a run exception handler when you create a session, so maybe you can do something like this:

session(runExceptionHandler = ...) {
   section { ... }.run { ... }
   section { ... }.run { ... }
}

which would be backwards compatible and less confusing because you explicitly signed up for it.

That said, it may just be easy enough to have run start throwing exceptions directly. That honestly might be OK. I can't imagine a situation where you'd want your program to continue if the run method crashes.

Give me a bit more time to chew on it. I'll reopen this bug as I think about it. I'm more and more leaning to just having run throw.

[bitspittle]

Also I didn't find a way from the "outside" to catch and propagate any exception in section to the main thread.

Yeah, this is not trivial, because the code in section is essentially separated from the main thread. When you create a section, you are really calling a simple function that saves your callback under the hood and then exits immediately. At that point, the callback is set up to run a separate thread as soon as it can (and it may get triggered additional times if more render requests come in).

It's only when you call run that execution blocks. That's why, when an exception happens there, it's easy to report it, because you just crash the main thread at the point run was called.

The only thing I can think of at the moment to report section exceptions is for you to wrap a section block with a try / catch, add exceptions to a list if they happen, and then when the run method finishes, check if the list is not empty. If it is not, throw any deferred exceptions then. Probably not worth it...

[bitspittle]

OK, good news. This conversation convinced me 1) that run should throw an exception on the current thread and, in fact, that 2) I overcomplicated my implementation of run

Before, I did something like this:

fun run(block: suspend () -> Unit) {
   val job = CoroutineScope(Dispatchers.Default).launch { 
     block()
   }
   runBlocking { job.join() }
}

because in my mental model, I kept thinking about run as a background thread to do long calculations on. Actually, the only thing that's really important is that it doesn't block rendering, but it won't because rendering itself already happens on a separate render thread.

So the code will essentially get simplified to:

fun run(block: suspend () -> Unit) {
   runBlocking { block() }
}

which has the same behavior as before but means any exceptions thrown will propagate back to you without any crazy tricks.

Thanks for giving me the chance to think this through. I already have it working locally; I'll upload a fix soon. And then I'll see if it makes sense to update the Kotter version soon!

[cedriessen]

Awesome! Thanks for taking the time to look into it. Really appreciated.

[bitspittle]

By the way, 1.0.0-rc1 was just published, which contains this fix.

…

On Sat, Oct 8, 2022, 4:45 AM Christoph Drießen ***@***.***> wrote: Awesome! Thanks for taking the time to look into it. Really appreciated. — Reply to this email directly, view it on GitHub <#71 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKNONASAHTI63GYY34CXG3DWCFNFBANCNFSM5JNUJ67A> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[cedriessen]

Thanks again. I will give it an immediate shot tomorrow.