[CID-156932] - Fix the imaginary resource leak in NullOutputStream #172
Conversation
DigestOutputStream may change in the future, hence it worth to just refactor the code
|
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
| @@ -74,8 +74,10 @@ static Checksum forFile(File file) throws IOException { | |||
| static Checksum forURL(URL url) throws IOException { | |||
| try { | |||
| MessageDigest md = MessageDigest.getInstance(JarLoaderImpl.DIGEST_ALGORITHM); | |||
| Util.copy(url.openStream(), new DigestOutputStream(new NullOutputStream(), md)); | |||
| return new Checksum(md.digest(), md.getDigestLength() / 8); | |||
| try(OutputStream ostream = new DigestOutputStream(new NullOutputStream(), md)) { | |||
There was a problem hiding this comment.
If you actually were concerned about NullOutputStream leaking something (impossible), you would use
try (NullOutputStream nos = new NullOutputStream();
OutputStream ostream = new DigestOutputStream(nos)) {There was a problem hiding this comment.
Not really required, because DigestOutputStream closes the nested one
There was a problem hiding this comment.
Unless DigestOutputStream.<init> throws an exception. That is why as a rule you should declare every Closeable object you are constructing as a separate variable in the block.
| Util.copy(url.openStream(), new DigestOutputStream(new NullOutputStream(), md)); | ||
| return new Checksum(md.digest(), md.getDigestLength() / 8); | ||
| try(OutputStream ostream = new DigestOutputStream(new NullOutputStream(), md)) { | ||
| Util.copy(url.openStream(), ostream); |
There was a problem hiding this comment.
But here you are failing to provably close the one stream here which might actually hold open a socket or file handle! So you need to include this InputStream amongst the closeables being bound.
DigestOutputStreammay change in the future, hence it worth to just refactor the code. I have not found annotations which would indicate that no closing it required for NullOutputStream. There are such warnings in the core as well OTOH@reviewbybees