[Proposal] Global error handler for execute functions

[jakoss]

I have the requirement to trace all errors in app, even if those are properly handled by the code and UI. So i need to create some global error handler for all execute function, that will be called on Fail reducer set. My proposal:

Add new property to MavericksViewModelConfig with signature something like:

val executeErrorHandler: ((Exception) -> Unit)? = null

I'm not sure that should be called handler, since it wouldn't have any impact on error handling, it's just for logging/tracing purposes. Maybe name executeErrorLogger would be more appropriate?

Next, i'd add the same field to MavericksViewModelConfigFactory that would be passed down to configs.

In MavericksViewModel i'd add call config.executeErrorHandler?.invoke(e) after setState { reducer(Fail(e ... for all execute functions.

What do you think about that? If it's fine i'll create a PR with that.

Also, i have a minor question about error handling - is there a particular reason to catch Exception instead of Throwable?

[gpeal]

@jakoss How would you propose failures would be detected. Reducers are opaque to Mavericks. We don't know what each one does and wouldn't be able to tell you where the error originated from either.

re: Throwable. We could probably switch to that if it's causing issues. It just hasn't caused any issues (that we've heard of) yet.

[jakoss]

I wouldn't detect failure on reducer, just after it in execute functions:
here:

mavericks/mvrx/src/main/kotlin/com/airbnb/mvrx/MavericksViewModel.kt

Line 230 in f4b7492

setState { reducer(Fail(e, value = retainValue?.get(this)?.invoke())) }

and here:

mavericks/mvrx/src/main/kotlin/com/airbnb/mvrx/MavericksViewModel.kt

Line 262 in f4b7492

return catch { error -> setState { reducer(Fail(error, value = retainValue?.get(this)?.invoke())) } }

We have Exception there.

In my experience with android i had 2 situations with code that crashed on production (native library integration issue) just because i was catching Exception instead of Throwable. Since Throwable handles basically everything bad that can happen it might be safer, especially if user assumes that execute is totally safe in that regard

[gpeal]

@elihart What do you think of adding a config for this?

@jakoss I'd be open to a PR to change Exception to Throwable

[elihart]

This all sounds great to me, thanks for laying out a clear plan. Agree with Throwable.

executeErrorLogger isn't bad, but the implementation doesn't necessarily need to use it for logging, so we can pick a more generic name. Agree that handler is misleading. Maybe AsyncFailCallback, to represent that this is more the Fail case, and open it up to any other places we may use Fail besides execute.

instead of executeErrorHandler?.invoke(e) we can do onFail, and I think we can pass the Fail object directly. It may also be be good to pass a reference to the viewmodel in case any handling wants that info (otherwise it can be hard to tell where the issue came from).

Do you think there is any possibility of needing this for Success and/or Loading states too? If so we could make the callback interface have those too, or at least design the interface so they can be added later if needed (I maybe prefer the latter)

[jakoss]

So maybe an interface like this:

interface AsyncEventsListener {
    fun onSuccess(event: Success<*>) {}
    fun onLoading(event: Loading<*>) {}
    fun onFail(event: Fail<*>) {}
}

Default method implementations might be handy if someone wants to listen only to one/two of events. I can go through the codebase and find all places that can emit Async events and invoke proper methods there.

I can replace Exception with Throwable in the same PR if that's ok with you

[gpeal]

I'm worried that this could introduce some unexpected bugs. If somebody were to ever change an execute to a setOnEach, they would lose this functionality. Is that an acceptable tradeoff?

[elihart]

setOnEach doesn't use Async, so it doesn't seem like a change that would really happen, would it?

If you can think of a more generic interface that would capture that use case though then all the better

[jakoss]

My original idea is to make Async more useful, I haven't considered other states. I'm not sure if there's a similar benefit from tracking everything from setOnEach

[gpeal]

@elihart It doesn't inherently but it's not uncommon to use Async as an async result type outside of the context of execute. I'm still open to this but worried that somebody might do

val something= flowOf<Async<Foo>>()
something.setOnEach { copy(something = it) }

At Tonal, some of our lower level data streams emit async so sometimes execute calls are converted to setOnEach.

[jakoss]

So maybe in setOnEach something like this?

onEach {
    if (it is Async<*>){
        // report to logger 
    }
} 

[gpeal]

@jakoss @elihart What is the expected thing to do with the handed Async values? In many cases, an Async value in isolation doesn't have enough context to make it clear where it originated from which could lead to some very challenging debugging.

[jakoss]

The only thing I care about is the exception and stack trace. I can do some experiments how it will look

[gpeal]

@jakoss That might be okay for your project but I'm worried that it doesn't work well for many/large apps. In many cases, generic IOExceptions from room/retrofit might not provide enough information to help debug an issue.

[jakoss]

I agree, I don't think it will give a lot of feedback. But it can be useful for example for error occurrence statistics, to issue an warning if errors are getting out of hand. It's not a powerful debug tool, just a hint that something might be wrong

[jakoss]

Other idea - maybe we can pass viewModel instance as a parameter for listeners? That can give a little bit of context. Or even allow to pass optional parameter to execute, something like DebugInfo class that could store context info for listeners (although I think this could be just Any reference, so user can store anything he wants for debug)

[elihart]

Yes I think it would be good to pass the viewmodel reference to the listener. I suggested that in an earlier comment.

I don't think I'm in favor of including debug information in the execute function. Instead you could have an extension function on Async that you invoke in your execute callback like execute { result -> result.logIfFail("some context"); copy(... }

It would be nice to automatically include context on the execute call site to the global handler so that doesn't need to be done in each place, though I'm not sure how we might do that

[jakoss]

I have no more ideas here 😅

[gpeal]

@jakoss You could also create a function in your own base view model like executeWithLogging or whatever you want to call it.

// pseudocode
fun executeWithLogging(reducer: S.(T) -> S) {
    execute { value ->
        if (value is Fail) log(...)
        reducer(value)
    }
}

[jakoss]

Sure, I can solve this using extension if you don't see a value in global handler. No problem with that

[elihart]

Maybe it's best to go with your own execute extension for now, and we can leave this proposal open to see if any other people weigh in with similar use cases. I'm still open to a global handler, but agree that the use case should be strong and the API should be flexible for different use cases

[jakoss]

Sure. And what about Throwable instead of Exception? Is it worth it for a dedicated PR?

@gpeal How open adding open to execute functions? I have my abstract class BaseMavericksViewModel that i use everywhere, overriding execute could work for me perfectly

[gpeal]

@jakoss Sure, feel free to put up a PR for Throwable

@elihart I think making execute open would be okay. Any objections?