Add completedFutureFrom method

[eshrubs]

This method is to be used to create completed futures but in a way that is safe from leaking exceptions.

Today, one can do something like this,

CompletionStage<String> getValue() {
  return completedFuture(computeValue());
}

but if computeValue() would throw then this exception leaks synchronously and will break.

To handle this, one can do

CompletionStage<String> getValue() {
  try {
    return completedFuture(computeValue());
  } catch (Exception e) {
    return exceptionallyCompletedFuture(e);
  }
}

but it is hard and burdemsome to always remember to handle that exception.

This patch proposes a new alternative, but using a checked supplier;

CompletionStage<String> getValue() {
  // if an exception thrown then stage is exceptionally completed
  return completedFutureFrom(this::computeValue);
}

[codecov-io]

Codecov Report

Merging #55 into master will not change coverage. The diff coverage is 100%.

@@           Coverage Diff           @@
##             master    #55   +/-   ##
=======================================
  Coverage       100%   100%           
- Complexity       63     64    +1     
=======================================
  Files             5      5           
  Lines           162    165    +3     
  Branches          9      9           
=======================================
+ Hits            162    165    +3

Impacted Files	Coverage Δ	Complexity Δ
...n/java/com/spotify/futures/CompletableFutures.java	100% <100%> (ø)	38 <1> (+1)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d617a83...b87fe1d. Read the comment docs.

[udoprog]

Hey,

In a typical Java application there are too many sources of runtime exceptions. Catching and handling them in middleware is absolutely necessary.

My concerns with this PR are:

It might give a false sense of security and discourage people from making use of required middleware. Encouraging bad design where errors are not logged, causing the engineer to have to realize this is what's happening.

At the point where you catch exceptions you usually do something a bit more special than just wrapping it. The equivalent of re-throwing with more details. Otherwise you might as well permit falling back to your error-catching middleware.

Thanks,

[pettermahlen]

@udoprog isn't this PR about making it easier to solve the problem you're referring to? I'm not sure I understand the reasoning behind your objection. I feel like this feature may make it more likely that people handle immediate exceptions, but it sounds like you feel it will make it less likely that people do that?

[udoprog]

@pettermahlen I'll try to elaborate.

The first objection was in short; it might discourage people from making use of required middleware.

To explain I'll ask the question: when would I reach for this method?

The scenario I can think of would be for ad-hoc translating thrown exceptions into completable futures. What @eshrubs characterizes as handling "leaking exceptions". To me this is an indication that the error handling middleware* of your application does not handle thrown exceptions.

but it is hard and burdemsome to always remember to handle that exception.

If the middleware was in place there would be little need to handle leaking exceptions. What is then the recommended way to use this method? My conclusion was it it would primarily be used to patch a lack of proper error handling in the root of your application.

This is when my second object comes into play. Assuming there is middleware in place, this method becomes the equivalent of:

try {
    computeValue();
} catch(final Exception e) {
    throw new RuntimeException(e);
}

While the above does propagate a checked exception, it seems it will primarily be used to re-throw an existing exception without adding any more information (checked exception are going out of style with functional Java). While harmless, it doesn't do anything useful.

What do you think?

*: Error handling middleware is the piece of code at the root of your application or service handler that translates and/or logs any exceptions it detects. For an HTTP service this would cause different kinds of HTTP status codes, 500 being the most notable.

[spkrka]

I think it is a useful feature to easily transform a thrown exception to an exceptional future.

[pettermahlen]

what @spkrka said - this helps you move the error handling from when you invoke an async method to when it returns/helps you avoid duplicating that error handling in both the immediate case (when the method is invoked) and when handling the future result. I don't think it's the same as converting checked exceptions to runtime exceptions.

[udoprog]

this helps you move the error handling from when you invoke an async method to when it returns/helps you avoid duplicating that error handling in both the immediate case (when the method is invoked) and when handling the future result.

My central point here is that it doesn't. You always have to catch and handle thrown exceptions anyways. Preferably in a single, abstracted place. Which reduces the usefulness of this API. No?

I don't think it's the same as converting checked exceptions to runtime exceptions.

I was not trying to make that comparison. So we agree there.

@spkrka You gave a simple answer. But I read from it that you don't share my concern about promoting a bad practice?

[pettermahlen]

So, I have previously come to the same conclusion as you: that to handle exceptions perfectly in situations like this, you need to both catch and handle exceptions thrown on async method invocation and on async method result availability. That tends to boil down to duplicating code (so doing the same kinds of checks on both these two code paths), which means I don't think that there is a 'single, abstracted place' to do these checks. I'm sort of wondering now if I kind of think that you can improve the error handling/reduce duplication by moving as many exceptions as possible to the async method result availability path, which is what this PR does. That's not going to be 100% correct, but maybe it can allow you to simplify your application-level exception handling and leave very exceptional cases to be handled in a less slick (from the caller's perspective) way. Saying this is completely useless might be letting perfect become the enemy of good, in a way. I'm not super-convinced that that's correct, but that's where I'm coming from right now.

[spkrka]

I think that by promoting all exceptions from a specific code path to be part of the future can simplify and unify error handling.

I can agree that this is probably not correct in all situations, but I also think that there exists situations where it makes perfect sense.

I would prefer to be careful about trying to enforce strict coding conventions without having full knowledge of the use case.

[udoprog]

I can agree that this is probably not correct in all situations, but I also think that there exists situations where it makes perfect sense.

Maybe. I would be convinced by this addition if these cases would be described or exemplified. In API design it's easy to add things, but hard to remove them.

I'm happy that you've echoed some of my concerns. I'll leave it to you to decide.

[dflemstr]

Personal opinion, but I think for run-time exceptions we already have CompletableFuture.supplyAsync(...) and for checked exceptions they should translate into something that's part of the application domain instead of generally transform into futures that are exceptionally completed.

[eshrubs]

@dflemstr I wish that supplyAsync did what this did, and I like it for some cases - especially when the operation could be expensive. However I prefer this for 2 reasons; first, exceptions thrown in supplyAsync will end up wrapped with ExecutionException which is inconvenient to deal with later, and second is the worry of contention on the FJP when posting that task.

[eshrubs]

@udoprog The motivation for this here is to avoid the side-effects of creating a future. It encapsulates the whole state into the stage so it can be consistently handled (with exceptionally or whenComplete)

[dflemstr]

Just to be clear I use the overload of supplyAsync which takes an executor, or sameThreadExecutor if for some reason I want to run an expensive operation on the same thread.

I think ExecutionException is generally useful because it differentiates "there was a runtime exception while running your supplied lambda/Callable" vs "there was a runtime exception while scheduling your runnable on an executor"

[dflemstr]

If we are talking about wrapping synchronous code in a future, I would simply not use futures in that case.

[caesar-ralf]

closing 6 years old PR. Feel free to re-open if you feel it's something that still could be useful