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

Ignore NoSuchFileException in @TempDir cleanup #3667

Conversation

gilday
Copy link
Contributor

@gilday gilday commented Jan 30, 2024

Overview

In TempDirectory's cleanup logic, while walking the temp directory, the file walker may fail to visit a file that has been removed by another thread or process. In this case, JUnit should ignore the NoSuchFileException, because there's nothing to clean up.

I have observed such failures when using JGit to manage repositories in temporary directories. JGit uses lock files that it cleans up in background threads. JGit and JUnit will race to delete the lock file, and the JUnit TempDirectory cleanup fails when JGit wins that race.

I considered what an automated test for this new behavior would look like. I could add a clear-box test that uses high-level concurrency objects to make sure that the test can delete a file between the time that the SimpleFileVisitor lists the temp directory and the time that the SimpleFileVisitor attempts to delete the file, but this would require adding new seams to TempDirectory that are inconsistent with the existing tests for that type. Would you accept this change without adding new test coverage?


I hereby agree to the terms of the JUnit Contributor License Agreement.


Definition of Done

@sbrannen sbrannen changed the title Ignore NoSuchFileException in TempDirectory Cleanup Ignore NoSuchFileException in TempDirectory cleanup Jan 31, 2024
@sbrannen sbrannen changed the title Ignore NoSuchFileException in TempDirectory cleanup Ignore NoSuchFileException in @TempDir cleanup Jan 31, 2024
While walking the temp directory, the file walker may fail to visit a
file that has been removed by another thread or process. In this case,
JUnit should ignore the NoSuchFileException, because there's nothing to
clean-up.
Add NoSuchFileException suppression to release notes.
@gilday gilday force-pushed the ignore-nosuchfileexceptin-in-directory-cleanup branch from a90e282 to ed397b4 Compare April 9, 2024 15:03
@marcphilipp
Copy link
Member

I considered what an automated test for this new behavior would look like. I could add a clear-box test that uses high-level concurrency objects to make sure that the test can delete a file between the time that the SimpleFileVisitor lists the temp directory and the time that the SimpleFileVisitor attempts to delete the file, but this would require adding new seams to TempDirectory that are inconsistent with the existing tests for that type. Would you accept this change without adding new test coverage?

Given that such a test would likely be flaky I don't think adding it would be justified. I'd be okay with making an exception in this case.

@marcphilipp marcphilipp changed the title Ignore NoSuchFileException in @TempDir cleanup Ignore NoSuchFileException in TempDir cleanup Apr 16, 2024
@marcphilipp marcphilipp merged commit 6dbc84e into junit-team:main Apr 16, 2024
13 checks passed
@marcphilipp
Copy link
Member

@gilday Thank you for your contribution! 👍

@sbrannen sbrannen added this to the 5.11 M1 milestone Apr 20, 2024
@sbrannen sbrannen changed the title Ignore NoSuchFileException in TempDir cleanup Ignore NoSuchFileException in @TempDir cleanup Apr 20, 2024
@marcphilipp marcphilipp modified the milestones: 5.11 M1, 5.10.3 Jun 17, 2024
marcphilipp pushed a commit that referenced this pull request Jun 17, 2024
While walking the temp directory, the file walker may fail to visit a
file that has been removed by another thread or process. In this case,
JUnit should ignore the NoSuchFileException, because there's nothing to
clean up.

(cherry picked from commit 6dbc84e)
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