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

FileUtils.deleteDirectory could throw UncheckedIOException #478

Closed
wants to merge 2 commits into from

Conversation

jglick
Copy link

@jglick jglick commented Sep 7, 2023

At some point between 2.11.0 and 2.13.0, I think as of 323d376, there seems to have been an incompatible change due to use of UncheckedIOException. I found the following from a piece of code calling FileUtils.deleteDirectory and catching and logging IOException, which failed to catch the runtime exception:

java.nio.file.NoSuchFileException: /path/to/file
	at java.base/sun.nio.fs.UnixException.translateToIOException(UnixException.java:92)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:111)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:116)
	at java.base/sun.nio.fs.UnixFileAttributeViews$Posix.readAttributes(UnixFileAttributeViews.java:234)
	at java.base/sun.nio.fs.UnixFileAttributeViews$Posix.readAttributes(UnixFileAttributeViews.java:147)
	at java.base/sun.nio.fs.UnixFileSystemProvider.readAttributes(UnixFileSystemProvider.java:149)
	at java.base/sun.nio.fs.LinuxFileSystemProvider.readAttributes(LinuxFileSystemProvider.java:99)
	at java.base/java.nio.file.Files.readAttributes(Files.java:1764)
	at org.apache.commons.io.function.Uncheck.apply(Uncheck.java:162)
Caused: java.io.UncheckedIOException
	at org.apache.commons.io.function.Uncheck.wrap(Uncheck.java:242)
	at org.apache.commons.io.function.Uncheck.apply(Uncheck.java:164)
	at org.apache.commons.io.file.PathUtils.readAttributes(PathUtils.java:1259)
	at org.apache.commons.io.file.PathUtils.readPosixFileAttributes(PathUtils.java:1349)
	at org.apache.commons.io.file.PathUtils.deleteFile(PathUtils.java:582)
	at org.apache.commons.io.file.PathUtils.delete(PathUtils.java:476)
	at org.apache.commons.io.FileUtils.forceDelete(FileUtils.java:1337)
	at org.apache.commons.io.function.IOStream.lambda$forAll$11(IOStream.java:340)
	at java.base/java.util.Spliterators$ArraySpliterator.forEachRemaining(Spliterators.java:948)
	at java.base/java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:658)
	at org.apache.commons.io.function.IOStream.forAll(IOStream.java:338)
	at org.apache.commons.io.function.IOStreams.forAll(IOStreams.java:42)
	at org.apache.commons.io.function.IOStreams.forAll(IOStreams.java:36)
	at org.apache.commons.io.function.IOConsumer.forAll(IOConsumer.java:80)
	at org.apache.commons.io.FileUtils.cleanDirectory(FileUtils.java:333)
	at org.apache.commons.io.FileUtils.deleteDirectory(FileUtils.java:1192)

Never mind the exact reason for the failure of Files.readAttributes; something to do with NFS I think. The point is that deleteDirectory should either succeed, or throw some subtype of IOException. Reproduced similar behavior in the unit test prior to src/main/ fix:

org.opentest4j.AssertionFailedError: Unexpected exception type thrown, expected: <java.io.IOException> but was: <java.io.UncheckedIOException>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:67)
	at org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:35)
	at org.junit.jupiter.api.Assertions.assertThrows(Assertions.java:3111)
	at org.apache.commons.io.file.PathUtilsDeleteFileTest.testForceDeleteFileDoesNotExist(PathUtilsDeleteFileTest.java:115)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
Caused by: java.io.UncheckedIOException: java.nio.file.NoSuchFileException: /tmp/org.apache.commons.io.file.PathUtilsDeleteFileTest15920347910072703873/nonexistent
	at org.apache.commons.io.function.Uncheck.wrap(Uncheck.java:339)
	at org.apache.commons.io.function.Uncheck.apply(Uncheck.java:165)
	at org.apache.commons.io.file.PathUtils.readAttributes(PathUtils.java:1259)
	at org.apache.commons.io.file.PathUtils.readPosixFileAttributes(PathUtils.java:1349)
	at org.apache.commons.io.file.PathUtils.deleteFile(PathUtils.java:582)
	at org.apache.commons.io.file.PathUtils.deleteFile(PathUtils.java:544)
	at org.apache.commons.io.file.PathUtilsDeleteFileTest.lambda$testForceDeleteFileDoesNotExist$0(PathUtilsDeleteFileTest.java:115)
	at org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:53)
	... 6 more
Caused by: java.nio.file.NoSuchFileException: /tmp/org.apache.commons.io.file.PathUtilsDeleteFileTest15920347910072703873/nonexistent
	at java.base/sun.nio.fs.UnixException.translateToIOException(UnixException.java:92)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:111)
	at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:116)
	at java.base/sun.nio.fs.UnixFileAttributeViews$Posix.readAttributes(UnixFileAttributeViews.java:234)
	at java.base/sun.nio.fs.UnixFileAttributeViews$Posix.readAttributes(UnixFileAttributeViews.java:147)
	at java.base/sun.nio.fs.UnixFileSystemProvider.readAttributes(UnixFileSystemProvider.java:149)
	at java.base/sun.nio.fs.LinuxFileSystemProvider.readAttributes(LinuxFileSystemProvider.java:99)
	at java.base/java.nio.file.Files.readAttributes(Files.java:1764)
	at org.apache.commons.io.function.Uncheck.apply(Uncheck.java:163)
	... 12 more

I attempted to track all direct or indirect callers of PathUtils.readAttributes and make sure they either documented UncheckedIOException or translated it to IOException. I did not attempt to do the same for the many other uses of Uncheck.

@codecov-commenter
Copy link

codecov-commenter commented Sep 7, 2023

Codecov Report

Merging #478 (74c2a3a) into master (45cbb66) will decrease coverage by 0.04%.
The diff coverage is 55.55%.

@@             Coverage Diff              @@
##             master     #478      +/-   ##
============================================
- Coverage     84.79%   84.75%   -0.04%     
  Complexity     3364     3364              
============================================
  Files           227      227              
  Lines          8076     8083       +7     
  Branches        953      953              
============================================
+ Hits           6848     6851       +3     
- Misses          974      978       +4     
  Partials        254      254              
Files Changed Coverage Δ
.../apache/commons/io/file/DirectoryStreamFilter.java 71.42% <33.33%> (-28.58%) ⬇️
...ain/java/org/apache/commons/io/file/PathUtils.java 71.97% <66.66%> (-0.20%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

Thanks for catching this. This is pretty bad.

There's probably more work to be done. I'm not sure I've ever seen a case where UncheckedIOException is anything other than a bug.

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

@elharo
Copy link
Contributor

elharo commented Jan 29, 2024

#491 fixed this so this PR can be closed

@garydgregory
Copy link
Member

Closing per previous comment.

@jglick jglick deleted the UncheckedIOException branch January 29, 2024 17:36
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.

4 participants