Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 80 additions & 0 deletions src/org/labkey/test/TestFileUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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));
}
Comment on lines +473 to +478
Copy link
Copy Markdown
Member

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.


FileUtils.deleteDirectory(dir);
LOG.info(String.format("Deletion of directory %s was successful.", dir));
break;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Consider making this return. If you do that, you can eliminate the if (attempt == 10) statement below and move its body outside of the loop.

} catch (IOException ioException) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

File.walk().forEach() can throw UncheckedIOException. Probably good to catch that here too.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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
} catch (IOException ioException) {
} catch (AccessDeniedException adException) {
throw adException;
} catch (IOException ioException) {

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) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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.
e.g.

Suggested change
if (attempt == 10) {
if (attempt == MAX_ATTEMPTS) {


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).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Process started implementing Closeable in Java 26. We're using Java 25.

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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Consider try-with-resources for the stream

Suggested change
try {
String output = new String(p.getInputStream().readAllBytes(), StringUtilsLabKey.DEFAULT_CHARSET);
LOG.info("Running processes:\n" + output);
}
try (InputStream is = p.getInputStream()) {
String output = new String(is.readAllBytes(), StringUtilsLabKey.DEFAULT_CHARSET);
LOG.info("Running processes:\n" + output);
}

finally {
// Don't leak the process resource.
p.destroy();
}

} catch (IOException diagnosticException) {
LOG.warn("Failed to run lock diagnostic: " + diagnosticException.getMessage());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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
LOG.warn("Failed to run lock diagnostic: " + diagnosticException.getMessage());
LOG.warn("Failed to run lock diagnostic: " + diagnosticException.getMessage(), diagnosticException);

}
throw ioException;
}
sleep(10_000);
}
}

}

private static void checkFileLocation(File file)
{
try
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package org.labkey.test.tests.assay;

import org.apache.commons.io.FileUtils;
import org.junit.Test;
import org.junit.experimental.categories.Category;
import org.labkey.api.util.FileUtil;
Expand All @@ -15,8 +14,6 @@
import org.labkey.test.params.assay.GeneralAssayDesign;

import java.io.File;
import java.io.IOException;
import java.nio.file.AccessDeniedException;
import java.nio.file.Files;
import java.nio.file.Path;

Expand Down Expand Up @@ -48,29 +45,8 @@ public void testMissingParentDirectoryRegression() throws Exception
assayDesignerPage.addTransformScript(transformFile);
assayDesignerPage.clickSave();

// Now delete the parent dir to ensure we handle it reasonably. On Windows something locks the directory, maybe
// an external process. If that happens sleep for a second and try again.
for (int attempt = 1; attempt <= 10; attempt++) {
try
{
FileUtils.deleteDirectory(parentDir.toFile());
log(String.format("Deletion of directory %s was successful.", parentDir));
break;
} catch (AccessDeniedException deniedException) {
// Yes I know AccessDeniedException is a subset of an IOException, but I wanted to log explicitly a
// failure and retry because of an AccessDeniedException from some other IOException.
log(String.format("Access denied trying to delete directory %s. Error: %s. Waiting 10s and retrying. Attempt %d of 10.",
parentDir, deniedException.getMessage(), attempt));
if (attempt == 10) throw deniedException;
sleep(10_000);
}
catch (IOException ioException) {
log(String.format("IOException trying to delete directory %s. Error: %s. Waiting 10s and retrying. Attempt %d of 10.",
parentDir, ioException.getMessage(), attempt));
if (attempt == 10) throw ioException;
sleep(10_000);
}
}
// Now delete the parent dir to ensure we handle it reasonably
TestFileUtils.deleteDirWithRetry(parentDir.toFile());

// Attempt to import data and verify a reasonable error message is shown
String importData = """
Expand Down
Loading