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.

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

Iqss/7309 s3 resource leaks #7313

Conversation

qqmyers
Copy link
Member

@qqmyers qqmyers commented Oct 7, 2020

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:

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.)
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.003%) to 19.465% when pulling ec791bd on GlobalDataverseCommunityConsortium:IQSS/7309-S3_resource_leaks into 62da27f on IQSS:develop.

@@ -131,6 +131,7 @@ public boolean canDecodeInput(Object source) throws IOException {

@Override
public boolean canDecodeInput(BufferedInputStream stream) throws IOException {
//who closes this stream?
Copy link
Member Author

Choose a reason for hiding this comment

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

guessing this method could/should close it but I didn't see where it was getting called.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


tempIngestSourceChannel.transferFrom(dataFileChannel, 0, storageIO.getSize());

File tempFile = File.createTempFile("tempIngestSourceFile", ".tmp");
Copy link
Member Author

Choose a reason for hiding this comment

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

FWIW - this private method doesn't get used according to eclipse - do we need it?

Copy link
Contributor

Choose a reason for hiding this comment

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

It really appears an unused leftover method.

Copy link
Contributor

@landreev landreev left a comment

Choose a reason for hiding this comment

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

Awesome!

@kcondon kcondon self-assigned this Oct 8, 2020
@kcondon kcondon merged commit 6af5099 into IQSS:develop Oct 8, 2020
@djbrooke djbrooke added this to the 5.2 milestone Oct 8, 2020
@djbrooke djbrooke mentioned this pull request Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Potential S3 resource leaks exposed by 5.1
5 participants