javatry-catchinputstreambufferedreaderinputstreamreader

How to properly close input streams?


There are two methods of doing the same thing. The first:

public String getIpByName(String name) {
       var builders = NetworkUtil.buildProcess(name);
    try (var ip = new BufferedReader(new InputStreamReader(executor.execPipelineAndGetInputStream(builders)))) {
        return ip.lines().collect(Collectors.joining());
    } catch (IOException exception) {
        throw new NotFoundException(name);
    }
}

Second:

public String getIpByName(String name) {
    var builders = NetworkUtil.buildProcess(name);
    try (var result = executor.execPipelineAndGetInputStream(builders)) {
        var input = new InputStreamReader(result);
        var reader = new BufferedReader(input);
        var ip = reader.lines().collect(Collectors.joining());
        input.close();
        reader.close();
        return ip;
    } catch (IOException exception) {
        throw new NotFoundException(name);
    }

Which method would be more correct?


Solution

  • The second one is never correct. Either the close() calls are important, or they are not. If they are important, they should be try/finally-ied or try-with-resourced. If they are not important, they are not important, and you should not bother writing the statements.

    Thus, we have 3 alternatives, not 2, and only your first alternative is left unchanged:

    Second:

    public String getIpByName(String name) {
        var builders = NetworkUtil.buildProcess(name);
        try (var result = executor.execPipelineAndGetInputStream(builders)) {
            var input = new InputStreamReader(result);
            var reader = new BufferedReader(input);
            return reader.lines().collect(Collectors.joining());
        } catch (IOException exception) {
            throw new NotFoundException(name);
        }
    }
    

    and there's a third, using the resource chaining feature of try-with-resources:

    public String getIpByName(String name) {
        var builders = NetworkUtil.buildProcess(name);
        try (var result = executor.execPipelineAndGetInputStream(builders);
          var input = new InputStreamReader(result);
          var reader = new BufferedReader(input)) {
    
            return reader.lines().collect(Collectors.joining());
        } catch (IOException exception) {
            throw new NotFoundException(name);
        }
    }
    

    Of these 3 options, you get into a bit of a debate; the first option seems fine; the implementations of these so-called 'filter streams' (those are readers/writers/outputstreams/inputstreams that 'wrap' another stream) have the deal that close()ing them will close the thing they wrapped. Thus, ordinarily #1 seems fine, but if an exception were to occur in the constructor of the filterstream, then you leak a resource. Will these exceptions occur? Ordinarily impossible, but not always, here's a trivial way to cause a commonly used filterstream to crash in construction:

    new InputStreamReader(someData, "some non existing charset");
    

    Thus, I strongly advise against the first. That leaves door #2 and door #3: It really doesn't matter; I think the second one is probably the most readable, but the problem with the second option is that various IDE and linting tools will complain about it, they have a hard time telling the difference between resource-representing streamlikes, and filters/memory-only streamlikes. This is not their fault, really: How could they possibly know if the InputStream returned by your execPipelineAndGetInputStream method is supposed to be 'thing you need to close' or 'thing you can close but it doesnt matter' or 'thing you should not be closing at all'?