Error-prone: Find resource leaks (AutoCloseables that should have been closed)

[cushon]

_Original issue created by [email protected] on 2014-10-30 at 04:01 PM_

---

Originally reported by kevinb9n:

Many ways of obtaining an AutoCloseable instance imply responsibility on the obtainer to close that instance (e.g. by obtaining it inside try-with-resources), and many don't. For error-prone to be able to detect resource leaks it would need some way to differentiate. How?

eaftan notes:

We can use dataflow to determine how the instance was obtained. Is it always the case that knowing the provenance of the instance is enough to know whether it needs to be closed? Can you provide examples other than try-with-resources?

[cushon]

For error-prone to be able to detect resource leaks it would need some way to differentiate. How?

Maybe we could create an annotation to distinguish between the two cases?

[JeffreyFalgout]

JSR 305 has some annotations which could be used:
@WillClose
@WillNotClose
@WillCloseWhenClosed

[kevinb9n]

For any method whose return type implements AutoCloseable, or constructor of a concrete class that implements AutoCloseable, I suspect that it's most commonly the case that the caller _does_ take on the responsibility to close that instance (or to return it to someone else, passing the buck). It would probably be more efficient for us to annotate the exceptions (and blacklist for the sources we can't annotate, like StringWriter constructor).

[kevinb9n]

I'm... sorta surprised I said that. It seems a lot more practical to try building up a whitelist of things that we know imply caller responsibility.

I'm newly concerned about this issue because JDK 8 brings an attractive nuisance of a method called Files.lines(). People will use it like this:

ImmutableList<String> lines =
    Files.lines(path, UTF_8)
        .filter(line -> line.contains(str))
        .limit(1000)
        .collect(ImmutableList::toImmutableList);

This is bad. We need them to write

ImmutableList<String> lines;
try (Stream<String> lines = Files.lines(path, UTF_8)) {
  lines = stream
      .filter(line -> line.contains(str))
      .limit(1000)
      .collect(ImmutableList::toImmutableList);
}

[kevinb9n]

FYI, I glanced at 20 usages of Files.lines() and 9 of them were not closing.

[cushon]

There's now a check specifically for Files.lines (FilesLinesLeak), and a @MustBeClosed annotation with enforcement.

[leventov]

Are there plans to support annotations like @WillClose? Currently MustBeClosedChecker produces an overwhelming amount of false positives on an attempt to use @MustBeClosed consistently in a codebase. Support for @WillClose would help to reduce that noise tremendously.