@TempDir fails to delete read-only directories on Windows

[garretwilson]

I'm using JUnit 5.9.3 with Java 17 on Windows 10 with @TempDir. There are several ironies in this ticket. 😄

It appears that @TempDir knows how to clean up files that are set to read-only, but doesn't know how to clean up directories that are read-only.

I'm working on my own ticket, JAVA-312: Option to force deletion of read-only files for file subtrees. This is primarily to clean up Git temporary repositories; see JGit Bug 582051. I of course need to test my library to make sure it cleans things up with a "force" parameter.

These JUnit tests work:

/** @see {@link Files#deleteIfExists(Path, boolean)} */
@Test
@EnabledOnOs(value = OS.WINDOWS, disabledReason = "Working with DOS read-only files should only be done on Windows.")
void verifyDeleteIfExistsNotForceFailsOnReadOnlyFile(@TempDir final Path tempDir) throws IOException {
  assumeTrue(readAttributes(tempDir, BasicFileAttributes.class) instanceof DosFileAttributes,
      "We assume that on Windows the file system uses DOS attributes; otherwise this test will not work.");
  final Path file = createFile(tempDir.resolve("foo.bar"));
  setAttribute(file, ATTRIBUTE_DOS_READONLY, true);
  assertThat(exists(file), is(true));
  assertThrows(AccessDeniedException.class, () -> Files.deleteIfExists(file, false));
}

/** @see {@link Files#deleteIfExists(Path, boolean)} */
@Test
@EnabledOnOs(value = OS.WINDOWS, disabledReason = "Working with DOS read-only files should only be done on Windows.")
void verifyDeleteIfExistsForceDeletesReadOnlyFile(@TempDir final Path tempDir) throws IOException {
  assumeTrue(readAttributes(tempDir, BasicFileAttributes.class) instanceof DosFileAttributes,
      "We assume that on Windows the file system uses DOS attributes; otherwise this test will not work.");
  final Path file = createFile(tempDir.resolve("foo.bar"));
  setAttribute(file, ATTRIBUTE_DOS_READONLY, true);
  assertThat(exists(file), is(true));
  assertThat(Files.deleteIfExists(file, true), is(true));
  assertThat(exists(file), is(false));
}

Basically the first test verifies that without the "force" parameter, my deletion method will fail if a file is set to read-only. Note also that this results in a file being set to read-only at the end of the test, but JUnit cleans it up just fine.

Now I do the same test for a read-only directory:

/** @see {@link Files#deleteIfExists(Path, boolean)} */
@Test
@EnabledOnOs(value = OS.WINDOWS, disabledReason = "Working with DOS read-only files should only be done on Windows.")
void verifyDeleteIfExistsNotForceFailsOnReadOnlyDirectory(@TempDir final Path tempDir) throws IOException {
  assumeTrue(readAttributes(tempDir, BasicFileAttributes.class) instanceof DosFileAttributes,
      "We assume that on Windows the file system uses DOS attributes; otherwise this test will not work.");
  final Path directory = createDirectory(tempDir.resolve("dir"));
  setAttribute(directory, ATTRIBUTE_DOS_READONLY, true);
  assertThat(exists(directory), is(true));
  assertThrows(AccessDeniedException.class, () -> Files.deleteIfExists(directory, false));
}

/** @see {@link Files#deleteIfExists(Path, boolean)} */
@Test
@EnabledOnOs(value = OS.WINDOWS, disabledReason = "Working with DOS read-only files should only be done on Windows.")
void verifyDeleteIfExistsForceDeletesReadOnlyDirectory(@TempDir final Path tempDir) throws IOException {
  assumeTrue(readAttributes(tempDir, BasicFileAttributes.class) instanceof DosFileAttributes,
      "We assume that on Windows the file system uses DOS attributes; otherwise this test will not work.");
  final Path directory = createDirectory(tempDir.resolve("dir"));
  setAttribute(directory, ATTRIBUTE_DOS_READONLY, true);
  assertThat(exists(directory), is(true));
  assertThat(Files.deleteIfExists(directory, true), is(true));
  assertThat(exists(directory), is(false));
}

Same as before, except in the first method, verifyDeleteIfExistsNotForceFailsOnReadOnlyDirectory(…), JUnit itself fails when cleaning things up:

Stack Trace

java.io.IOException: Failed to delete temp directory C:\Users\user\AppData\Local\Temp\junit…. The following paths could not be deleted (see suppressed exceptions for details): <root>, dir
	at org.junit.jupiter.engine.extension.TempDirectory$CloseablePath.createIOExceptionWithAttachedFailures(TempDirectory.java:351)
	at org.junit.jupiter.engine.extension.TempDirectory$CloseablePath.close(TempDirectory.java:252)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.jupiter.engine.execution.ExtensionValuesStore.lambda$closeAllStoredCloseableValues$3(ExtensionValuesStore.java:68)
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1540)
	at java.base/java.util.stream.SortedOps$RefSortingSink.end(SortedOps.java:395)
	at java.base/java.util.stream.Sink$ChainedReference.end(Sink.java:258)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:485)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
	at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150)
	at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:497)
	at org.junit.jupiter.engine.execution.ExtensionValuesStore.closeAllStoredCloseableValues(ExtensionValuesStore.java:68)
	at org.junit.jupiter.engine.descriptor.AbstractExtensionContext.close(AbstractExtensionContext.java:80)
	at org.junit.jupiter.engine.execution.JupiterEngineExecutionContext.close(JupiterEngineExecutionContext.java:53)
	at org.junit.jupiter.engine.descriptor.JupiterTestDescriptor.cleanUp(JupiterTestDescriptor.java:222)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$cleanUp$1(TestMethodTestDescriptor.java:155)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.cleanUp(TestMethodTestDescriptor.java:155)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.cleanUp(TestMethodTestDescriptor.java:68)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$cleanUp$10(NodeTestTask.java:167)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.cleanUp(NodeTestTask.java:167)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:98)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1540)
	at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:155)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
	at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1540)
	at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:155)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
	at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
	at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.submit(SameThreadHierarchicalTestExecutorService.java:35)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:57)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine.execute(HierarchicalTestEngine.java:54)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:147)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:127)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:90)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.lambda$execute$0(EngineExecutionOrchestrator.java:55)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.withInterceptedStreams(EngineExecutionOrchestrator.java:102)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:54)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:114)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:95)
	at org.junit.platform.launcher.core.DefaultLauncherSession$DelegatingLauncher.execute(DefaultLauncherSession.java:91)
	at org.junit.platform.launcher.core.SessionPerRequestLauncher.execute(SessionPerRequestLauncher.java:60)
	at org.eclipse.jdt.internal.junit5.runner.JUnit5TestReference.run(JUnit5TestReference.java:98)
	at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:40)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:529)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:756)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:452)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:210)
	Suppressed: java.nio.file.DirectoryNotEmptyException: C:\Users\user\AppData\Local\Temp\junit…
		at java.base/sun.nio.fs.WindowsFileSystemProvider.implDelete(WindowsFileSystemProvider.java:267)
		at java.base/sun.nio.fs.AbstractFileSystemProvider.delete(AbstractFileSystemProvider.java:105)
		at java.base/java.nio.file.Files.delete(Files.java:1141)
		at org.junit.jupiter.engine.extension.TempDirectory$CloseablePath$1.deleteAndContinue(TempDirectory.java:294)
		at org.junit.jupiter.engine.extension.TempDirectory$CloseablePath$1.postVisitDirectory(TempDirectory.java:289)
		at org.junit.jupiter.engine.extension.TempDirectory$CloseablePath$1.postVisitDirectory(TempDirectory.java:265)
		at java.base/java.nio.file.Files.walkFileTree(Files.java:2742)
		at java.base/java.nio.file.Files.walkFileTree(Files.java:2796)
		at org.junit.jupiter.engine.extension.TempDirectory$CloseablePath.deleteAllFilesAndDirectories(TempDirectory.java:265)
		at org.junit.jupiter.engine.extension.TempDirectory$CloseablePath.close(TempDirectory.java:250)
		... 64 more
	Suppressed: java.nio.file.AccessDeniedException: C:\Users\user\AppData\Local\Temp\junit…\dir
		at java.base/sun.nio.fs.WindowsException.translateToIOException(WindowsException.java:89)
		at java.base/sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:103)
		at java.base/sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:108)
		at java.base/sun.nio.fs.WindowsFileSystemProvider.implDelete(WindowsFileSystemProvider.java:270)
		at java.base/sun.nio.fs.AbstractFileSystemProvider.delete(AbstractFileSystemProvider.java:105)
		at java.base/java.nio.file.Files.delete(Files.java:1141)
		at org.junit.jupiter.engine.extension.TempDirectory$CloseablePath$1.deleteAndContinue(TempDirectory.java:294)
		at org.junit.jupiter.engine.extension.TempDirectory$CloseablePath$1.postVisitDirectory(TempDirectory.java:289)
		at org.junit.jupiter.engine.extension.TempDirectory$CloseablePath$1.postVisitDirectory(TempDirectory.java:265)
		at java.base/java.nio.file.Files.walkFileTree(Files.java:2742)
		at java.base/java.nio.file.Files.walkFileTree(Files.java:2796)
		at org.junit.jupiter.engine.extension.TempDirectory$CloseablePath$1.resetPermissionsAndTryToDeleteAgain(TempDirectory.java:315)
		at org.junit.jupiter.engine.extension.TempDirectory$CloseablePath$1.deleteAndContinue(TempDirectory.java:304)
		at org.junit.jupiter.engine.extension.TempDirectory$CloseablePath$1.postVisitDirectory(TempDirectory.java:289)
		at org.junit.jupiter.engine.extension.TempDirectory$CloseablePath$1.postVisitDirectory(TempDirectory.java:265)
		at java.base/java.nio.file.Files.walkFileTree(Files.java:2742)
		at java.base/java.nio.file.Files.walkFileTree(Files.java:2796)
		at org.junit.jupiter.engine.extension.TempDirectory$CloseablePath.deleteAllFilesAndDirectories(TempDirectory.java:265)
		at org.junit.jupiter.engine.extension.TempDirectory$CloseablePath.close(TempDirectory.java:250)
		... 64 more

