ZOOKEEPER-4246: Resource leaks in org.apache.zookeeper.server.persistence.SnapStream#getInputStream and #getOutputStream

[kelloggm]

Bug report is here: https://issues.apache.org/jira/browse/ZOOKEEPER-4246

This fix is simple: it just closes the possibly-leaked streams and re-throws the exception. We can't use a try-with-resources or a finally block here because in the happy case the resulting streams need to be returned open. I checked each of the other constructor calls in these methods, and none of the others can throw an exception as far as I can tell.

[maoling]

@kelloggm Looks reasonable.

I noticed the first because of the use of the error-prone GZIPOutputStream, and the other two after looking at the surrounding code.

Could you please show us how the error-prone complain about this?

[kelloggm]

@maoling Sorry for the confusion. You asked:

Could you please show us how the error-prone complain about this?

in reference to my comment:

I noticed the first because of the use of the error-prone GZIPOutputStream, and the other two after looking at the surrounding code.

In this comment, I'm using "error-prone" as an adjective - i.e. I'm saying that I think GZIPOutputStream is easy to use incorrectly. I did not intend to refer to error-prone, Google's code analysis tool. From your comment, it sounds like you understood me to mean that tool - sorry for the confusion! As far as I'm aware, that tool cannot find this bug, but I haven't run it on this code.

[maoling]

@kelloggm Sorry for my misunderstanding for code analysis tool: error-prone. Do you observe this resource leaks in the production or just your code analysis?

[anmolnar]

Good catch.

[anmolnar]

Put the entire block in a try-catch:

try {
        switch (getStreamMode(file.getName())) {
        case GZIP:
            is = new GZIPInputStream(fis);
            break;
        case SNAPPY:
            is = new SnappyInputStream(fis);
            break;
        case CHECKED:
        default:
            is = new BufferedInputStream(fis);
        }
        return new CheckedInputStream(is, new Adler32());
} catch (IOException e) {
    fis.close();
    throw e;
}

[kelloggm]

I've made this change. Would you all also prefer one try around the entire switch in the other method, where only one of the cases can throw? It would make the code look more symmetrical, but it's otherwise unnecessary.

[kelloggm]

@maoling no worries, sorry again for the confusion

@kelloggm Sorry for my misunderstanding for code analysis tool: error-prone. Do you observe this resource leaks in the production or just your code analysis?

I didn't encounter these in production, to be clear. I was already familiar with the JDK bug I linked in my bug report, and got suspicious when I saw GZIP was supported here so I looked at the code and noticed these.

[kelloggm]

@anmolnar is there anything else you need from me on this PR? It's been a few weeks and I don't want it to be forgotten.

[ztzg]

LGTM.

I've made this change. Would you all also prefer one try around the entire switch in the other method, where only one of the cases can throw? It would make the code look more symmetrical, but it's otherwise unnecessary.

I, for one, have a slight preference for the "symmetrical" version, but let's not bikeshed.

@anmolnar, @maoling: do you approve?

[maoling]

LGTM.
If no other concerns, I'll merge it at this weekend(05-15). Cc @ztzg @anmolnar @kelloggm

[eolivelli]

Lgtm

[maoling]

@kelloggm
Thanks for your contribution.