2

Our application has some code that runs asynchronously that is failing. Like this:

CompletableFuture.runAsync(
    () -> { throw new RuntimeException("bad"); },
    executorService
);

We want default exception handling code that can catch these errors, in case specific uses forget to handle exceptions (this came from a production bug).

This is apparently tricky. The answer given in Handling exceptions from Java ExecutorService tasks does not work.

It relies on the task being a Future<?> and then calling get() on it, resulting in the exception being thrown again. But this is not the case for runAsync code.

runAsync creates a java.util.concurrent.CompletableFuture.AsyncRun class that seems to try to supress all exceptions. Despite being a Future itself, it does not indicate being isDone(), and seems to provide no way to get exceptions out of it.

So, given the following boilerplate, how should we catch these gnarly exceptions?

Note that we really want something that will catch all unhandled exceptions in runAsync code, not something we can add to each runAsync invocation. It's just too easy to forget to add handling code to each one.

public class ExceptionTest {
    public static void main(String[] args) throws RuntimeException {
        ExecutorService executorService = new ThreadPoolExecutor(
            1, 1, 0L,
            TimeUnit.MILLISECONDS,
            new LinkedBlockingQueue()
        ) {
            protected void afterExecute(Runnable r, Throwable t) {
                super.afterExecute(r, t);

                // TODO: Magically extract the exception from `r`
            }
        };

        CompletableFuture.runAsync(
            () -> { throw new RuntimeException("bad"); },
            executorService
        );
    }
}
Yona Appletree
  • 8,801
  • 6
  • 35
  • 52
  • have you tried `CompletableFuture.runAsync(...).handle((o, throwable) -> { //do something with the error here});`? – Kartik Nov 09 '18 at 01:06
  • That's what I specifically _don't_ want to do. It's too easy to forget that kind of thing. I want something generic that will handle all such errors in case we forget to do that (which we did, and it caused a very hard-to-debug production error). – Yona Appletree Nov 09 '18 at 01:08
  • But that's how `CompletionStage`, and by extension `CompletableFuture`, was designed to be used. If you want to handle exceptions you should use `handle` or `exceptionally` or maybe even `whenComplete`. – Slaw Nov 09 '18 at 01:09
  • What's annoying is that `CompletableFuture.AsyncRun` seems designed to make this sort of thing impossible, even with nasty hacking. It specifically _doesn't_ implement the contract of `Future` (which it implements) and then goes out of its way to nullify its references to the actual `Future`. What the hell. – Yona Appletree Nov 09 '18 at 01:10
  • It was specifically designed to be impossible to have global exception handlers for? Things like `Thread.setDefaultUncaughtExceptionHandler` seem to indicate that the java designers at least have respect for the idea of global, generic error handling. – Yona Appletree Nov 09 '18 at 01:11
  • The problem is when `supplyAsync`, or any other stage, throws an exception it _is not uncaught_. The `CompletableFuture`, and things like `FutureTask`, _catch_ the exception and only report it to the caller when one tries to get the result. `ThreadPoolExecutor` provides a nice way to implement "global" error handling, but that is not inherent in the `Future` or `CompletionStage` interfaces. – Slaw Nov 09 '18 at 01:14
  • Well, it's uncaught by _application code_. And the way `AsyncRun` is implemented, it _cannot_ be caught by the `ExecutorService`. This seems like a bug in the SDK, to me. – Yona Appletree Nov 09 '18 at 01:15
  • I agree what you want would be very useful and should be possible, but it also isn't required by any of the involved interfaces (as far as I understand them). Looking at the implementation I think you're right and this appears impossible. Maybe submit a feature request (if one doesn't exist)? – Slaw Nov 09 '18 at 01:30
  • Indeed. I'll take a look. In the meantime, I posted a hacky answer that handles at least my particular case of this. Thanks for the help! – Yona Appletree Nov 09 '18 at 01:46
  • Instead of using `CompletableFuture.runAsync` directly, have you considered refactoring to a custom class? The 'too easy to forget' problem can be solved by having just one place in your codebase do that 'too easy to forget' thing and everything else calls it. Then use code lint or something to scrub for stray calls to `CompletableFuture`. – Travis Well Nov 14 '18 at 21:11
  • @TravisWellman Perfectly fine suggestion, but it's really the same problem. It's just as easy to forget to call MyAsync.runAsync() as it is to forget to handle the exceptions. – Yona Appletree Nov 14 '18 at 21:20
  • @YonaAppletree not really. It's a lot easier to eliminate all calls to `CompletableFuture.runAsync` than to ensure they're correct. Your CI server could do it. – Travis Well Nov 15 '18 at 21:10

1 Answers1

1

So, this is a terrible hack, but it does handle the case where you forget to call exceptionally when using runAsync. I'd love to see more generic and less hacky solutions.

It works by intercepting the AsyncRun before it's executed and patching on an exceptionally block.

Seriously janky, though. But it'll work, maybe, until Oracle changes how runAsync works.

    ExecutorService executorService = new ThreadPoolExecutor(
        1,
        1,
        0L,
        TimeUnit.MILLISECONDS,
        new LinkedBlockingQueue()
    ) {
        @Override
        protected void beforeExecute(final Thread t, final Runnable r) {
            super.beforeExecute(t, r);

            if (r.getClass().getName().equals("java.util.concurrent.CompletableFuture$AsyncRun")) {
                try {
                    final Field f = r.getClass().getDeclaredField("dep");
                    f.setAccessible(true);
                    ((CompletableFuture<?>) f.get(r)).exceptionally(e -> {
                        LoggerFactory.getLogger(ExceptionTest.class).error("Error in runAsync " + r, e);
                        UnsafeUtils.getUnsafe().throwException(e);
                        return null;
                    });
                } catch (Exception e) {
                    System.out.println("Failed to hack CompletableFuture$AsyncRun to report exceptions.");
                }
            }
        }

        protected void afterExecute(Runnable r, Throwable t) {
            super.afterExecute(r, t);

            if (t == null && r instanceof Future<?>) {
                try {
                    Future<?> future = (Future<?>) r;
                    if (future.isDone()) {
                        future.get();
                    }
                } catch (CancellationException ce) {
                    t = ce;
                } catch (ExecutionException ee) {
                    t = ee.getCause();
                } catch (InterruptedException ie) {
                    Thread.currentThread().interrupt();
                }
            }
            if (t != null) {
                LoggerFactory.getLogger(ExceptionTest.class).error("Error in async task " + r, t);
            }
        }
    };
Yona Appletree
  • 8,801
  • 6
  • 35
  • 52