Iqss/7309 s3 resource leaks #7313
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
using try-with-resources where it looks straight forward, or IOUtils.closeQuietly() where it isn't. (The latter is deprecated in favor of using try-with-resources these days.)
qqmyers
commented
Oct 7, 2020
| @@ -131,6 +131,7 @@ public boolean canDecodeInput(Object source) throws IOException { | |||
|
|
|||
| @Override | |||
| public boolean canDecodeInput(BufferedInputStream stream) throws IOException { | |||
| //who closes this stream? | |||
There was a problem hiding this comment.
guessing this method could/should close it but I didn't see where it was getting called.
There was a problem hiding this comment.
We are not calling these methods at this time. They are part of a builtin infrastructure for automatically matching ingest plugins to specific file types... We are not using it now, but we may go back to it.
It would be the responsibility of the code that calls these methods to close the streams.
qqmyers
commented
Oct 7, 2020
|
|
||
| tempIngestSourceChannel.transferFrom(dataFileChannel, 0, storageIO.getSize()); | ||
|
|
||
| File tempFile = File.createTempFile("tempIngestSourceFile", ".tmp"); |
There was a problem hiding this comment.
FWIW - this private method doesn't get used according to eclipse - do we need it?
There was a problem hiding this comment.
It really appears an unused leftover method.
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it: Closes more streams/channels that may hold S3 http connections open and exhaust the aws s3 client's pool.
Which issue(s) this PR closes:
Closes #7309
Special notes for your reviewer:
Tried to use the new try-with-resources format where that looked simple and just used Apache's IOUtils.closeQuietly() for others (it just closes if the object isn't null and eats any exception). If I understand correctly, using try-with-resources will still throw any exception (unless you add a catch) so I think the only effect is to assure the streams/channels are closed.
One place where this PR isn't complete is in the tabulardata/impl/plugins classes. There are other classes there that do the same thing as DTAFileReaderSpi in creating a stream that doesn't look like it gets closed, and in having methods that have a stream param. In this latter case, I didn't find where these methods were getting called, so I didn't want to just close the streams in case that was already handled by the caller (or the stream is reused, etc.). So - some discussion about what could/should happen there would help.
Suggestions on how to test this: If we can reliably exhaust the pool by setting a low value and using some of the functionality affected by this PR (e.g. thumbnails created for pdfs), then applying this PR should stop that problem.
Testing should also cover making sure that things still work, e.g. that I haven't accidentally closed streams/channels before they're used.
Does this PR introduce a user interface change? If mockups are available, please link/include them here: No.
Is there a release notes update needed for this change?:
Additional documentation: