Use try-with-resources to ensure streams are closed#710
Use try-with-resources to ensure streams are closed#710oliverlockwood wants to merge 6 commits intodrewnoakes:mainfrom
Conversation
drewnoakes
left a comment
drewnoakes
left a comment
There was a problem hiding this comment.
Thank you for resurrecting this PR with more targeted changes.
In reviewing these, I'm wondering which of these changes is actually addressing a memory leak. I can only see one of the several changes here that, to my understanding, should have a positive impact at run time (and even then I'm not sure), which is the closing of InflaterInputStream.
There are a few places that introduce a change in behaviour, where the new code will close a stream that's passed as a parameter. This doesn't feel very idiomatic in Java, and I'm reluctant to take those changes. The general philosophy seems to be if you create a stream, you close it. So the close should be handled by the caller. This allows a caller to re-use the stream if they need to, perhaps by seeking or performing further reads beyond where the called method reached.
There's no need to close ByteArrayInputStream or ByteArrayOutputStream. I'm unclear on BufferedInputStream but suspect that most (all?) implementations would avoid using unmanaged resources.
Adding try/finally blocks to methods that didn't previously have them can cause the JIT to be more conservative in its code generation, so I like to avoid these when they don't add anything.
tl;dr can you share where you saw the leaks you mentioned please?
| try { | ||
| InflaterInputStream inflateStream = new InflaterInputStream(new ByteArrayInputStream(compressedProfile)); | ||
| try (ByteArrayInputStream bais = new ByteArrayInputStream(compressedProfile); | ||
| InflaterInputStream inflateStream = new InflaterInputStream(bais)) { |
There was a problem hiding this comment.
Looks good for the inflate stream, but closing ByteArrayInputStream has no effect, per the docs. Not a big deal to close both though.
There was a problem hiding this comment.
Closing a ByteArrayInputStream does nothing, but many static code analysis tools will complain about any AutoClosable that isn't managed by try-with-resources, so I'd say that's a pretty standard way to do it. But, you shouldn't need to "split" the stream creation, it should be possible to use try (new InflaterInputStream(new ByteArrayInputStream(compressedProfile))). The assumption is that the outer stream will always close the inner stream when closed.
There was a problem hiding this comment.
Thanks for the feedback - I will inline the bais variable both here and in other cases, below.
| try (BufferedInputStream bufferedInputStream = inputStream instanceof BufferedInputStream | ||
| ? (BufferedInputStream)inputStream : new BufferedInputStream(inputStream)) { |
There was a problem hiding this comment.
I don't think this method should be taking ownership of the inputStream parameter. The caller may wish to use it after calling this method. My sense is that the changes to this file should be undone.
Source/com/drew/lang/StreamUtil.java
Outdated
| public static byte[] readAllBytes(InputStream stream) throws IOException | ||
| { | ||
| ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); | ||
| try (ByteArrayOutputStream outputStream = new ByteArrayOutputStream()) { |
There was a problem hiding this comment.
Closing ByteArrayOutputStream does nothing. The changes in this file add overhead and can interfere with JIT compilation due to the addition of try/finally. I'd revert the changes to this file.
| BufferedReader reader = new BufferedReader(new InputStreamReader(stream)); | ||
| StringBuilder sb = new StringBuilder(); | ||
| String line; | ||
| while ((line = reader.readLine()) != null) { | ||
| sb.append(line); | ||
| try (InputStreamReader inputStreamReader = new InputStreamReader(stream); | ||
| BufferedReader reader = new BufferedReader(inputStreamReader)) { | ||
| StringBuilder sb = new StringBuilder(); | ||
| String line; | ||
| while ((line = reader.readLine()) != null) { | ||
| sb.append(line); | ||
| } | ||
| return sb.toString(); | ||
| } | ||
| return sb.toString(); |
There was a problem hiding this comment.
This will close the provided stream. I don't think the changes in this file add anything other than overhead.
| // Extract information from embedded image since it is metadata-rich | ||
| ByteArrayInputStream jpegmem = new ByteArrayInputStream(jpegrawbytes); | ||
| try { | ||
| try (ByteArrayInputStream jpegmem = new ByteArrayInputStream(jpegrawbytes)) { |
There was a problem hiding this comment.
I'm inclined to leave this change in because although as you point out, closing a BAIS has no effect, we already have a try block so it's zero overhead to do so (and actually saves a line of code).
But if you want me to revert this change, please say so.
…g multiple stream initialisers in try-with-resources; removing try-with-resources for BAIS/BAOS where it just adds noise
|
Hi @drewnoakes , @Nadahar - thank you for the feedback. Apologies for the delay in response - I was on holiday and then ill for a few days.
Hopefully despite this, the PR in its simplified form will be useful to you, us, and others. |
3 participants
This replaces the clumsily-raised PR #662 (the delivery of which I apologise for on behalf of my now ex-colleague!) - please close it now.
We had been working with a local build due to time pressures, and unfortunately Nick failed to isolate the functional changes (for submission upstream) from the local changes relating to publication of the artifact at our own co-ordinates.
The context of this PR is as follows:
java.lang.AutoCloseableinterface (implicitly including those that implementjava.io.Closeable, also).Hopefully now with context clear and noise reduced, this should be fairly quick and easy for you to review.
Many thanks for your time and maintenance of the library.