Improve mechanism for extracting the result of a PlainActionFuture

[DaveCTurner]

If a PlainActionFuture is known to be complete then it seems we should be able to extract its result, but in fact today there's not really a good way to do this truly accurately:

• The result() method mangles checked exceptions, but not unchecked ones, so it's impossible to unmangle the result accurately.
• The get() method yields exceptions faithfully but will throw an InterruptedException if the calling thread is in an interrupted state even if the result is available and no blocking occurs.

I think I'd be inclined to change result() to wrap any exception in an ExecutionException always, and fix up the callers to handle this. It's only used in tests today. But maybe there are other ideas too.

[elasticsearchmachine]

Pinging @elastic/es-distributed (Team:Distributed)

[nicktindall]

Just had an initial look at this one. If I understand correctly the goal is

• for result() to either
  • return the result
  • return the exception thrown (checked or otherwise), wrapped in an UncategorizedExecutionException (and also an ExecutionException? not sure why we need the second wrapper?) - we do this because result() doesn't declare any checked exceptions?

If we implement that, what do we expect from actionResult(), currently that will ElasticSearchException#unwrapCause any thrown ElasticSearchExceptions but do nothing to wrapped checked exceptions.

The equivalent behaviour post-change would seem to be to unwrap the UncategorizedExecutionExcepion.cause and re-wrap the result? perhaps we should do the ElasticSearchException#unwrapCause prior to wrapping, then actionResult() would just be equivalent to result()?

[DaveCTurner]

My preference would be to avoid any conditional wrapping or unwrapping so that the caller can easily get hold of the exact exception that completed the listener. We could declare that result() throws Exception and throw the bare exception directly, although that would be somewhat indistinguishable from exceptions arising from a future that had not yet been completed (i.e. pending or cancelled). So maybe we should declare that result() throws ExecutionException and apply an ExecutionException wrapper.

actionResult() is frankly a mess. My preference there would be to get rid of it and have callers do whatever wrapping or unwrapping is needed themselves (frequently, none). It's a royal pain to get a stack trace in some logs or HTTP response that is missing vital context thanks to the unwrapping process.