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

[all] Replacing IOUtils.closeQuietly(foo) with try-with-resources statements #1401

Merged
merged 4 commits into from Oct 26, 2018

Conversation

Projects
None yet
4 participants
@Thunderforge
Contributor

Thunderforge commented Oct 19, 2018

Replaces most IOUtils.closeQuietly(foo) calls with try-with-resources statements for better memory handling. I have run ./mvnw clean verify and it passes.

There were two remaining issues that I was unsure of how to address:

  • In some cases, IOUtils.closeQuietly(foo) was suppressing an IOException, and since that's a checked exception, I had to catch it if it wasn't being closed already. The most common way of handling this I saw in existing code was to just print the stack trace, so that's what I did. If I should handle it differently, let me know.
  • There were several instances in PMD Core where I couldn't get rid of IOUtils.closeQuietly(), and I would appreciate suggestions on how to handle them:
    • RuleSetWriter L65 has it as the only call in a public close() method.
    • Formatter L192 calls IOUtils.closeQuietly() if we're in an error state, but the method returns the unclosed Writer if not, and that becomes an instance variable that is closed at a later time.
    • AbstractRenderer L82 has the call as part of a flush() method
    • IOUtil L56 has it as the only operation of tryCloseClassLoader(ClassLoader classLoader), which is a public method.

Please let me know if I should handle the remaining issues in any way, and if there are any other issues with the code I have changed.

@pmd-test

This comment has been minimized.

pmd-test commented Oct 19, 2018

1 Message
📖 This changeset introduces 0 new violations and 0 new errors,
removes 0 violations and 0 errors. Full report
This changeset introduces 0 new violations and 0 new errors,
removes 0 violations and 0 errors. Full report

Generated by 🚫 Danger

@hooperbloob

This comment has been minimized.

Contributor

hooperbloob commented Oct 19, 2018

Nice. Do we have a rule that identifies these optimizations? If not, we should.

@adangel adangel changed the title from Replacing IOUtils.closeQuietly(foo) with try-with-resources statements to [all] Replacing IOUtils.closeQuietly(foo) with try-with-resources statements Oct 22, 2018

@adangel adangel added the is:WIP label Oct 24, 2018

@Thunderforge

This comment has been minimized.

Contributor

Thunderforge commented Oct 25, 2018

Nice. Do we have a rule that identifies these optimizations? If not, we should.

@hooperbloob There is CloseResource that detects if you don't call .close(), but it only works for classes related to databases (e.g. Connection, Statement, ResultSet). I'm unsure of how that would have worked for IOUtils.closeQuietly(in) because that's a third-party Apache method.

Looks like we now have #1405 to request a new rule that will detect IOUtils.closeQuietly(in).

@adangel adangel self-assigned this Oct 26, 2018

@adangel

This comment has been minimized.

Member

adangel commented Oct 26, 2018

@Thunderforge
About your questions for the other places where we explictily use closeQuietly:

I would appreciate suggestions on how to handle them:

  • RuleSetWriter L65 has it as the only call in a public close() method.

Without a deeper look: Maybe we should make RuleSetWriter itself implement AutoClosable? Then we can use a try-with-resources when RuleSetWriter is used. We'd still need to either use closeQuietly here or implement it manually ourselves (if we want to get rid of the deprecated method call).
Or we just throw the IOException here - the caller has to deal with it then. So, we just close it and do not use closeQuietly.

  • Formatter L192 calls IOUtils.closeQuietly() if we're in an error state, but the method returns the
    unclosed Writer if not, and that becomes an instance variable that is closed at a later time.

Theoretically an error would only occur, if the File is not found, which could happen when creating the FileOutputStream. So I'm not sure, whether we need to close anything at all in the error case. Since the method already throws IOException, I guess, we should just let it throw. The method returns a Writer, and the caller is then responsible for closing it.

  • AbstractRenderer L82 has the call as part of a flush() method

Hm... closing in the flush method sounds wrong. We probably should provide an explicit close method or document, that a renderer is not responsible for closing the underlying writer...

  • IOUtil L56 has it as the only operation of tryCloseClassLoader(ClassLoader classLoader), which
    is a public method.

I guess, there is not much to do about it. We could make the method more specific for PMD's use case: We use this method twice and check before, whether the classloader is ours, so we could move the check inside the method (instanceof ClasspathClassLoader). But we still need to close the classloader explicitly (also because ClassLoader is not Closable by default, so we can't use it in a try-with-resources...)

I'll look at your updated PR later today - thanks for your work!

@adangel adangel added this to the 6.9.0 milestone Oct 26, 2018

@adangel adangel removed the is:WIP label Oct 26, 2018

@adangel adangel merged commit 3cd41d7 into pmd:master Oct 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

adangel added a commit that referenced this pull request Oct 26, 2018

@Thunderforge Thunderforge deleted the Thunderforge:replacing-ioutils.closequietly()-with-try-with-resources branch Oct 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment