Try/catch on generated override of @Background and @UiThread hiding exception from global exception handler.

@background

Hello,

The following code shows the generated code of a @UiThread method:

   @Override
    public void logIn() {
        handler_.post(new Runnable() {
            @Override
            public void run() {
                try {
                    LogInFragment_.super.logIn();
                } catch (RuntimeException e) {
                    Log.e("LogInFragment_", "A runtime exception was thrown while executing code in a runnable", e);
                }
            }

        }
        );
    }

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,

Quenio

@background

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 AAExceptionHandler interface
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 @Background annotated 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.

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 @Background methods, 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 exceptionHandler parameter and replace this.onExceptionCaughtby BackgroundActivity_.this.onExceptionCaught which is the correct way to call this method

For 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/@UIThread annotated 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 ?

@tbruyelle

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.

onExceptionCaught could be nice too, but I agree that the default behavior should be to propagate the exception.

@DayS

@DayS What do you think about simply propagate the exception ? Exception handler should be provided at a higher level in another feature, and not only for exceptions in threads.

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 :)

@DayS

@DayS +1 for big warning 😄

Personally, I prefer a crash than a silent error with just a log.

I have currently already coded that behavior in my fork, but on 2.7.1 version. I can do it on 3.0 and make a PR.

@bishopmatthew

@bishopmatthew Yes there is a problem with hidden exceptions, and a discussion has started about that here

@DayS

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 BackgroundExecutor and just fo BackgroundExecutor.makeAppCrashOnException(e);.

@KevinGaudin

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 @ExceptionHandler for now.
What do you think?

Seems good to me :)
Go ahead and make a PR. Don't forget to add lots of unit tests.

Sounds fine, you can start by revert that commit 4cb345a to set again the try{} catch{} in the relevant methods.

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.

Fixed androidannotations#646 sending background exception to the glob…

@JoanZapata

I tested @JoanZapata solution on my project and it works very well. That's a brillant and simple idea thx !

Merge pull request #763 from JoanZapata/develop

Navigation Menu

Repository navigation

Try/catch on generated override of @Background and @UiThread hiding exception from global exception handler. #646

4 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Try/catch on generated override of @Background and @UiThread hiding exception from global exception handler. #646

Description

Activity

quenio commented on Jun 26, 2013

quenio commented on Jun 26, 2013

DayS commented on Jun 27, 2013

RobBlairInq commented on Jun 28, 2013

DayS commented on Jun 28, 2013

RobBlairInq commented on Jun 28, 2013

DayS commented on Jun 28, 2013

tbruyelle commented on Jul 16, 2013

JoanZapata commented on Jul 16, 2013

tbruyelle commented on Jul 25, 2013

DayS commented on Jul 25, 2013

DayS commented on Jul 25, 2013

tbruyelle commented on Jul 25, 2013

4 remaining items

tbruyelle commented on Aug 9, 2013

JoanZapata commented on Oct 17, 2013

JoanZapata commented on Oct 17, 2013

DayS commented on Oct 17, 2013

tbruyelle commented on Oct 17, 2013

KevinGaudin commented on Oct 17, 2013

tbruyelle commented on Oct 17, 2013

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions