Kotlin / anko

Pleasant Android application development
Apache License 2.0
15.89k stars 1.29k forks source link

doAsync hides exceptions by default #271

Closed BoD closed 7 years ago

BoD commented 7 years ago

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 can be expected (for instance an IOException) then you can already put a try catch inside the lambda, or pass an exceptionHandler. But I am talking about the unexpected case (a bug in your code) - ignoring such a case by default is a bad idea IMHO. It makes the developer's life needlessly complicated.

Basically this makes it too 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 really think it would be better if the exception was not caught at all.

(This was originally discussed in issue #182)

rostopira commented 7 years ago

OMG, they caught it by default? Awesome, going to remove all that crappy try-catch blocks

Actually, I think it should log by default but also be configurable. But that just an IMHO

BoD commented 7 years ago

Related article about how you should not try to silently swallow exceptions: http://jeroenmols.com/blog/2017/03/08/appcrash/

yanex commented 7 years ago

Exceptions are logged by default:

private val crashLogger = { throwable : Throwable -> throwable.printStackTrace() }

You can also specify the custom error handler:

doAsync(exceptionHandler = { e -> reportException(e) }) {
    // Your asynchronous code
}
yanex commented 7 years ago

Also, you can use the kotlinx.coroutines library so you can set the completion handler:

launch { /* ... */ }.invokeOnCompletion { e ->
    // Handle an exception
}
palto-blubek commented 7 years ago

I'm not sure I understand what you mean by that. Are you saying there was a change since this issue was opened and now there's a default exception handler?

mtrewartha commented 7 years ago

Sorry to comment on a closed issue, but I didn't want to open yet another for discussion on this exact thing. I really agree here that exceptions should not be caught, because unintended exceptions (i.e. bugs) can then be captured by whatever exception handler a project has in place, like Crashlytics. I understand that you can now pass an exceptionHandler to doAsync(...), but I really don't want to rely on myself and every other dev on my team remembering to do so. I can think of two solutions, but maybe someone else can think of another:

  1. Make the default exception handler configurable, so that I can set it one time (e.g. in my Application, while I'm configuring logging and exception handling) and never have to remember to pass it to doAsync(...)
  2. Is there some way I can override the doAsync(...) methods with my own default exception handler and then delegate to Anko's implementation of the functions? I can't see a way to do it while keeping the doAsync names, but I'm still pretty new to Kotlin, so maybe there's a way I'm not thinking of.

Anywho, thank you guys/gals for a killer library 🤘 We love Anko.