3
\$\begingroup\$

Edit: cross-posted this question on Stackoverflow.

Suppose I have a function that takes a FileInputStream, performs some operation using the file data in the background, and returns a CompletableFuture<Void that will be completed when the background operation finishes:

CompletableFuture<Void> asyncFileOperation(FileInputStream in) {
  var cf = new CompletableFuture<Void>();
  // ...
  return cf;
}

FileInputStream is a resource that has to be closed when not needed anymore. With synchronous code, we would use finally or try-with-resources. But with CompletableFuture, it's a bit more complicated. I can think of four ways, with pros and cons:

  1. Simply using whenComplete:
FileInputStream in = openFile("data.txt");
asyncFileOperation(in)
  .whenComplete((r, x) -> in.close());

This effectively closes the stream when asyncFileOperation completes, regardless whether it succeeds or fails. Except, when asyncFileOperation throws a synchronous RuntimeException before even returning the CompletableFuture, whenComplete will not be called, so neither will close.

We could write this off as "never happens", but unexpected exceptions are the very reason we use finally and try-with-resources in the synchronous case, so we need to expect the unexpected.

  1. Calling asyncFileOperation inside thenCompose:
CompletableFuture.completedFuture(openFile("data.txt");)
  .thenCompose(this::asyncFileOperation)
  .whenComplete((r, x) -> in.close());

This solves the problem of solution 1 by executing asyncFileOperation in the context of an existing CompletableFuture, so any exception gets caught and whenComplete is always executed. The problem with this is that we swallow the exception, which is particularly bad for unexpected ones such as NullPointerException.

  1. Using both try-catch and whenComplete:
FileInputStream in = openFile("data.txt");
try {
  asyncFileOperation(in)
    .whenComplete((r, x) -> in.close());
} catch (RuntimeException exc) {
  in.close();
  throw exc;
}

This solves all of the above problems, and doesn't really introduce new ones, except that it's ugly and repetitive.

  1. Doing it inside asyncFileOperation:
CompletableFuture<Void> asyncFileOperation(FileInputStream in) {
  var cf = new CompletableFuture<Void>();
  // ...
  return cf.whenComplete((r, x) -> in.close());
}

asyncFileOperation(openFile("data.txt"));

Here, asyncFileOperation closes the stream itself ones it completes. Depending on the implementation, it might be obvious that there can be no RuntimeException, so we could eliminate some cases. If not, we need to use one of the approaches above to handle this, but we can handle it once and for all rather for every call. Still, I don't like this because maybe we want to continue to use the stream for some other operations, and its always nice when resources are created and cleaned up in the same place.

So what are your thoughts on these options? Do you have other ones?

\$\endgroup\$
3
  • \$\begingroup\$ Hello at CodeReview@SE - SE's site for open-ended feedback on working source code from your project, including application of best practices (along What topics can I ask about here?). I think your question off topic: Check stackoverflow. \$\endgroup\$
    – greybeard
    Aug 10, 2021 at 6:40
  • \$\begingroup\$ Yeah, I wasn't sure whether to post in on Stackoverflow, Software Engineering, or here, so thanks! Should I just copy it into Stackoverflow? \$\endgroup\$
    – mperktold
    Aug 10, 2021 at 7:26
  • \$\begingroup\$ (Please consult the help pages - far as I remember, the canonical procedure is to ask for a moderator to move the post. Whenever you feel justified to cross-post, by all means cross-link the posts, too.) \$\endgroup\$
    – greybeard
    Aug 10, 2021 at 7:31

1 Answer 1

1
\$\begingroup\$

I can think of a different approach in which it would be the asynchronous' call responsibility to create the resource and close it:

If you want to let the responsibility of knowing how to create the resource to the caller, you could use a Provider:

CompletableFuture<Void> asyncFileOperation(Provider<FileInputStream> inProvider) {
  // This try-with-resources would actually be used in the background task, but you get the idea
  try (var in = inProvider.get()) {
    var cf = new CompletableFuture<Void>();
    // ...
    return cf;
  }
}

asyncFileOperation(() -> openFile("data.txt"))

If you want to leave the responsibility of knowing how to create the resource to asyncFileOperation, then just pass a filepath to the method.

The trade off with this solution is you'd have to create the resource again if you want to use it somewhere else.

\$\endgroup\$
5
  • \$\begingroup\$ It's certainly a nice idea I didn't think of, thanks! It gives me another, somewhat opposite idea: We could pass the async operation to the method that opens the file, like useFileAsync("data.txt", this::asyncFileOperation), so it would have the signature useFileAsync(String, Function<FileInputStream, CompletableFuture<Void>). \$\endgroup\$
    – mperktold
    Aug 9, 2021 at 13:57
  • \$\begingroup\$ Still, I would be interested in a more general solution, not only for files, but any kind of cleanup to be executed when a CompletableFuture completes, and I don't think this would work in the general case. But maybe even option 1 with just whenComplete already works fine and I'm just being paranoid ;) \$\endgroup\$
    – mperktold
    Aug 9, 2021 at 14:03
  • \$\begingroup\$ To make it more generic, you could use make a class that takes a Provider<T extends Autoclosable> and applies it to a Consumer<Provider<T extends Autoclosable>> \$\endgroup\$ Aug 9, 2021 at 17:41
  • \$\begingroup\$ I wasn't talking about the type, mroe about the approach. I'm not sure it's always possible to open the resource only in the background task. For example, you might have an existing DB connection that you jused used for synchronous work and you want to reuse it for the background task, but you need to close it afterwards. This is a pattern that appears quite often in our codebase. But maybe it's an anti-pattern and this should also follow your approach? \$\endgroup\$
    – mperktold
    Aug 10, 2021 at 5:50
  • 1
    \$\begingroup\$ Reading this Baelding article and this answer, you should probably let each thread/process request their own database connection using a connection pool. Still, if we are talking about database connections, you probably have that logic centralised, so maybe the is no need to use any kind of provider, and just let the corresponding thread request a connection \$\endgroup\$ Aug 10, 2021 at 12:57

Not the answer you're looking for? Browse other questions tagged or ask your own question.