The workaround is to add my own cleanup code:

/** @see {@link Files#deleteIfExists(Path, boolean)} */
@Test
@EnabledOnOs(value = OS.WINDOWS, disabledReason = "Working with DOS read-only files should only be done on Windows.")
void verifyDeleteIfExistsNotForceFailsOnReadOnlyDirectory(@TempDir final Path tempDir) throws IOException {
  assumeTrue(readAttributes(tempDir, BasicFileAttributes.class) instanceof DosFileAttributes,
      "We assume that on Windows the file system uses DOS attributes; otherwise this test will not work.");
  final Path directory = createDirectory(tempDir.resolve("dir"));
  setAttribute(directory, ATTRIBUTE_DOS_READONLY, true);
  try {
    assertThat(exists(directory), is(true));
    assertThrows(AccessDeniedException.class, () -> Files.deleteIfExists(directory, false));
  } finally {
    setAttribute(directory, ATTRIBUTE_DOS_READONLY, false); //always remove the read-only attribute to allow JUnit to clean up
  }
}

But that to some extent defeats the purpose of @TempDir. As JUnit cleans up read-only files on Windows, I'm sure it was just an oversight that it doesn't clean up read-only directories as well.

[garretwilson]

I'm not positive if the read-only directory eventually got cleaned up some how; I can't find the JUnit temp directory in my user's temp directory. Nevertheless we don't want it to throw an exception in any case.

[sbrannen]

Can you please provide the implementation of setAttribute(directory, ATTRIBUTE_DOS_READONLY, true);?

[garretwilson]

It's part of the Java NIO API: Files.setAttribute(…)

In this case ATTRIBUTE_DOS_READONLY is a constant that equates to dos:readonly. You can find the values in DosFileAttributeView. See also DosFileAttributeView.name().

[marcphilipp]

I agree that this was an oversight. @garretwilson Could you please submit a PR to fix that?

[garretwilson]

Could you please submit a PR to fix that?

In theory I would love to, but I'm several layers deep in bug reports, PRs, etc. For the moment I'm just removing the read-only flag in my unit test. Otherwise I'll never get to the end of my current ticket, much less my current project.

I just wanted to file the ticket so it would at least be known so that it could be addressed some day. I don't think it has super-high severity or priority. If things calm down I would be glad to address it.

[garretwilson]

Another note: I don't know if you already have code to do this, but it's not strightforward—it requires detecting Java NIO support for DOS file attributes and POSIX file attributes, the latter of which sets read-only status by manipulating the parent directory permissions, etc. I'm working on this myself in my own library in JAVA-312. I have to reopen the ticket and implement the POSIX version. I was just now figuring out how to test this on Linux using a Docker image. Lots of fun still ahead.

[marschall]

Another note: I don't know if you already have code to do this, but it's not strightforward—it requires detecting Java NIO support for DOS file attributes and POSIX file attributes, ...

Set<String> supportedFileAttributeViews = tempDir.getFileSystem().supportedFileAttributeViews();
// dos is supported on unix
supportedFileAttributeViews.contains("dos") && !supportedFileAttributeViews.contains("unix")

Am I missing something?

readAttributes(tempDir, BasicFileAttributes.class) instanceof DosFileAttributes

I would not recommend this. This looks like relying on an implementation detail that can change at any point.

setAttribute(directory, ATTRIBUTE_DOS_READONLY, false);

I find this hard to read. I would do the following

Files.getFileAttributeView(directory, DosFileAttributeView.class).setReadOnly(false);

[garretwilson]

Am I missing something?

One thing you're missing is the rest of the sentence which you replaced with ellipsis 😉 regarding the different levels of "read-only" designation in POSIX for directories. My entire sentence taken into context was making the point that there were several gotchas involved, each one in itself perhaps not being too onerous, but taken together meant that the task is not straightforward as a whole.

> supportedFileAttributeViews.contains("dos")

Doing a "contains" without a proper parsing of the string is not best practices, and has launched a thousand bugs, as the decades of parsing User-Agent strings for browser detection will bear out. You might think that's being nit-picky, but it adds to the multitude of gotchas making this not straightforward. (Edit: Ignore this paragraph—I was mistakenly thinking a single string was being checked to contain "dos", but actually it's a Set<String>, which makes this logic correct. I was glancing over this too quickly, having historically seen too many people detecting substrings in an incorrect way. This was a side-note anyway—the larger point is that there's a few things to watch out for regarding read-only-ness across platforms.)

I didn't say it's hard. I'm just saying it's not something I can dash off in a couple of lines in five minutes.

[marschall]

Doing a "contains" without a proper parsing of the string is not best practices, and has launched a thousand bugs, as the decades of parsing User-Agent strings for browser detection will bear out. You might think that's being nit-picky, but it adds to the multitude of gotchas making this not straightforward.

Please tell me what the proper parsing here would look like and the possible gotchas.

[garretwilson]

Please tell me what the proper parsing here would look like and the possible gotchas.

Actually I made a mistake hurriedly looking over your code; I thought that contains(…) was being used on an individual string value (e.g. "some string containing dos and other things.contains("dos")). In this case the value is in fact a Set<String>, and "dos" is indeed the full name of an implementation of DosFileAttributeView, which means that logic is correct for detecting if a DosFileAttributeView is used. I'll edit my answer above to reflect this.

[marschall]

Actually I made a mistake hurriedly looking over your code; I thought that contains(…) was being used on an individual string value (e.g. "some string containing dos and other things.contains("dos")). In this case the value is in fact a Set<String>, and "dos" is indeed the full name of an implementation of DosFileAttributeView, which means that logic is correct for detecting if a DosFileAttributeView is used. I'll edit my answer above to reflect this.

Thanks for clarifying, I though I missed something or overlooked something. All good.