8000 Propagate exceptions on @Background/@UIThread annotated methods by tbruyelle · Pull Request #677 · androidannotations/androidannotations · GitHub
[go: up one dir, main page]
Skip to content
This repository was archived by the owner on Feb 26, 2023. It is now read-only.

Propagate exceptions on @Background/@UIThread annotated methods #677

Merged
JoanZapata merged 1 commit into from
Jul 26, 2013
Merged

Propagate exceptions on @Background/@UIThread annotated methods #677

JoanZapata merged 1 commit into from
Jul 26, 2013

Conversation

@tbruyelle
Copy link
Contributor
@tbruyelle tbruyelle commented Jul 26, 2013

Related to #646

Instead of only logging the exceptions thrown in @Background/@UIThread blocks, which make them invisibles to exception handlers, a decision has been made to propagate them.

Requires a big warning in the 3.0 change log 😄

The generated code :

 @Override
    public void emptyBackgroundMethod() {
        BackgroundExecutor.execute(new BackgroundExecutor.Task("", 0, "") {


            @Override
            public void execute() {
                try {
                    ThreadActivity_.super.emptyBackgroundMethod();
                } catch (RuntimeException e) {
                    Log.e("ThreadActivity_", "A runtime exception was thrown while executing code in a background task", e);
                    throw e;
                }
            }

        }
        );
    }

@tbruyelle
Propagate exceptions on @Background/@UIThread annotated methods
c4e8e3d 
The main goal of this fix is to avoid hiding exception to
ExceptionHandler.
JoanZapata added a commit that referenced this pull request Jul 26, 2013
@JoanZapata
Merge pull request #677 from tbruyelle/646_PropagateThreadExceptions
c46b0e7 
Propagate exceptions on @Background/@UIThread annotated methods
@JoanZapata JoanZapata merged commit c46b0e7 into androidannotations:develop Jul 26, 2013
@JoanZapata
Copy link
Contributor
JoanZapata commented Jul 26, 2013

👍

@yDelouis
Copy link
Contributor
yDelouis commented Jul 26, 2013

Why are we catching the exception to throw it just after ? And because we throw an unchecked exception, we don't have to log it since it will be logged by the system if the dev doesn't catch it.
Why do you think ?

@tbruyelle
Copy link
Contributor Author
tbruyelle commented Jul 26, 2013

I asked myself the same question and finally decided to change as little as possible. If people were used to check the log to see exceptions in their threads... But maybe I'm wrong.

@yDelouis
Copy link
Contributor
yDelouis commented Jul 26, 2013

If a dev switches to 3.0 and doesn't change anything, the exception will be logged twice. Once by AA, once by the system since it's an unchecked exception.

@DayS
Copy link
Contributor
DayS commented Jul 27, 2013

I agree. We should fix that

@tbruyelle
Copy link
Contributor Author
tbruyelle commented Jul 27, 2013

Ok I will remove the try/catch then.

@tbruyelle tbruyelle mentioned this pull request Jul 27, 2013
Merged
@tbruyelle
Copy link
Contributor Author
tbruyelle commented Jul 31, 2013

@DayS @yDelouis With the new BackgroundExecutor in AA 3.0, exceptions in a runnable are not caught by the system. This is due to the usage of ExecutorService.submit() which has a kind of exception handler in it.

So since I removed the try/catch and the log, exceptions in a runnable are completely invisible...

@DayS
Copy link
Contributor
DayS commented Jul 31, 2013

I tested this feature on a Sandbox project and it worked fine. That's strange. I'm gonna test it again, so.

@tbruyelle
Copy link
Contributor Author
tbruyelle commented Aug 2, 2013

Did you manage to reproduce the problem ? Me, yes.

When you use the ExecutorService.submit() method, exceptions throwed in the runnable will be part of the result, so not propagated, but retrievable via Future.get().

I found 2 ways to fix the issue, but both have drawbacks.

Solution 1 : never use Executor.submit(), only Executor.execute(). Drawback is the tasks won't be cancelable any more, and so BackgoundExecutor.cancelAll() will have no effect.

Solution 2 8000 : override ThreadPoolExecutor.afterExecute() to check if an exception has been thrown, if yes then propagate it. The drawback is if the user set his own Executor via BackgoundExecutor.setExecutor(), we loose the override (overriding is the only way because afterExecute is protected, so we can't use a wrapper around ThreadPoolExecutor). Source

@DayS
Copy link
Contributor
DayS commented Aug 2, 2013

I did manage to reproduce this problem. And I had the solution 1 in my mind... But it's not the best solution...

@tbruyelle tbruyelle mentioned this pull request Aug 9, 2013
Closed
@tbruyelle
Copy link
Contributor Author
tbruyelle commented Aug 9, 2013

Ok I found a solution with I think no major drawbacks.

The tip is to call Future.get() in an another thread, just after the submit/schedule, to check if an exception occurred. If yes, then we propagate it so it can be caught by the system or by an exception handler if set. I tested it on my application and it seems to work pretty well.

For that I created another thread pool in BackgroundExecutor dedicated to the exception watching.

Please check that commit tbruyelle/androidannotations@86d89d27 and if the tip is acceptable for you, then I will make a PR.

@tbruyelle tbruyelle mentioned this pull request Sep 17, 2013
Closed
@DayS DayS mentioned this pull request Oct 8, 2013
Closed
@marmor7 marmor7 mentioned this pull request Aug 29, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tbruyelle @JoanZapata @yDelouis @DayS
0