-
Notifications
You must be signed in to change notification settings - Fork 9
Add TestFileUtils.deleteDirWithRetry method. #2946
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
base: release26.3-SNAPSHOT
Are you sure you want to change the base?
Changes from all commits
4760e3e
9b27d34
957ae62
a6716e0
195b112
8806108
7850ea4
8bbb7ee
2c7ee8f
d9d2906
5019730
b124b46
1dcf68d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -21,6 +21,7 @@ | |||||||||||||||||
| import org.apache.commons.io.FileUtils; | ||||||||||||||||||
| import org.apache.commons.io.IOUtils; | ||||||||||||||||||
| import org.apache.commons.lang3.StringUtils; | ||||||||||||||||||
| import org.apache.commons.lang3.SystemUtils; | ||||||||||||||||||
| import org.apache.logging.log4j.LogManager; | ||||||||||||||||||
| import org.apache.logging.log4j.Logger; | ||||||||||||||||||
| import org.apache.pdfbox.Loader; | ||||||||||||||||||
|
|
@@ -40,6 +41,7 @@ | |||||||||||||||||
| import org.jetbrains.annotations.NotNull; | ||||||||||||||||||
| import org.labkey.api.util.FileUtil; | ||||||||||||||||||
| import org.jetbrains.annotations.Nullable; | ||||||||||||||||||
| import org.labkey.api.util.StringUtilsLabKey; | ||||||||||||||||||
| import org.openqa.selenium.NotFoundException; | ||||||||||||||||||
|
|
||||||||||||||||||
| import java.io.BufferedInputStream; | ||||||||||||||||||
|
|
@@ -57,6 +59,7 @@ | |||||||||||||||||
| import java.io.Writer; | ||||||||||||||||||
| import java.nio.charset.Charset; | ||||||||||||||||||
| import java.nio.charset.StandardCharsets; | ||||||||||||||||||
| import java.nio.file.AccessDeniedException; | ||||||||||||||||||
| import java.nio.file.FileVisitResult; | ||||||||||||||||||
| import java.nio.file.Files; | ||||||||||||||||||
| import java.nio.file.Path; | ||||||||||||||||||
|
|
@@ -71,10 +74,13 @@ | |||||||||||||||||
| import java.util.Set; | ||||||||||||||||||
| import java.util.TreeSet; | ||||||||||||||||||
| import java.util.stream.Collectors; | ||||||||||||||||||
| import java.util.stream.Stream; | ||||||||||||||||||
| import java.util.zip.GZIPInputStream; | ||||||||||||||||||
| import java.util.zip.ZipEntry; | ||||||||||||||||||
| import java.util.zip.ZipInputStream; | ||||||||||||||||||
|
|
||||||||||||||||||
| import static org.labkey.api.util.DebugInfoDumper.dumpHeap; | ||||||||||||||||||
| import static org.labkey.test.WebDriverWrapper.sleep; | ||||||||||||||||||
| import static org.labkey.test.util.TestDataGenerator.CHARSET_STRING; | ||||||||||||||||||
| import static org.labkey.test.util.TestDataGenerator.randomInt; | ||||||||||||||||||
| import static org.labkey.test.util.TestDataGenerator.randomName; | ||||||||||||||||||
|
|
@@ -445,6 +451,80 @@ public static void deleteDir(File dir) | |||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * Deletes a directory and all its contents, retrying up to 10 times with a 10-second delay between attempts. | ||||||||||||||||||
| * <p> | ||||||||||||||||||
| * Before each attempt, the directory and all its children are marked writable to handle read-only files or | ||||||||||||||||||
| * directories. This is primarily intended to work around Windows file-locking issues where an external process | ||||||||||||||||||
| * may hold a lock on the directory or its contents. | ||||||||||||||||||
| * <p> | ||||||||||||||||||
| * On the final failed attempt, a heap dump is captured for diagnostics if running on TeamCity. The list of running | ||||||||||||||||||
| * processes is also logged to help identify what may be holding the lock. | ||||||||||||||||||
| * | ||||||||||||||||||
| * @param dir the directory to delete | ||||||||||||||||||
| * @throws Exception if an unexpected error occurs | ||||||||||||||||||
| */ | ||||||||||||||||||
| public static void deleteDirWithRetry(File dir) throws Exception | ||||||||||||||||||
| { | ||||||||||||||||||
| // Sometimes on Windows the directory could be locked, maybe by an external process, or the child directory is | ||||||||||||||||||
| // readonly. Use a retry mechanism to set the writeable flag and then try to delete the parent directory. | ||||||||||||||||||
| for (int attempt = 1; attempt <= 10; attempt++) { | ||||||||||||||||||
| try { | ||||||||||||||||||
| dir.setWritable(true, false); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Wrap in a try to close the stream. | ||||||||||||||||||
| try (Stream<Path> files = Files.walk(dir.toPath())) { | ||||||||||||||||||
| files.forEach(p -> p.toFile().setWritable(true, false)); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| FileUtils.deleteDirectory(dir); | ||||||||||||||||||
| LOG.info(String.format("Deletion of directory %s was successful.", dir)); | ||||||||||||||||||
| break; | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider making this |
||||||||||||||||||
| } catch (IOException ioException) { | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should just rethrow permissions exceptions. We haven't actually run into errors with those.
Suggested change
|
||||||||||||||||||
| LOG.warn(String.format("IOException trying to delete directory %s. Error: %s. Waiting 10s and retrying. Attempt %d of 10.", | ||||||||||||||||||
| dir, ioException.getMessage(), attempt)); | ||||||||||||||||||
| if (attempt == 10) { | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Put the max attempt count in a constant to ensure that this stays in sync with the for loop.
Suggested change
|
||||||||||||||||||
|
|
||||||||||||||||||
| if (TestProperties.isTestRunningOnTeamCity()) { | ||||||||||||||||||
| LOG.info("Dump the heap."); | ||||||||||||||||||
| dumpHeap(); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| ProcessBuilder pb; | ||||||||||||||||||
| if (SystemUtils.IS_OS_WINDOWS) { | ||||||||||||||||||
| pb = new ProcessBuilder("tasklist"); | ||||||||||||||||||
| } | ||||||||||||||||||
| else { | ||||||||||||||||||
| pb = new ProcessBuilder("ps", "-ef"); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| try { | ||||||||||||||||||
| LOG.info("Lock diagnostic..."); | ||||||||||||||||||
| pb.redirectErrorStream(true); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Tried to use "try (Process p = pb.start()) {" without the finally block but our build | ||||||||||||||||||
| // system didn't like that and complained that Process doesn't implement closeable (it does). | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||||||
| Process p = pb.start(); | ||||||||||||||||||
| try { | ||||||||||||||||||
| String output = new String(p.getInputStream().readAllBytes(), StringUtilsLabKey.DEFAULT_CHARSET); | ||||||||||||||||||
| LOG.info("Running processes:\n" + output); | ||||||||||||||||||
| } | ||||||||||||||||||
|
Comment on lines
+508
to
+511
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider try-with-resources for the stream
Suggested change
|
||||||||||||||||||
| finally { | ||||||||||||||||||
| // Don't leak the process resource. | ||||||||||||||||||
| p.destroy(); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| } catch (IOException diagnosticException) { | ||||||||||||||||||
| LOG.warn("Failed to run lock diagnostic: " + diagnosticException.getMessage()); | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Loggers can include the entire stack-trace and I think it would be useful to do so.
Suggested change
|
||||||||||||||||||
| } | ||||||||||||||||||
| throw ioException; | ||||||||||||||||||
| } | ||||||||||||||||||
| sleep(10_000); | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| private static void checkFileLocation(File file) | ||||||||||||||||||
| { | ||||||||||||||||||
| try | ||||||||||||||||||
|
|
||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the tests should be in the business of changing file permissions (unless that's something we are explicitly testing at some point).
If the server (or other process) is leaving a file in an unwritable state, that is something we probably wouldn't want to ignore.