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 all 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
89 changes: 63 additions & 26 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,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);
pmlopes marked this conversation as resolved.
Show resolved Hide resolved
}
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
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