Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 4 commits into from
Closed

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

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e896efa
ZOOKEEPER-4246: Resource leaks in org.apache.zookeeper.server.persist…
kelloggm Mar 11, 2021
04c4b2f
fix accidental tab
kelloggm Mar 12, 2021
f023851
remove all the tabs for real
kelloggm Mar 12, 2021
fa1c0f0
surround whole switch with one try block instead of two inside the sw…
kelloggm Apr 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 20 additions & 3 deletions zookeeper-server/src/main/java/org/apache/zookeeper/server/persistence/SnapStream.java
View file
Open in desktop
Expand Up @@ -104,10 +104,20 @@ public static CheckedInputStream getInputStream(File file) throws IOException {
InputStream is;
switch (getStreamMode(file.getName())) {
case GZIP:
is = new GZIPInputStream(fis);
try {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

is = new GZIPInputStream(fis);
} catch (IOException e) {
fis.close();
throw e;
}
break;
case SNAPPY:
is = new SnappyInputStream(fis);
try {
is = new SnappyInputStream(fis);
} catch (IOException e) {
fis.close();
throw e;
}
break;
case CHECKED:
default:
Expand All @@ -129,9 +139,16 @@ public static CheckedOutputStream getOutputStream(File file, boolean fsync) thro
OutputStream os;
switch (streamMode) {
case GZIP:
os = new GZIPOutputStream(fos);
try {
os = new GZIPOutputStream(fos);
} catch (IOException e) {
fos.close();
throw e;
}
break;
case SNAPPY:
// Unlike SnappyInputStream, the SnappyOutputStream
// constructor cannot throw an IOException.
os = new SnappyOutputStream(fos);
break;
case CHECKED:
Expand Down