Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add documentation on Futures APIs and/or release "ClosingFuture" API for resources that must be released/closed #3975

Closed
carterkozak opened this issue Jul 27, 2020 · 5 comments
Assignees
Milestone

Comments

@carterkozak
Copy link
Contributor

Consider a managed resource where a successful SettableFuture.set transfers ownership.

SettableFuture<ManagedResource> settableFuture = SettableFuture.create();
ListenableFuture<String> transformed = Futures.transform(
  settableFuture,
  managedResource -> {
    try {
      return managedResource.getStringValue();
    } finally {
      managedResource.release();
    }
  },
  MoreExecutors.directExecutor());
getManagedResource(settableFuture);
return transformed;

Where getManagedResource uses the return values from SettableFuture.set to release the resource:

// perhaps on a queue, processed by another thread:
SettableFuture<ManagedResource> settableFuture = blockingQueue.take();
ManagedResource resource = checkOutResource();
if (!settableFuture.set(resource) {
  resource.release();
}

Unfortunately, if the transformed future is canceled while the transforming function is invoked, the managed resource is leaked.
It's unclear if this is something that can be resolved using existing APIs.

The issue is that the transformer may take arbitrarily long, and it's not clear whether the transforming function should bias toward allowing immediate cancellation to allow other listeners to complete quickly, or allow transformations to complete while the transformed future returns false from boolean cancel(boolean mayInterruptIfRunning).

The documentation mentions some details about cancellation propagation, but might benefit by describing the edge cases.

* <p>The returned {@code Future} attempts to keep its cancellation state in sync with that of the
* input future. That is, if the returned {@code Future} is cancelled, it will attempt to cancel
* the input, and if the input is cancelled, the returned {@code Future} will receive a callback
* in which it will attempt to cancel itself.

A few potential solutions if we decide to make a code change:

  1. AbstractTransformFuture.cancel delegates to inputFuture.cancel (requires dropping TrustedFuture, and waiting to null the inputFuture reference until after a successful completion). Pro: This avoids the disappearing resource problem. Con: Cancel is not guaranteed to result in immediate completion of the transformed future.
  2. New ListenableFuture transform/catch(async) apis on Futures with cancellation behavior matching lazyTransform, allowing callers to choose the desired behavior.
  3. Overloads of existing APIs which take some form of callback that is called when a result is received but cannot be set, allowing resources to be released.

What do you think? Feedback is always appreciated, thanks!

@cpovirk
Copy link
Member

cpovirk commented Jul 27, 2020

There is definitely a fundamental issue around releasing resources. I think you might be talking about something slightly different, but bear with me for a moment.

Suppose that the transformed future is canceled:

  • after the call to settableFuture.set succeeds, but
  • before the transformation function can start

In that case, no code is in a position to release the resource. The best you can hope for is to attach a listener to the transformed future (or perhaps some other future) that closes the resource upon completion. But this isn't necessarily always feasible.

To address that problem, @netdpb had done some work internally on a "ClosingFuture" class. It allows whatever task produces the managed resource to register it for closing when the chain of operations completes.

Again, I'm not 100% sure if you're talking about this problem or something slightly different. But regardless, I believe that your solution (1) solves this by preventing the transformed future from being canceled during that window. However, for Guava, we've tried to be more opinionated about how cancellation works, so we have avoided letting cancellation fail unless the future is actually done. I don't think we'd want to change that here. (IIRC, while it solves the problem in this case, it doesn't solve the problem if the managed resource comes from some prior Future operation, like a transform or submit: Such an operation has no way to insert a hook for when set fails.)

The best chance we have of doing something that would help you is probably releasing ClosingFuture (sort of like your solution (3)). (Well, and adding docs is always a nice thing to do.) I don't think there are current plans to do that, but hearing that this bit you at least provides us with a nudge. Thanks for letting us know, and sorry for the trouble.

@carterkozak
Copy link
Contributor Author

Thanks @cpovirk, the problem you describe sounds exactly like what I've observed. The time between set succeeding and before the transformation starts is roughly equivalent to the time after the transformation starts and before it completes if we can assume we're able to fail a cancel invocation by returning false. I agree that additional documentation expanding on operations like transform would have been helpful, and that ListeningExecutorService behavior in this case does make things much more complicated.
For the time being, my library is well enough isolated, and it's easy enough to use a separate utility function to transform values internally in most cases.
I like the idea of a ClosingFuture with the ability manage resource ownership, I'd be very keen to replace my workaround with this kind of first class solution if/when it becomes available!

@cpovirk cpovirk changed the title Resource leak using Futures.transform on managed resources Add documentation on Futures APIs and/or release "ClosingFuture" API for resources that must be released/closed Jul 27, 2020
@cpovirk cpovirk added P2 and removed P3 labels Aug 12, 2020
@cpovirk cpovirk added this to the NEXT milestone Aug 12, 2020
@cpovirk
Copy link
Member

cpovirk commented Sep 16, 2020

https://github.com/google/guava/blob/master/guava/src/com/google/common/util/concurrent/ClosingFuture.java

Next release, hopefully in the not too distant future.

We should also still document ClosingFuture from ListenableFuture, Futures, etc., but let's call that #3975.

@cpovirk cpovirk closed this as completed Sep 16, 2020
@carterkozak
Copy link
Contributor Author

Fantastic! Thanks, Chris.

@cpovirk cpovirk modified the milestones: NEXT, 30.0 Oct 16, 2020
@cpovirk
Copy link
Member

cpovirk commented Oct 19, 2020

This is in 30.0, released Friday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants