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

Improve exception readability in case a directory can't be deleted because it still contains files #814

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

Vlatombe
Copy link
Member

Found this while investigating a test flake report and realized the current reported exception was a bit confusing.

So, proposing this:

Use suppressed exceptions rather than wrap the exception so that we get the primary cause first, then details.

Also resolve files relative to their containing folders.

Before:

java.io.IOException: [REDACTED]/target/tmp/j h16565971146678333138/users/Fred_10744520479289247763/config.xml
	at org.jvnet.hudson.test.TemporaryDirectoryAllocator.delete(TemporaryDirectoryAllocator.java:144)
	at org.jvnet.hudson.test.TemporaryDirectoryAllocator.delete(TemporaryDirectoryAllocator.java:131)
	at org.jvnet.hudson.test.TemporaryDirectoryAllocator.delete(TemporaryDirectoryAllocator.java:131)
	at org.jvnet.hudson.test.TemporaryDirectoryAllocator.dispose(TemporaryDirectoryAllocator.java:99)
	at org.jvnet.hudson.test.TestEnvironment.dispose(TestEnvironment.java:84)
	at org.jvnet.hudson.test.JenkinsRule.after(JenkinsRule.java:527)
	at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:665)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.lang.Thread.run(Thread.java:840)
Caused by: java.nio.file.DirectoryNotEmptyException: [REDACTED]/target/tmp/j h16565971146678333138/users/Fred_10744520479289247763
	at java.base/sun.nio.fs.UnixFileSystemProvider.implDelete(UnixFileSystemProvider.java:246)
	at java.base/sun.nio.fs.AbstractFileSystemProvider.deleteIfExists(AbstractFileSystemProvider.java:110)
	at java.base/java.nio.file.Files.deleteIfExists(Files.java:1191)
	at org.jvnet.hudson.test.TemporaryDirectoryAllocator.delete(TemporaryDirectoryAllocator.java:141)
	... 8 more

After:

java.nio.file.DirectoryNotEmptyException: [REDACTED]/target/tmp/j h16565971146678333138/users/Fred_10744520479289247763
        at java.base/sun.nio.fs.UnixFileSystemProvider.implDelete(UnixFileSystemProvider.java:289)
        at java.base/sun.nio.fs.AbstractFileSystemProvider.deleteIfExists(AbstractFileSystemProvider.java:109)
        at java.base/java.nio.file.Files.deleteIfExists(Files.java:1191)
        at org.jvnet.hudson.test.TemporaryDirectoryAllocator.delete(TemporaryDirectoryAllocator.java:142)
        at org.jvnet.hudson.test.TemporaryDirectoryAllocator.dispose(TemporaryDirectoryAllocator.java:100)
        at org.jvnet.hudson.test.TestEnvironment.dispose(TestEnvironment.java:84)
        at org.jvnet.hudson.test.JenkinsRule.after(JenkinsRule.java:586)
        at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:724)
        at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317)
        at java.base/java.lang.Thread.run(Thread.java:1583)
        Suppressed: java.io.IOException: These files still exist : config.xml
                at org.jvnet.hudson.test.TemporaryDirectoryAllocator.delete(TemporaryDirectoryAllocator.java:146)
                ... 6 more

Manually tested with the following patch to produce non-empty directories.

Index: src/main/java/org/jvnet/hudson/test/TemporaryDirectoryAllocator.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/main/java/org/jvnet/hudson/test/TemporaryDirectoryAllocator.java b/src/main/java/org/jvnet/hudson/test/TemporaryDirectoryAllocator.java
--- a/src/main/java/org/jvnet/hudson/test/TemporaryDirectoryAllocator.java	(revision 3b5659834a066c27a991c742649d976b535dc18a)
+++ b/src/main/java/org/jvnet/hudson/test/TemporaryDirectoryAllocator.java	(date 1723541060918)
@@ -126,13 +126,6 @@
 
     private void delete(Path p) throws IOException {
         LOGGER.fine(() -> "deleting " + p);
-        if (Files.isDirectory(p, LinkOption.NOFOLLOW_LINKS)) {
-            try (DirectoryStream<Path> children = Files.newDirectoryStream(p)) {
-                for (Path child : children) {
-                    delete(child);
-                }
-            }
-        }
         try {
             if (isWindows()) {
                 // Windows throws an access denied exception when deleting read-only files

Testing done

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

…cause it still contains files

Use suppressed exceptions rather than wrap the exception so that we get
the primary cause first, then details.

Also resolve files relative to their containing folders.

Before:

```
java.io.IOException: [REDACTED]/target/tmp/j h16565971146678333138/users/Fred_10744520479289247763/config.xml
	at org.jvnet.hudson.test.TemporaryDirectoryAllocator.delete(TemporaryDirectoryAllocator.java:144)
	at org.jvnet.hudson.test.TemporaryDirectoryAllocator.delete(TemporaryDirectoryAllocator.java:131)
	at org.jvnet.hudson.test.TemporaryDirectoryAllocator.delete(TemporaryDirectoryAllocator.java:131)
	at org.jvnet.hudson.test.TemporaryDirectoryAllocator.dispose(TemporaryDirectoryAllocator.java:99)
	at org.jvnet.hudson.test.TestEnvironment.dispose(TestEnvironment.java:84)
	at org.jvnet.hudson.test.JenkinsRule.after(JenkinsRule.java:527)
	at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:665)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.lang.Thread.run(Thread.java:840)
Caused by: java.nio.file.DirectoryNotEmptyException: [REDACTED]/target/tmp/j h16565971146678333138/users/Fred_10744520479289247763
	at java.base/sun.nio.fs.UnixFileSystemProvider.implDelete(UnixFileSystemProvider.java:246)
	at java.base/sun.nio.fs.AbstractFileSystemProvider.deleteIfExists(AbstractFileSystemProvider.java:110)
	at java.base/java.nio.file.Files.deleteIfExists(Files.java:1191)
	at org.jvnet.hudson.test.TemporaryDirectoryAllocator.delete(TemporaryDirectoryAllocator.java:141)
	... 8 more
```

After:

```
java.nio.file.DirectoryNotEmptyException: [REDACTED]/target/tmp/j h16565971146678333138/users/Fred_10744520479289247763
        at java.base/sun.nio.fs.UnixFileSystemProvider.implDelete(UnixFileSystemProvider.java:289)
        at java.base/sun.nio.fs.AbstractFileSystemProvider.deleteIfExists(AbstractFileSystemProvider.java:109)
        at java.base/java.nio.file.Files.deleteIfExists(Files.java:1191)
        at org.jvnet.hudson.test.TemporaryDirectoryAllocator.delete(TemporaryDirectoryAllocator.java:142)
        at org.jvnet.hudson.test.TemporaryDirectoryAllocator.dispose(TemporaryDirectoryAllocator.java:100)
        at org.jvnet.hudson.test.TestEnvironment.dispose(TestEnvironment.java:84)
        at org.jvnet.hudson.test.JenkinsRule.after(JenkinsRule.java:586)
        at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:724)
        at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317)
        at java.base/java.lang.Thread.run(Thread.java:1583)
        Suppressed: java.io.IOException: These files still exist : config.xml
                at org.jvnet.hudson.test.TemporaryDirectoryAllocator.delete(TemporaryDirectoryAllocator.java:146)
                ... 6 more
```
@Vlatombe Vlatombe requested a review from jglick August 13, 2024 09:26
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Thanks!

@jglick jglick merged commit cff7ad4 into jenkinsci:master Aug 13, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants