Skip to content

[IO-438] Clarify IOUtils.closeQuietly() Javadocs#857

Open
sarankumarbaskar wants to merge 2 commits into
apache:masterfrom
sarankumarbaskar:IO-438-fix-closequietly-javadocs
Open

[IO-438] Clarify IOUtils.closeQuietly() Javadocs#857
sarankumarbaskar wants to merge 2 commits into
apache:masterfrom
sarankumarbaskar:IO-438-fix-closequietly-javadocs

Conversation

@sarankumarbaskar

Copy link
Copy Markdown
Contributor

IOUtils.closeQuietly() ignores exceptions thrown during close, which is useful for cleanup paths but can be unsafe when closing output streams or writers after successful writes.

This PR clarifies the Javadocs to warn that output close failures may indicate buffered data was not fully written. It also removes examples that used closeQuietly() after IOUtils.copy(), since those examples could encourage ignoring output close errors.

No runtime behavior changed.

Resolves IO-438.

Checklist:

  • Read the contribution guidelines for this project.
  • Read the ASF Generative Tooling Guidance.
  • Ran mvn -Drat.skip=true -DskipTests javadoc:javadoc

Remove examples that quietly close output streams after copy operations, and document when close failures should not be ignored.

@garydgregory garydgregory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Sarankumar18
Don't delete examples, propose new ones.

…Show output streams being closed normally before falling back to closeQuietly() for cleanup.
@sarankumarbaskar

Copy link
Copy Markdown
Contributor Author

@garydgregory
Thanks, updated. I kept the examples and changed them to close the output stream normally first, then fall back to closeQuietly() only if the normal close was skipped. This keeps the example while avoiding silently ignored output close failures in the successful path.

@garydgregory garydgregory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Sarankumar18
Please see my comments.

* } finally {
* IOUtils.closeQuietly(inputStream);
* IOUtils.closeQuietly(outputStream);
* IOUtils.closeQuietly(outputStream); // only if normal close was skipped

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Sarankumar18
This example makes no sense to me:

  • First, you close() the OS on line 809 with the contradictory comment "close errors are handled"; the close() call does not handle errors.
  • Second, in the finally block, the comment "only if normal close was skipped" is not true because the finally block is always executed.
    Please come up with an example and comments that make sense and are consistent with what actually happens.
    TY

* try {
* return IOUtils.copy(inputStream, outputStream);
* IOUtils.copy(inputStream, outputStream);
* outputStream.close(); // close errors are handled

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same comment as above.

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.

2 participants