AbstractListeningExecutorServices/ListenableFutureTask swallows Errors in listeners

[GoogleCodeExporter]

Hi,

I'm using MoreExecutors.listeningDecorator to decorate a plain ExecutorService, 
where the threads created have a UncaughtExceptionHandler attached. I noticed 
that any Errors thrown in a listener will silently disappear, at least on 
Oracle JDK 6. On OpenJDK6, it goes to my UncaughtExceptionHandler.

My backtrace:

Any jobs submitted to this decorated submit method are handled in 
AbstractListeningExecutorService.submit, which creates a ListenableFutureTask 
(LFT), and then executes that on the underlying ES.

When the LFT is ready, the FutureTask calls done() and this triggers the 
execution of all listeners. Any exceptions in the listeners are handled like 
this:
1. If it is a RuntimeException, log it through JUL logger.
2. Else, (i.e. an Error), propagate it.

Now, this is where the odd parts are. I've expected to get this propagated all 
the way up to my UncaughtExceptionHandler on my thread. Why did I expect this? 
Well, using OpenJDK, this is what happens. The FutureTask implementation there 
does NOT catch any errors.
However, in the Oracle JDK version of FutureTask, any exceptions in done() are 
catched, and setException() is called on the FutureTask (which swallows this 
since the FutureTask has already been run).

Anyhow, the end result is that RuntimeExceptions are logged as intended, but on 
Oracle JDK, any Errors which occurs in a callback will go unnoticed.

OpenJDK FutureTask: 
http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/7-b147/ja
va/util/concurrent/FutureTask.java
Oracle-JDK FutureTask: 
http://javasourcecode.org/html/open-source/jdk/jdk-6u23/java/util/concurrent/Fut
ureTask.java.html

FutureTask test code: https://gist.github.com/stromnet/5568329

Not sure what the proper fix here is, but I thought I'd ticket this if anyone 
else have the same issues.
Of course, if the executor catches (i.e. not using sameThreadExecutor but 
something smarter), the problem is out of the way. But still..

Original issue reported on code.google.com by johan%he...@gtempaccount.com on 13 May 2013 at 1:36

[GoogleCodeExporter]

Interesting. I see that the culprit is innerRun(). (Digression: This bug 
reminds me a lot of one that we had in Guava: 
<http://code.google.com/p/guava-libraries/source/detail?r=19d312977252b2f494f699
7d5dd881f349af6292&path=/guava/src/com/google/common/util/concurrent/Futures.jav
a>).

Here's the bug report to the JDK: 
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6415572

And here's the fix: 
http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/main/java/util/concurren
t/FutureTask.java?view=log#rev1.55

I'm not sure what the proper fix here is, either. We *could* just say that 
we're not too worried about working around JDK bugs, or we could import the 
fixed FutureTask from jsr166.

Original comment by cpov...@google.com on 13 May 2013 at 1:55

[GoogleCodeExporter]

(Whatever we end up doing, I thank you for the report. This is bound to catch 
someone else.)

Original comment by cpov...@google.com on 13 May 2013 at 1:56

[GoogleCodeExporter]

You're welcome! Thanks for Guava.. :)

Ah, fixed in Java 7. We have not yet upgraded to 7 though. Luckily that Error 
was thrown due to a bad deploy on my dev setup, so nothing which we normally 
have problems with.

Also, could this be related (or have a related fix)? 
https://code.google.com/p/guava-libraries/issues/detail?id=1336

Anyhow, looking forward to a fix, whatever that turns out to be!

Original comment by johan%he...@gtempaccount.com on 13 May 2013 at 2:06

[GoogleCodeExporter]

Yes, Issue 1336's static ExecutionList.setUncaughtExceptionHandler is one 
possible workaround. I have the usual reservations about static state, though, 
so don't hold your breath on that one. It deserves some attention, but we're 
already got some changes to Future exception handling going into 15.0, so I'm 
especially nervous about making further changes.

http://code.google.com/p/guava-libraries/source/detail?r=0aa888b199bc12eae29633c
71a424bd28750961b

Original comment by cpov...@google.com on 13 May 2013 at 5:30

[GoogleCodeExporter]

A static approach would not be very practical, no. In our case we run the apps 
in an OSGi environment, which means static configuration would not work at all 
(since multiple applications share the same guava-bundle).

Original comment by johan%he...@gtempaccount.com on 14 May 2013 at 6:28

[GoogleCodeExporter]

This issue has been migrated to GitHub.

It can be found at https://github.com/google/guava/issues/<issue id>

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:12

• Added labels: MigratedToGitHub

[GoogleCodeExporter]

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:17

• Changed state: Migrated

[GoogleCodeExporter]

Original comment by cgdecker@google.com on 3 Nov 2014 at 9:08

• Added labels: Restrict-AddIssueComment-Commit