Repository navigation
Try/catch on generated override of @Background and @UiThread hiding exception from global exception handler. #646
Description
Activity
One of my peers suggested that perhaps the framework could provide some mechanism to control logging on the application scope, since there other instances where the try/catch/log code is generated besides the @background and @UiThread annotations I mentioned above.
For example, the following is code from the fragment injection:
private void injectFragmentArguments_() { Bundle args_ = getArguments(); if (args_!= null) { if (args_.containsKey("VIEW_TYPE")) { try { viewType = ((ViewType) args_.getSerializable("VIEW_TYPE")); } catch (ClassCastException e) { Log.e("LogInFragment_", "Could not cast argument to the expected type, the field is left to its default value", e); } } } }
Maybe some hook that would allow the application/activities/fragments to log/handle these exceptions or not.
I didn't mean to close the issue. Sorry...
I agree. I had this kind of feature in my mind for a while but couldn't figure how to do this.
Right now, I'm thinking of this solution :
- Adding an
AAExceptionHandlerinterface - Use a given implementation of this interface in method parameters to handle exception as we want.
public interface AAExceptionHandler { void onExceptionCatched(Exception e); }
A generated code for
@Backgroundannotated method with an exception handler should looks like this :@Override public void aBackgroundMethod(final AAExceptionHandler exceptionHandler) { BackgroundExecutor.execute(new BackgroundExecutor.Task("", 0, "") { @Override public void execute() { try { BackgroundActivity_.super.addSerializedBackgroundMethod(i); } catch (RuntimeException e) { exceptionHandler.onExceptionCatched(e); } } }); }
EDIT: The only problem is to pass the handler on every call to the annotated method... An automatic solution could be great.
- Adding an
You could have an interface on the Annotated class for handle exception, returning a boolean saying handled / not handled.
@Override public boolean onExceptionCaught(String reason, Exception e) { if (!super.onExceptionCaught(reason, e)) { Log.e("MyFragment_", reason, e); } } @Override public void aBackgroundMethod() { BackgroundExecutor.execute(new BackgroundExecutor.Task("", 0, "") { @Override public void execute() { try { BackgroundActivity_.super.addSerializedBackgroundMethod(i); } catch (RuntimeException e) { BackgroundActivity_.this.onExceptionCaught("A runtime exception was thrown while executing code in a runnable", e); } } }); }
Probably a clever way to see if a super class implements the interface too.
That's a good idea too 👍
However, if we have many@Backgroundmethods, it should be useful to easily know from which one the exception came from. But we can't rely on the method name in a String (because it's can't be refactored) nor reflection (because it's not efficient)
We could also mix our two ideas to handle the full use case :)Note: I updated your post to remove the unused
exceptionHandlerparameter and replacethis.onExceptionCaughtbyBackgroundActivity_.this.onExceptionCaughtwhich is the correct way to call this methodFor very custom exception handling, it's probably easier to put the try/catch in the method itself.
Our (Quenio and I) need today is just the ability to have a blanket exception handler.Yep. You're right. I'll work on this ASAP, but if you want to do it, tell me and go ahead :)
I've just implemented a global exception handler and I was surprised to note that AA catches exception in
@Background/@UIThreadannotated method. I think it's very intrusive, catching and logging ok that's fine, but why not rethrow the exception !
For a convinced AA user like me, it's shocking :), I think this implementation is outside the AA philosophy.I see the fixes currently in progress, but why not simply propagate the exception ?
I agree with @tbruyelle. I'd rather see my app crashing and let the reporting tools do their job than knowing it stuck in a state I haven't thought before.
onExceptionCaughtcould be nice too, but I agree that the default behavior should be to propagate the exception.I'm not sure why it was design like this...
I think we should provide an interface to let the developer handle exceptions.
But for the default mechanism, what should we do ? If we let the exception go we'll break the API and possibly make some applications crash...As it's a major release with some big changes, we could also let the exception propagate as a default behavior, and just write a big warning on the change log :)
4 remaining items
@bishopmatthew Yes there is a problem with hidden exceptions, and a discussion has started about that here
If #727 implementation doesn't look nice, what about the implementation @DayS proposed 4 months ago?
@Override public void aBackgroundMethod(final AAExceptionHandler exceptionHandler) { BackgroundExecutor.execute(new BackgroundExecutor.Task("", 0, "") { @Override public void execute() { try { BackgroundActivity_.super.addSerializedBackgroundMethod(i); } catch (RuntimeException e) { exceptionHandler.onExceptionCatched(e); } } }); }
Ok so, to make things move, what about this ?
@Background void doSomethingOnBackground() { throw new RuntimeException(""); } @ExceptionHandler void onException(Exception e){ // If an @ExceptionHandler is present, it will receive the exception. // Otherwise the exception is thrown to the system and make the app crash. // Idea for v3.1: possibility to have multiple ```@ExceptionHandler```s with different exception types. When an exception occurs, we try to find the more accurate handler for the exception, otherwise we throw it to the system. }
About @background implementation, instead of having a separate thread to catch exception, what about we create one only when an exception occurs? That means, basically, to do this:
@Override public void aBackgroundMethod(final AAExceptionHandler exceptionHandler) { BackgroundExecutor.execute(new BackgroundExecutor.Task("", 0, "") { @Override public void execute() { try { BackgroundActivity_.super.addSerializedBackgroundMethod(i); } catch (final RuntimeException e) { new Thread(){ public void run(){ throw e; } }.start(); } } }); }
The code would look a little better if we move the ugly code in the
BackgroundExecutorand just foBackgroundExecutor.makeAppCrashOnException(e);.Oh! A lot better, did you know about UncaughtExceptionHandler ?
try { BackgroundActivity_.super.addSerializedBackgroundMethod(i); } catch (final RuntimeException e) { Thread .getDefaultUncaughtExceptionHandler() .uncaughtException(Thread.currentThread(), e); }
That let the user catch exception if he wants to. ACRA is probably based on this method too, @KevinGaudin?
And we forget about@ExceptionHandlerfor now.
What do you think?Seems good to me :)
Go ahead and make a PR. Don't forget to add lots of unit tests.Yes, ACRA is an UncaughtExceptionHandler.
By doing this, you tell the system to behave as if the exception was not handled by your code => Force Close dialog or ACRA then the application is killed.
- added 6 commits that reference this issue
on Oct 17, 2013 I tested @JoanZapata solution on my project and it works very well. That's a brillant and simple idea thx !
- added a commit that references this issue
on Oct 18, 2013
Hello,
The following code shows the generated code of a @UiThread method:
Notice that it wraps the super.logIn() method call within a try/catch that logs the exception.
In our application, we use BugSense, which has a global exception handler reporting unhandled exceptions. The try/batch block above prevents BugSense from seeing the exceptions.
We'd like the ability to remove the try/catch wrapping in the generated code perhaps via a new boolean property available in the @background and @UiThread annotations, or via some other mechanism.
Thanks,