10

I am using Java 8 streams in place of many old style for loops to iterate through a bunch of results and produce summary statistics. For example:

int messages = IntStream.rangeClosed(0, 7).map(ids::get).reduce(Integer::sum).getAsInt();

Note: I know there are other ways to do the counting that I show above. I'm doing it that way in order to illustrate my question.

I am using SonarQube 5.3 with the Java 3.9 plugin. In that configuration, the above line of code gives me a violation of squid rule S2095: "Resources should be closed." That's the result I would expect to see if an AutoCloseable (e.g., a FileInputStream) was opened but never closed.

So here's my question: does the terminal operation reduce close the stream? Should it? Or is this a false positive in the squid rule?

PaoloC
  • 3,817
  • 1
  • 23
  • 27
Bob Cross
  • 22,116
  • 12
  • 58
  • 95
  • ```IntStream``` has ```onClose(Runnable closeHandler)``` method inherited from ```BaseStream``` class. You can examine when stream is closed or not closed using this handler. –  Jan 13 '16 at 14:42
  • Monitor https://jira.sonarsource.com/browse/SONARJAVA-1478 – PaoloC Feb 07 '18 at 11:11

1 Answers1

10

It isn't closed, because AutoCloseable interface works only inside try-with-resources. But this close operation is totally unnecessary for IntStream as it said in AutoCloseable interface javadoc:

However, when using facilities such as java.util.stream.Stream that support both I/O-based and non-I/O-based forms, try-with-resources blocks are in general unnecessary when using non-I/O-based forms.

So yes S2095 is a false positive for IntStream. That will be hopefully fixed by SONARJAVA-1478

PaoloC
  • 3,817
  • 1
  • 23
  • 27
Andremoniy
  • 34,031
  • 20
  • 135
  • 241
  • So my questions about when the stream is actually closed still stand. Are you saying this is a false positive in the squid rule? Or are you saying that the whole point is moot? – Bob Cross Jan 13 '16 at 14:19
  • 3
    @BobCross This is indeed a false positive. The `AutoClosable` interface changed in Java 8 and now something that is `AutoClosable` may not actually hold a resource to close. – Tunaki Jan 13 '16 at 14:25
  • 5
    Static analysis doesn't detect bugs; it detects things that might be bugs, and might be worth looking at. In this case, you looked at it, and determined its not a bug, because the stream holds no resources that need release. – Brian Goetz Jan 13 '16 at 14:26
  • @Tunaki, you might want to write that up as an answer – Bob Cross Jan 13 '16 at 14:26
  • 2
    @Tunaki More precisely, what `AutoCloseable` means is something that *can* be auto-closed via try-with-resources. It is not necessarily the case that all closeable entities actually *need to* be closed. (And, this isn't new with Java 8; ByteArrayInputStream has been around forever, and it doesn't need to be closed either.) – Brian Goetz Jan 13 '16 at 14:27
  • @BrianGoetz, I agree that I can see that this isn't a bug in this code. I'm currently updating our static analysis configuration and, if this is a false positive, I'll be disabling this detector until it is updated to handle the difference between an IntStream and a FileInputStream. – Bob Cross Jan 13 '16 at 14:29
  • 5
    @BrianGoetz well we do our best to avoid wasting user time to not look at unworthy things that are not bug :) – benzonico Jan 13 '16 at 16:22
  • 4
    @BobCross this sounds like a false positive indeed. We would have to finetune this rule to exclude types that are not actually holding resources. This can end up to be really tricky in some cases. I want to have a closer look before actually opening a ticket to handle this case but I will link it to this question – benzonico Jan 13 '16 at 16:24
  • 3
    @benzonico Good. I think you want to walk back to find the stream source (if you can) and make your decision based on that. I would focus on the static stream factory methods in Files (walk, lines, etc) as being the resource-holders. I would not worry about methods like BufferedReader.lines(), because in that case, the stream is not the primary resource holder, its the BufferedReader, and that's what needs to be closed (closing the resulting stream doesn't close the BR anyway.) – Brian Goetz Jan 13 '16 at 17:46