continuation with bad Executor leads to un-predictable result and the Exception is hidden

[yunarta]

I'm using bolts heavily with Executor to synchronizes the continuation blocks.
When i made a mistake in the Executor implementation, causes a NullPointerException to be raised, i found that this would crash the app due to the exception was raised to the previous Task.

The snippets kind of like below

Task.callInBackground(T1).onSuccess(new Continuation() {
    public Object then(Task task) { }
}, BadExecutors).continueWith(new Continuation() {
    public Object then(Task task) { 
        if (task.isFaulted()) task.getError().printStackTrace()
    }
});

Since the BadExecutors throws NPE, rather then having the NPE being sent to to next continuation block, it crashes the app with IllegalStateException("Cannot set the error on a completed task.")

This is caused by the the Executor.execute is not wrapped by try-catch, causing the exception being sent back to finishing block of the previous task in the chain where the Task result being set as completed, continuations blocks is being executed

[facebook-github-bot]

By analyzing the blame information on this pull request, we identified @bklimt, @grantland and @belemaire to be potential reviewers.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[grantland]

I'm not sure we'd want to pass a runtime exception from the executor to the next task in the list and a failure of that magnitude should cause the application to crash as you've seen.

Is there any reason you'd want to be able to recover from a exception such as this where there was an error in your executor implementation?

[yunarta]

I agree that in most cases, failure on this scale should crash the app. The main problem was that Bolts hides the exception cause and on bigger project, it would takes a lot of effort to found the origin of the problem.

Current implementation will only crashed the app with IllegalStateException("Cannot set the error on a completed task.") with stacktrace pointing to Bolts source, rather than the app source.

And since most of my Bolts continuation ended with checking and logging for any fault found, that is the reason i put the exception into continuation.

Anyway, I would propose that the exception hiding bug should be handle in different way, while decision for sending the exception to Task continuation or should be crashing the app, can be decided by your team.

As my own preference, and usage pattern, i do want to recover from executor fault. In my project, I'm using android Handler for Executor implementation and i would like to have Task to fail gracefully without crashing when i shutdown the Handler looper :)

[grantland]

Hrm actually I stand corrected, a bad TaskScheduler will actually propagate the exception to the next continuation: https://dotnetfiddle.net/BlwLqU

Mind squashing your commits so I can merge?

Thanks and sorry about that!

[grantland]

Actually, we'd probably want this safety on Task.call(callable, executor) as well.

[facebook-github-bot]

@yunarta updated the pull request.

[facebook-github-bot]

@yunarta updated the pull request.

[yunarta]

I made fixes to Task.call(callable, executor) as well

[grantland]

Could you also add tests for these as well? I can see we'd need ones for the following:

• Task.call
• continueWith
• continueWithTask

[facebook-github-bot]

@yunarta updated the pull request.

[yunarta]

Hi Grant

I added test for the requested three items. And i decided to remove ExecutorException, so developer can see the exception caused by their Bad Executor

[facebook-github-bot]

@yunarta updated the pull request.

[facebook-github-bot]

@yunarta updated the pull request.

[grantland]

Thanks for adding tests!

I actually think think that we should keep ExecutorException so the developer can distinguish problems in the executor vs failed tasks. In addition, the developer can still get the original exception via Exception#getCause() as well as see it in the stack trace.

[yunarta]

I removed ExecutorException because when I'm using Bolts task chaining, I would throw in middle and catch it as i know what Exception being thrown back then.

With ExecutorException, when someone purposely throw a specific exception subclass in Executor execute function, they would be perplexed when they found they get ExecutorException instead for the first time, and probably will contemplate why Bolts need to wrap the know exception in the first place.

[grantland]

That works in the case where you own both the Executor and the continuations, but I don't think it does if you don't.

For example, if you were using a prebuilt Executor that threw a generic exception such as NullPointerException, there will be no way to determine whether or not the NPE was from your previous Task or from the Executor.

What do you think of this case?

[yunarta]

Let me test more cases

[yunarta]

I think you are right, it would be much better to emphasize that this error originating from Executor.
I will revise the pull request. And i've updated the test as well

[facebook-github-bot]

@yunarta updated the pull request.

[grantland]

Could you remove this? We don't record authorship on class JavaDocs, but through git blame/commit history.

[facebook-github-bot]

@yunarta updated the pull request.

[grantland]

Thanks for you're awesome contribution! As a thank you, we'd like to send you some Parse swag. If you're interested, send me your mailing address and t-shirt size to my email address on my profile page.

[yunarta]

Nice to work with you as well Grant 👍
I'll send my mailing address to your mail soon