google / google/error-prone

Find Futures.combine (or whenAllSucceed/whenAllComplete.call/callAsync) tasks that read from a non-input Future

Open
#390 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Java
Stars
7.2k
Forks
820
Avg merge
5h 9m
Merged PRs (30d)
50

Description

The summary is probably unclear, so here's an example:

```
Futures.whenAllComplete(a, b, c).call(
new Callable() {
@Override
public Foo call() throws Exception {
return doSomething(a.get(), b.get(), c.get(), d.get()); // uh-oh
}
});
```

Here, we provide a task to be executed once `a`, `b`, and `c` are complete. But our task accidentally depends on `d`, as well. The result is code that accidentally blocks, perhaps even on a network thread. I'm told that this was [common in <redacted Google product>](http://go/ibuoo) before they switched to a higher-level API.

I'm hoping that such a check could be an error, but there may be false positives:
- Maybe `d` is known to be an input to `a`, so if `a` is done, so too is `d` (assuming that `a` wasn't somehow cancelled without affecting `d`). But I'm hoping that those cases are rare and that their authors see them as fragile. They can always add `d` to the `whenAllComplete` call without a change in behavior.
- Maybe the code actually says something like `d.isDone() ? d.get() : someDefaultValue`. If this comes up often enough, we could add an `isDone` heuristic.

We probably want to recognize calls to other variants of `get`:
- maybe `get(timeout, unit)`, though perhaps not if the timeout is 0 (since this would be similar to the `isDone` check)
- `Futures.getUninterruptibly`
- `Futures.getChecked` and `getUnchecked`
- possibly `Futures.getNow` and/or `getDone` (if we add such things to Guava)
- possibly `CompletableFuture.getNow`

I'm unsure about `getNow` methods. On the one hand, the use of `getNow` prevents the code from hanging. On the other hand, if users use `getNow` for exactly that reason, they would lose the checking that Error Prone would provide.

I might try to write this someday, but it probably won't be anytime soon. If I do, I might want to steal a few pieces from my Google-internal `AsyncFunctionReturnsImmediate`.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.