Error-prone: False alarm for StreamResourceLeak with method-chaining

[PhilippWendler]

What version of Error Prone are you using?

2.2.0

Does this issue reproduce with the latest release?

Yes.

What did you do?

Example program:

public class Test {

  public static void main(String... args) throws IOException {
    try (Stream<Path> paths = getPaths()) {
      paths.forEach(System.out::println);
    }
  }

  @MustBeClosed
  public static Stream<Path> getPaths() throws IOException {
    return Files.walk(Paths.get("/tmp")).filter(Files::isReadable);
  }
}

What did you expect to see?

No warning, because the method getPaths is annotated with @MustBeClosed.

What did you see instead?

A warning for getPaths:

Test.java:40: warning: [StreamResourceLeak] Streams that encapsulate a closeable resource should be closed using try-with-resources
    [javac]     return Files.walk(Paths.get("/tmp")).filter(Files::isReadable);
    [javac]                      ^
    [javac]     (see http://errorprone.info/bugpattern/StreamResourceLeak)
    [javac]   Did you mean 'try (Stream<Path> stream = Files.walk(Paths.get("/tmp"))) {'?

The warning goes away (as expected) if the filter call is removed and the result of Files.walk is returned directly.

[boris-petrov]

Another false positive is using something like:

java.nio.file.Files.walk(somePath).forEach(file -> {});

This gives a StreamResourceLeak warning but is actually safe.

[PhilippWendler]

Actually, the warning is even produced if I inline getPaths():

    try (Stream<Path> paths = Files.walk(Paths.get("/tmp")).filter(Files::isReadable)) {
      paths.forEach(System.out::println);
    }

The suggested fix in this case is broken:

Did you mean 'try (Stream<Path> paths ;'?

@boris-petrov In your case one could argue that is preferable to use try-with-resources to close the stream explicitly and not rely on automatic closing.

[boris-petrov]

@PhilippWendler - I'm not sure how would you use try-with-resources on what I gave as an example - forEach returns void. Also, why would you use try-with-resources when forEach is a terminal operation?

[PhilippWendler]

@boris-petrov You would need to use a local variable for the stream if you want to close it explicitly. Not sure if its worth it, but it might be a reason why error-prone wants to flag code like this, whereas for the original example, the stream is actually explicitly closed and error-prone still warns about it.

[michaelhixson]

Terminal operations don't call close(). For example, this code does not print "closed":

Stream.of(1, 2, 3)
      .onClose(() -> System.out.println("closed"))
      .forEach(n -> System.out.println(n));

[boris-petrov]

@michaelhixson - thanks for the example, I didn't know this and it doesn't make sense in my book. :) I also don't understand why are these streams supposed to be closed at all?

[nick-someone]

Those streams are supposed to be close()d since they hold onto file descriptors. Per Files.walk() javadoc:

The returned stream encapsulates one or more DirectoryStreams. If timely disposal of file system resources is required, the try-with-resources construct should be used to ensure that the stream's close method is invoked after the stream operations are completed.

That is what this check is trying to enforce.

Towards fixing this bug:
https://github.com/google/error-prone/blob/083720dc364dcad7fd71c288e342ed66ae025df6/core/src/main/java/com/google/errorprone/bugpatterns/AbstractMustBeClosedChecker.java#L80 should be updated to allow for the case of "a leakable stream is immediately chained to a non-terminal operation, then that object is successfully closed". There's probably some refactoring to do to make this not mess up http://errorprone.info/bugpattern/MustBeClosedChecker