AbstractFuture.afterCommit?

[cpovirk]

In a recent review, I said:

I wonder if there's a place for some kind of afterCommit() hook that, like afterDone(), runs upon a call to set()/setException()/cancel() but then also runs on a call to setFuture()....

The idea is that it could reduce duplication between afterDone and other "We just computed the value, so we're done with the inputs" code, like here:

https://github.com/google/guava/blob/2d4c517de9f12ba4dd2d9f720e700d7c835ecc4c/guava/src/com/google/common/util/concurrent/TrustedListenableFutureTask.java#L83

However, I'm not sure that a one-size-fits-all solution will work well here. Compare what we do in AbstractTransformFuture:

https://github.com/google/guava/blob/2d4c517de9f12ba4dd2d9f720e700d7c835ecc4c/guava/src/com/google/common/util/concurrent/AbstractTransformFuture.java#L89

There, we null out the input-future field even before we run the function. (Hmm, I guess that this prevents toString from giving full detail.... We might want to think about that more.)

I suppose that TrustedListenableFutureTask could similarly null out this.task immediately after reading it to a local variable. If so, then we could extract that nulling-out into a method that we call from both run and afterDone. But it's already just a single assignment. Even in the case of AbstractTransformFuture, it's only 2 assignments. Plus, it's nice to keep those assignments in the same method as the reads just to avoid needing to think more about cross-method dependencies. It would be one thing if we could automatically null out the fields at the beginning of run, but we can't because, well, we need the fields in run :) There is probably some possible crazy solution here -- a series of RunnableTrustedFutureWithInputs1, RunnableTrustedFutureWithInputs2, etc. classes with 1, 2, etc. fields of user-configurable type, all automatically read and passes as parameters to a run(T1, T2, ...) method (and pendingToString(T1, T2, ...)? and maybe even have a special class for "one of the inputs is a Future, so you should cancel it automatically?") -- but I don't think we need more layers of superclasses right now unless we have a very good reason. (Edit: And the T1, T2 parameters won't have useful names by default, though obviously subclasses can name them whatever they want.)

...OK, maybe I'm a little intrigued, in part because AbstractTransformFuture/AbstractCatchingFuture may share more code with TrustedListenableFutureTask/InterruptibleTask if we ever make them interruptible. At that point, we could consider sharing more code. But I don't think we're there yet.

[cpovirk]

One tiny unfortunate thing is that, if we have both afterCommit and afterDone, we need to think about which one happens first in simple cases like set. I think it pretty clearly has to be afterCommit first, since that's what happens with complicated setFuture cases. So my tiny concern is more that we have to think about the ordering between two separate methods at all. But maybe that's no worse than what we already deal with when the "afterCommit" logic is split across multiple methods, often duplicated. And hopefully our tests for the setFuture case would catch any bugs.

[cpovirk]

Oh, and I just noted in a code review that it would be just slightly nice for AbstractTransformFuture to null out function before calling maybePropagateCancellationTo(inputFuture). We would get that for free if we nulled out function in afterCommit.

[cpovirk]

(When completing a future, we'd have to know whether afterCommit has been called. But this is just asking whether the previous state was SetFuture. That information is available to all callers of complete(), so they could pass it as a parameter.)

[cpovirk]

The lack of this was a contributor to internal bug 139750022.

[cpovirk]

I prototyped this internally in CL 266457646. Some thoughts:

• AbstractFuture becomes marginally more complicated (and complete acquires a branch, though one that's easy to understand and hopefully won't affect performance much). (This would be more motivation for #2934.)
• AggregateFuture and its subclasses become a little simpler if I move their cleanup into afterCommit.
• But moving the cleanup into afterCommit can make that cleanup happen at least slightly later. In the case of AggregateFuture, I think it's only undoing recent extra-optimization CL 266130757, not important fix CL 265489523. In the case of AbstractTransformFuture and the other single-input futures, I'm not sure the cleanup happens any later, though maybe it gets harder for us to release the reference to the input future until after apply has run.
• I wonder if a better way to approach some of this is what I've previously called FutureListener (#1988). Then we wouldn't need a reference to the input future at all except for cancellation purposes. Hmm, but we'd probably still have other fields that we'd want to clear at the same time as we would have wanted to clear the input-future field. Plus, we'd probably still want to use afterCommit to clear the input future....
• I've come to dislike the name "afterCommit," particularly for a method that will often be run after afterDone (because afterDone might need to interrupt the input future, which we'd be clearing in afterCommit). (But it can also be run before or even concurrently.) Maybe we should just call it releaseResources -- though of course subclasses could still use it for other purposes, and I'd feel bad if they had good use cases that don't fit under the umbrella of "releasing resources." (I'm not sure if subclasses would have such use cases or not. Maybe I should look at some existing afterDone implementations that might use the proposed method.)

[cpovirk]

(Another somewhat related idea I had was to give AbstractFuture a concept of "set to the result provided by the given InterruptibleTask." This would complicate AbstractFuture further (its implementation and API both, really). But it would also help with the cleanup and interruption of the task, and it might make #1989 simpler when we hopefully do that someday.)