You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2021/03/11 22:50:26 UTC
[GitHub] [zookeeper] kelloggm opened a new pull request #1638: ZOOKEEPER-4246: Resource leaks in org.apache.zookeeper.server.persistence.SnapStream#getInputStream and #getOutputStream
kelloggm opened a new pull request #1638:
URL: https://github.com/apache/zookeeper/pull/1638
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] kelloggm commented on pull request #1638: ZOOKEEPER-4246: Resource leaks in org.apache.zookeeper.server.persistence.SnapStream#getInputStream and #getOutputStream
Posted by GitBox <gi...@apache.org>.
kelloggm commented on pull request #1638:
URL: https://github.com/apache/zookeeper/pull/1638#issuecomment-829471452
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on pull request #1638: ZOOKEEPER-4246: Resource leaks in org.apache.zookeeper.server.persistence.SnapStream#getInputStream and #getOutputStream
Posted by GitBox <gi...@apache.org>.
maoling commented on pull request #1638:
URL: https://github.com/apache/zookeeper/pull/1638#issuecomment-813008280
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] kelloggm commented on pull request #1638: ZOOKEEPER-4246: Resource leaks in org.apache.zookeeper.server.persistence.SnapStream#getInputStream and #getOutputStream
Posted by GitBox <gi...@apache.org>.
kelloggm commented on pull request #1638:
URL: https://github.com/apache/zookeeper/pull/1638#issuecomment-809575037
@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](https://en.wiktionary.org/wiki/error-prone) - i.e. I'm saying that I think GZIPOutputStream is easy to use incorrectly. I did not intend to refer to [error-prone](https://github.com/google/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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on pull request #1638: ZOOKEEPER-4246: Resource leaks in org.apache.zookeeper.server.persistence.SnapStream#getInputStream and #getOutputStream
Posted by GitBox <gi...@apache.org>.
maoling commented on pull request #1638:
URL: https://github.com/apache/zookeeper/pull/1638#issuecomment-808859314
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on pull request #1638: ZOOKEEPER-4246: Resource leaks in org.apache.zookeeper.server.persistence.SnapStream#getInputStream and #getOutputStream
Posted by GitBox <gi...@apache.org>.
maoling commented on pull request #1638:
URL: https://github.com/apache/zookeeper/pull/1638#issuecomment-835781106
Looking:)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] kelloggm commented on pull request #1638: ZOOKEEPER-4246: Resource leaks in org.apache.zookeeper.server.persistence.SnapStream#getInputStream and #getOutputStream
Posted by GitBox <gi...@apache.org>.
kelloggm commented on pull request #1638:
URL: https://github.com/apache/zookeeper/pull/1638#issuecomment-814340351
@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](https://bugs.openjdk.java.net/browse/JDK-8180899) 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling closed pull request #1638: ZOOKEEPER-4246: Resource leaks in org.apache.zookeeper.server.persistence.SnapStream#getInputStream and #getOutputStream
Posted by GitBox <gi...@apache.org>.
maoling closed pull request #1638:
URL: https://github.com/apache/zookeeper/pull/1638
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] kelloggm commented on a change in pull request #1638: ZOOKEEPER-4246: Resource leaks in org.apache.zookeeper.server.persistence.SnapStream#getInputStream and #getOutputStream
Posted by GitBox <gi...@apache.org>.
kelloggm commented on a change in pull request #1638:
URL: https://github.com/apache/zookeeper/pull/1638#discussion_r608080517
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/persistence/SnapStream.java
##########
@@ -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 {
Review comment:
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 `case`s can throw? It would make the code look more symmetrical, but it's otherwise unnecessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] asfgit closed pull request #1638: ZOOKEEPER-4246: Resource leaks in org.apache.zookeeper.server.persistence.SnapStream#getInputStream and #getOutputStream
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1638:
URL: https://github.com/apache/zookeeper/pull/1638
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] anmolnar commented on a change in pull request #1638: ZOOKEEPER-4246: Resource leaks in org.apache.zookeeper.server.persistence.SnapStream#getInputStream and #getOutputStream
Posted by GitBox <gi...@apache.org>.
anmolnar commented on a change in pull request #1638:
URL: https://github.com/apache/zookeeper/pull/1638#discussion_r607945496
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/persistence/SnapStream.java
##########
@@ -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 {
Review comment:
Put the entire block in a try-catch:
```java
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;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on pull request #1638: ZOOKEEPER-4246: Resource leaks in org.apache.zookeeper.server.persistence.SnapStream#getInputStream and #getOutputStream
Posted by GitBox <gi...@apache.org>.
maoling commented on pull request #1638:
URL: https://github.com/apache/zookeeper/pull/1638#issuecomment-841682958
@kelloggm
Thanks for your contribution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling edited a comment on pull request #1638: ZOOKEEPER-4246: Resource leaks in org.apache.zookeeper.server.persistence.SnapStream#getInputStream and #getOutputStream
Posted by GitBox <gi...@apache.org>.
maoling edited a comment on pull request #1638:
URL: https://github.com/apache/zookeeper/pull/1638#issuecomment-835781106
LGTM.
If no other concerns, I'll merge it at this weekend(05-15). Cc @ztzg @anmolnar @kelloggm
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org