Anko: Exception in an async lambda

[xxdavid]

First of all, thank you for this great library!

When there's an exception thrown in an async lambda, the lambda ends its execution but nothing else happens. Application doesn't crash, nothing is logged.

It's very hard to debug this kind of bugs. I personally would be much more happy if the app just crashed.

Is there anything that could be done with that?

[yanex]

exceptionHandler parameter was added to async() functions.

[kingsleyadio]

Hello,
I was checking through the commits. I noticed async is being replaced with doAsync, and the exception handler mechanism added. However I have a few reservations

1. I actually do prefer async to the new doAsync. Are there any reasons for this change?
2. The exception handler function should also be called on the UI thread, and the same restrictions as with uiThread should apply, since some UI states might also need to be updated
3. The exceptionHandler argument feels like something that should be placed after the completion handlers instead of before, as it currently is. Since it's optional anyway. Having completion before exception also kind of feels more natural
4. What are possible situations where the new onComplete will be useful? Since at that point, we might not even have any reference to the caller anymore, and executing stuff might cause some random crashes here and there

On a slightly different note, I am thinking of a situation where both exceptions and results are made available at the time of executing uiThread or its other variants.
Since we can have valid results (e.g from an API call), but they might not necessarily be successful responses, having both completion and exception handler means we will have to handle exceptional cases in two different sections of the code.

I haven't really thought about how the implementation of a case like this will be, but usage could be something like:

async() {
  var (result, exception) = exec { /* Do long running task */ }
  uiThread {
    // React from the UI thread
  }
}

Do you think these are viable considerations?

[Ribesg]

1. async became a reserved* word in Kotlin. It was intended that we only had to write async { and not async() { in Anko originally. Changing to doAsync allows us to write doAsync { without parentheses.
2. The exception handler function does not have to be called on the UI thread imho. If I just want to log the exception with something which does not care about which thread it's being called from (like Crashlytics), then it's better to not run something on the UI thread if you don't have to. (Makes sense with my answer to 3. below)
3. For me the exceptionHandler is a way to set a standard "catch all unexpected" exception handler. If you expect an exception, you should use a try-catch in the doAsync { bloc. The exceptionHandler may be some kind of global variable stored somewhere in your Application, which basically logs the exception as it should not occur. val exceptionHandler = { e -> Crashlytics.logException(e) }
4. uiThread will not be called if the context "disappeared". onComplete will be called with a null argument. It's the only difference between the two inside a doAsync { afaik.

*: not really, but still

[kingsleyadio]

1. Ah. True. That makes sense.
2. More often than not, I show the user a kind of progress indicator when I'm about to perform a long running task. Once this task completes (due to successful run or an exception), one of the first things to do is to dismiss the indicator and notify them of the result, which happens on the UI thread. So I would expect any callback on completion of my task to naturally take place on the UI thread. I suppose Crashylitics and co usually queue the heavy logging work to run on some background thread (not particularly sure about Crashylitics internal implementation, but seems like a logical approach), so I don't see a problem with calling it from the UI.
3. As I explained in 2, cleaning up the UI is a higher priority, I can then do other stuff in the background if I really feel it will take a while to complete
4. True. I suppose there could be some non UI work that one might really want to do

With the example I described initially, since I have both result and exception, I could just do the non UI work outside the uiThread block, and everybody will be fine. This is inline with 2 & 3 above. Something like this:

doAsync {
  var (result, exception) = exec { /* Do long running task */ }
  // Do non UI work
  uiThread {
    // React from the UI thread
  }
  // Do more non UI work. e.g Crashylitics.logException(exception)
}

A possible implementation of exec could go like:

fun <T, R> AnkoAsyncContext<T>.exec(f: () -> R): Pair<R?, Throwable?> {
    var result: R? = null
    var exception: Throwable? = null
    try {
        result = f()
    } catch(t: Throwable) {
        exception = t
    }
    return result to exception
}

[yanex]

@kingsleyadio Sure, you can just not to use the exceptionHandler functionality.
exec() function is interesting, but I think it's completely irrelevant to AnkoAsyncContext. You can actually use it in cases like: exec { string.toInt() }.value, converting the "dangerous" function call into a nullable result.
I like the exec() function, though the name is too general. How about attempt()?

[kingsleyadio]

@yanex
Truly, exec can exist completely independent of AnkoAsyncContext. I can see myself using it in a number of situations, a lot more than I would use the exceptionHandler (personal preference though). Even then, there is no reason why both of them cannot co-exist

I'll love to see a standard function like it in anko, maybe even in the Kotlin stdlib :)
Yes, attempt() actually sounds more like it. :100:

[kingsleyadio]

By the way @Ribesg, I see your point with the global exceptionHandler variable. :+1:

In fact, in some other non async() context, say we already declared

val exceptionHandler = { e: Throwable -> XYZ.log(e) }

One could use it together with a try/catch block like

try {
  // Do something that could throw an exception
} catch (ex: Exception) {
  exceptionHandler(ex)
}

[BoD]

Sorry to comment on a closed issue but I think the current default implementation (doing nothing, not even a log) is a bad idea.

I really think the default behavior should be to not catch exceptions - that is, let them be thrown, which in turn will 1/ make the exception reported by tools like Crashlytics 2/ make the app crash.

If an exception is "normal" (expected) then you can already, as mentioned in a previous comment above, put a try catch inside the lambda. But I am talking about the abnormal (bug) case - ignoring such a case by default is a mistake IMHO. It makes the developer's life needlessly complicated.

Basically this makes it easy to miss bugs :)

Also this would be in line with Android's standard behavior (both in Thread and AsyncTask).

At the very minimum the exception should be logged - but I'd really prefer if the exception was not caught at all.

[BoD]

Should I open a new issue instead then?

[Ribesg]

I think it's an idea a lot of people would obviously agree with, default behavior on error should not be silent. I guess opening a new issue would give more visibility.