Skip to content

Commit

Permalink
Merge pull request #3581 from eclipse-vertx/issues/race-resolver-close
Browse files Browse the repository at this point in the history 
Fixes Resolver delete race against file resolve
  • Loading branch information
@pmlopes
pmlopes authored Oct 2, 2020
2 parents 287cd41 + 0a888a1 commit 4744506
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 33 deletions.
89 changes: 63 additions & 26 deletions src/main/java/io/vertx/core/file/impl/FileResolver.java
View file
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,11 @@ public class FileResolver {
private static final Pattern JAR_URL_SEP_PATTERN = Pattern.compile(JAR_URL_SEP);

private final File cwd;
private final boolean enableCpResolving;
private final boolean enableCaching;
// mutable state
private 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,30 +100,32 @@ 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());
}

/**
* Close this file resolver, this is a blocking operation.
*/
public void close() throws IOException {
final Thread hook;
synchronized (this) {
if (shutdownHook != null) {
// May throw IllegalStateException if called from other shutdown hook so ignore that
try {
Runtime.getRuntime().removeShutdownHook(shutdownHook);
} catch (IllegalStateException ignore) {
}
hook = shutdownHook;
// disable the shutdown hook thread
shutdownHook = null;
}
if (hook != null) {
// May throw IllegalStateException if called from other shutdown hook so ignore that
try {
Runtime.getRuntime().removeShutdownHook(hook);
} catch (IllegalStateException ignore) {
// ignore
}
}
deleteCacheDir();
Expand All @@ -139,6 +140,10 @@ public File resolveFile(String fileName) {
if (!this.enableCpResolving) {
return file;
}
// if cacheDir is null, the delete cache dir was already called.
if (cacheDir == null) {
throw new IllegalStateException("cacheDir is null");
}
// We need to synchronized here to avoid 2 different threads to copy the file to the cache directory and so
// corrupting the content.
synchronized (this) {
Expand Down Expand Up @@ -236,10 +241,15 @@ private synchronized File unpackFromFileURL(URL url, String fileName, ClassLoade
} else {
cacheFile.mkdirs();
String[] listing = resource.list();
for (String file: listing) {
String subResource = fileName + "/" + file;
URL url2 = getValidClassLoaderResource(cl, subResource);
unpackFromFileURL(url2, subResource, cl);
if (listing != null) {
for (String file: listing) {
String subResource = fileName + "/" + file;
URL url2 = getValidClassLoaderResource(cl, subResource);
if (url2 == null) {
throw new VertxException("Invalid resource: " + subResource);
}
unpackFromFileURL(url2, subResource, cl);
}
}
}
return cacheFile;
Expand Down Expand Up @@ -375,14 +385,36 @@ 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 File setupCacheDir(String fileCacheDir) {
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 suffixed a unique id to avoid eavesdropping from other processes/users
// 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);
}
// Add shutdown hook to delete on exit
shutdownHook = new Thread(() -> {
synchronized (this) {
// no-op if cache dir has been set to null
if (this.cacheDir == null) {
return;
}
}

final Thread deleteCacheDirThread = new Thread(() -> {
try {
deleteCacheDir();
Expand All @@ -398,18 +430,23 @@ private synchronized void setupCacheDir(String id) {
}
});
Runtime.getRuntime().addShutdownHook(shutdownHook);
return cacheDir;
}

private void deleteCacheDir() throws IOException {
Path path;
final File dir;
synchronized (this) {
if (cacheDir == null || !cacheDir.exists()) {
if (cacheDir == null) {
return;
}
path = cacheDir.toPath();
// save the state before we force a flip
dir = cacheDir;
// disable the cache dir
cacheDir = null;
}
FileSystemImpl.delete(path, true);
// threads will only enter here once, as the resolving flag is flipped above
if (dir.exists()) {
FileSystemImpl.delete(dir.toPath(), true);
}
}
}

14 changes: 7 additions & 7 deletions src/test/java/io/vertx/core/file/FileResolverTestBase.java
View file
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

0 comments on commit 4744506

Please sign in to comment.