Is returning Futures.immediateFuture(x) in an API an antipattern?

user_1542667 0
Viewed 733

This doesn't appear to be covered in https://github.com/google/guava/wiki/ListenableFutureExplained

But the very common pattern

@Override
public ListenableFuture<Void> loadResources() {
  fastSyncMethod();

  return Futures.immediateFuture(null);
}

Is there guidance somewhere on this? A lot of the usage of code using ListenableFutures would be affected by assumptions that we can safely make when calling or implementing APIs using ListenableFuture.

guava
3 Answers

We don't know what's in syncMethod, but the name seems to be chosen to imply "some synchronous communication code", in which case the code sample you show is manifestly an antipattern. I suspect it's not covered in the guava docs simply because they never thought anyone would write it. It would be like telling people not to declare their method as returning an object if it actually always returns null, or calling it "doX" if it actually does Y. Signatures promise behaviour, and returning a future promises that you, at least sometimes, return an incomplete future.

If you have a method which sometimes delays work and sometimes doesn't, it make sense to return a completed future on the non-delayed path. Here are some guidelines:

  1. Consider the exception handling of your caller: it may be expecting certain types of failure to result in a failed future, and may not correctly handle your method raising an exception directly.
  2. Don't directly call any code which raises InterruptedException. That's a sure sign you're breaking your caller's expectations about whether you are asynchronous or not.
  3. IOException is also a red flag, as it usually accompanies slow network or disk calls that your caller is likely expecting to happen asynchronously.

Unfortunately there's no general solution for how to convert your synchronous call into an asynchronous one. Ideally you should be using asynchronous libraries under the hood to do your network calls, so that your threads aren't sitting around gathering dust while bytes bounce between servers. If that's not an option, you'll have to spin up a thread pool — just keep an eye on how many threads you're making.

user_1256630

My 2c

Seems like an antipattern to me.

  • It leaves two codepaths for error handling, a required try {} catch {} in calling code, plus exceptions in the ListenableFuture pipeline
  • It makes the execution time of syncMethod() relevant to whether callers can assume the method in Synchronous. For high throughput code, even a log statement writing to disk or using the InetAddress api is a blocking operation.
  • It breaks the somewhat Functional paradigm of Futures e.g. referential transparency.
user_15426670

Your example shows a method being overridden, which is important here.

By having the method return a ListenableFuture, the author of the interface or superclass allows implementations to return asynchronously. Likewise, callers of the method know to tolerate an asynchronous return value.

In return, the implementation promises to be non-blocking. Because it returns a ListenableFuture, it has the opportunity to shift any expensive work off to a background thread, use an asynchronous network call, or whatever.

It doesn't have to run asynchronously, though. If the implementation is non-blocking and will complete quickly, there's absolutely no reason to force in a thread-switch just to satisfy some expectation that the work will complete asynchronously. And in that case, immediateFuture() is a good way to conform to the interface.

So, is your example an antipattern? It depends how expensive syncMethod() is. If it's fast (non-blocking, and doesn't do some lengthy computation) then the example is fine. If it's slow (perhaps it waits on the network, or factorizes a very large number) then yes, it's an antipattern.

user_4228740
Related