Add ExecutionList.setUncaughtExceptionHandler

[gissuebot]

Original issue created by simon.m.stewart on 2013-03-14 at 04:26 PM

---

There is currently no mechanism in place to handle exceptions that are thrown in FutureCallbacks. This patch provides a "setUncaughtExceptionHandler" to ExecutionList, allowing these exceptions to be caught and handled cleanly.

https://codereview.appspot.com/6458061/

[gissuebot]

Original comment posted by kurt.kluever on 2013-03-14 at 10:57 PM

---

"a better model may be having an Executor wrapper that handles uncaught exceptions rather than putting it into ExecutionList directly."

"+1 to having the Executor handle this issue."

---

Owner: [email protected]
CC: [email protected], [email protected]

[gissuebot]

Original comment posted by [email protected] on 2013-03-15 at 07:22 PM

---

What's your use-case? Are you using ExecutionList directly? Are you calling it via ListenableFutureTask?

[gissuebot]

Original comment posted by bolinfest on 2013-03-18 at 11:36 PM

---

On Android, I believe that the original motivation was a FutureCallback that was throwing an exception in its onFailure() method. Normally, we would expect this sort of thing to crash the app so we could discover and fix it. Instead, the exception was logged and the app was just left in a bad state. We introduced this diff so that we would invoke ExecutionList.setUncaughtExceptionHandler() at startup so that it was passed a handler that was guaranteed to crash the app.

(I assumed this particular FutureCallback could only receive a specific type of Exception in our code, so I categorically did the cast without doing an instanceof check. I didn't account for the fact onFailure() could also get a CancellationException, so I got a ClassCastException at runtime in onFailure().)

I agree that making this a property of the Executor may be the cleaner solution, though (1) Executor is an interface that you/we don't control, and (2) it may require us to specify the UncaughtExceptionHandler in every place that we create an Executor. I'm sure we could rearchitect around that, though.

[gissuebot]

Original comment posted by bolinfest on 2013-03-19 at 07:48 PM

---

Though thinking about this more: what is the justification for ExecutionList swallowing this exception in the first place?

[gissuebot]

Original comment posted by bolinfest on 2013-03-19 at 07:49 PM

---

I suppose the comment in the code answers that question, but I'm not completely sure that I buy the answer.

[gissuebot]

Original comment posted by [email protected] on 2013-03-19 at 08:13 PM

---

Arguably our error was in making sameThreadExecutor() propagate exceptions. That interpretation suggests a workaround: Supply another sameThreadExecutor() that accepts an UncaughtExceptionHandler. (Our sameThreadExecutor() is more complex than it probably needs to be, mostly thanks to its support for the ExecutorService interface, including shutdown, so you could whip one up yourself.)