Skip to content

[common] Close input when copy output close fails#3746

Draft
wolfkill wants to merge 1 commit into
apache:mainfrom
wolfkill:fix/copybytes-close-all-streams
Draft

[common] Close input when copy output close fails#3746
wolfkill wants to merge 1 commit into
apache:mainfrom
wolfkill:fix/copybytes-close-all-streams

Conversation

@wolfkill

Copy link
Copy Markdown

What is the purpose of the change

IOUtils.copyBytes(..., close = true) currently closes the output stream before the input stream in a single finally block. If out.close() throws, in.close() is skipped even though the method contract says both streams are closed in the finally clause.

This patch makes the close path attempt both closes and preserves the first close failure, adding any later close failure as suppressed.

Brief change log

  • Close both output and input streams when copyBytes owns stream closure, even if the first close fails.
  • Add a regression test covering an output close failure and verifying the input stream is still closed.

Verifying this change

RED:

  • mvn -pl fluss-common -am -DskipITs -Dcheckstyle.skip -Dspotless.check.skip=true -DfailIfNoTests=false -Dtest=IOUtilsTest#testCopyBytesClosesInputWhenOutputCloseFails test
    • failed before the fix because the input stream was not closed

GREEN:

  • mvn -pl fluss-common -am -DskipITs -Dcheckstyle.skip -Dspotless.check.skip=true -DfailIfNoTests=false -Dtest=IOUtilsTest#testCopyBytesClosesInputWhenOutputCloseFails test
  • mvn -pl fluss-common -am -DskipITs -Dcheckstyle.skip -Dspotless.check.skip=true -DfailIfNoTests=false -Dtest=IOUtilsTest test
  • mvn -pl fluss-common -am -DskipITs -Dspotless.check.skip=true -DfailIfNoTests=false -Dtest=IOUtilsTest test
  • git diff --check

Note: running the same Maven test command without -Dspotless.check.skip=true fails in this local JDK environment before reaching fluss-common, in fluss-test-utils spotless/google-java-format with NoSuchMethodError: com.sun.tools.javac.util.Log$DeferredDiagnosticHandler.getDiagnostics().

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.

1 participant