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.

Sign up for GitHub

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

Jump to bottom

Fixes Resolver delete race against file resolve #3581

Merged
merged 5 commits into from
Oct 2, 2020
Merged

Fixes Resolver delete race against file resolve #3581

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
40fd0d0
Fixes Resolver delete race against file resolve
pmlopes Sep 23, 2020
6e1a9d8
Added a comment
pmlopes Sep 23, 2020
05e70fe
Refactor based on review + more checks
pmlopes Sep 25, 2020
6196934
Updates based on review feedback
pmlopes Sep 28, 2020
0a888a1
less locking
pmlopes Oct 2, 2020
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
46 changes: 29 additions & 17 deletions src/main/java/io/vertx/core/file/impl/FileResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import java.net.URL;
import java.nio.file.FileAlreadyExistsException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.StandardCopyOption;
import java.util.Enumeration;
import java.util.UUID;
Expand Down Expand Up @@ -88,11 +87,10 @@ public class FileResolver {
private static final Pattern JAR_URL_SEP_PATTERN = Pattern.compile(JAR_URL_SEP);

private final File cwd;
private File cacheDir;
private final File cacheDir;
private Thread shutdownHook;
private final boolean enableCaching;
private final boolean enableCpResolving;
private final String fileCacheDir;

public FileResolver() {
this(new FileSystemOptions());
Expand All @@ -101,17 +99,14 @@ public FileResolver() {
public FileResolver(FileSystemOptions fileSystemOptions) {
this.enableCaching = fileSystemOptions.isFileCachingEnabled();
this.enableCpResolving = fileSystemOptions.isClassPathResolvingEnabled();
this.fileCacheDir = fileSystemOptions.getFileCacheDir();

String cwdOverride = System.getProperty("vertx.cwd");
if (cwdOverride != null) {
cwd = new File(cwdOverride).getAbsoluteFile();
} else {
cwd = null;
}
if (this.enableCpResolving) {
setupCacheDir(UUID.randomUUID().toString());
}
cacheDir = setupCacheDir(fileSystemOptions.getFileCacheDir());
}

/**
Expand Down Expand Up @@ -375,9 +370,24 @@ private ClassLoader getClassLoader() {
return cl;
}

private synchronized void setupCacheDir(String id) {
String cacheDirName = fileCacheDir + "/file-cache-" + id;
cacheDir = new File(cacheDirName);
/**
* Will prepare the cache directory to be used in the application or return null if classpath resolving is disabled.
*/
private synchronized File setupCacheDir(String fileCacheDir) {
pmlopes marked this conversation as resolved.
Show resolved Hide resolved
if (!this.enableCpResolving) {
return null;
}

// ensure that the argument doesn't end with separator
if (fileCacheDir.endsWith(File.separator)) {
fileCacheDir = fileCacheDir.substring(0, fileCacheDir.length() - File.separator.length());
}

// the cachedir will be prefixed a unique id to avoid eavesdroping from other processes
// also this ensures that if process A deletes cacheDir, it won't affect process B
String cacheDirName = fileCacheDir + "-" + UUID.randomUUID().toString();
File cacheDir = new File(cacheDirName);

if (!cacheDir.mkdirs()) {
throw new IllegalStateException("Failed to create cache dir: " + cacheDirName);
}
Expand All @@ -398,18 +408,20 @@ private synchronized void setupCacheDir(String id) {
}
});
Runtime.getRuntime().addShutdownHook(shutdownHook);
return cacheDir;
}

private void deleteCacheDir() throws IOException {
Path path;
if (cacheDir == null || !cacheDir.exists()) {
pmlopes marked this conversation as resolved.
Show resolved Hide resolved
pmlopes marked this conversation as resolved.
Show resolved Hide resolved
return;
}
synchronized (this) {
if (cacheDir == null || !cacheDir.exists()) {
return;
// if a previous run was already deleting
// after entering the monitor we can quickly abort of the
// directory doesn't exist anymore
if (cacheDir.exists()) {
FileSystemImpl.delete(cacheDir.toPath(), true);
}
path = cacheDir.toPath();
cacheDir = null;
}
FileSystemImpl.delete(path, true);
}
}

14 changes: 7 additions & 7 deletions src/test/java/io/vertx/core/file/FileResolverTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public void testResolveFileFromClasspath() throws Exception {
for (int i = 0; i < 2; i++) {
File file = resolver.resolveFile("afile.html");
assertTrue(file.exists());
assertTrue(file.getPath().startsWith(cacheBaseDir + File.separator + "file-cache-"));
assertTrue(file.getPath().startsWith(cacheBaseDir + "-"));
assertFalse(file.isDirectory());
assertEquals("<html><body>afile</body></html>", readFile(file));
}
Expand All @@ -110,7 +110,7 @@ public void testResolveFileFromClasspathDisableCaching() throws Exception {
for (int i = 0; i < 2; i++) {
File file = vertx.resolveFile("afile.html");
assertTrue(file.exists());
assertTrue(file.getPath().startsWith(cacheBaseDir + File.separator + "file-cache-"));
assertTrue(file.getPath().startsWith(cacheBaseDir + "-"));
assertFalse(file.isDirectory());
assertEquals("<html><body>afile</body></html>", readFile(file));
}
Expand All @@ -124,7 +124,7 @@ public void testResolveFileWithSpacesFromClasspath() throws Exception {
for (int i = 0; i < 2; i++) {
File file = resolver.resolveFile("afile with spaces.html");
assertTrue(file.exists());
assertTrue(file.getPath().startsWith(cacheBaseDir + File.separator + "file-cache-"));
assertTrue(file.getPath().startsWith(cacheBaseDir + "-"));
assertFalse(file.isDirectory());
assertEquals("<html><body>afile with spaces</body></html>", readFile(file));
}
Expand All @@ -135,7 +135,7 @@ public void testResolveDirectoryFromClasspath() throws Exception {
for (int i = 0; i < 2; i++) {
File file = resolver.resolveFile(webRoot);
assertTrue(file.exists());
assertTrue(file.getPath().startsWith(cacheBaseDir + File.separator + "file-cache-"));
assertTrue(file.getPath().startsWith(cacheBaseDir + "-"));
assertTrue(file.isDirectory());
}
}
Expand All @@ -145,7 +145,7 @@ public void testResolveFileInDirectoryFromClasspath() throws Exception {
for (int i = 0; i < 2; i++) {
File file = resolver.resolveFile(webRoot + "/somefile.html");
assertTrue(file.exists());
assertTrue(file.getPath().startsWith(cacheBaseDir + File.separator + "file-cache-"));
assertTrue(file.getPath().startsWith(cacheBaseDir + "-"));
assertFalse(file.isDirectory());
assertEquals("<html><body>blah</body></html>", readFile(file));
}
Expand All @@ -156,7 +156,7 @@ public void testResolveSubDirectoryFromClasspath() throws Exception {
for (int i = 0; i < 2; i++) {
File file = resolver.resolveFile(webRoot + "/subdir");
assertTrue(file.exists());
assertTrue(file.getPath().startsWith(cacheBaseDir + File.separator + "file-cache-"));
assertTrue(file.getPath().startsWith(cacheBaseDir + "-"));
assertTrue(file.isDirectory());
}
}
Expand All @@ -166,7 +166,7 @@ public void testResolveFileInSubDirectoryFromClasspath() throws Exception {
for (int i = 0; i < 2; i++) {
File file = resolver.resolveFile(webRoot + "/subdir/subfile.html");
assertTrue(file.exists());
assertTrue(file.getPath().startsWith(cacheBaseDir + File.separator + "file-cache-"));
assertTrue(file.getPath().startsWith(cacheBaseDir + "-"));
assertFalse(file.isDirectory());
assertEquals("<html><body>subfile</body></html>", readFile(file));
}
Expand Down