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.

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

Avoid UUID.randomUUID() in file system related startup code #5450

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
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
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ static void fromJson(Iterable<java.util.Map.Entry<String, Object>> json, FileSys
obj.setFileCacheDir((String)member.getValue());
}
break;
case "exactFileCacheDir":
if (member.getValue() instanceof String) {
obj.setExactFileCacheDir((String)member.getValue());
}
break;
}
}
}
Expand All @@ -43,5 +48,8 @@ static void toJson(FileSystemOptions obj, java.util.Map<String, Object> json) {
if (obj.getFileCacheDir() != null) {
json.put("fileCacheDir", obj.getFileCacheDir());
}
if (obj.getExactFileCacheDir() != null) {
json.put("exactFileCacheDir", obj.getExactFileCacheDir());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,17 @@ public class FileSystemOptions {
*/
public static final String DEFAULT_FILE_CACHING_DIR = SysProps.FILE_CACHE_DIR.get();

/**
* The default exact file caching dir, which is {@code null} as {@link FileSystemOptions#setExactFileCacheDir}
* is meant to be used by advanced integrations that can guarantee on their own that the cache dir
* will be unique
*/
public static final String DEFAULT_EXACT_FILE_CACHING_DIR = null;

private boolean classPathResolvingEnabled = DEFAULT_CLASS_PATH_RESOLVING_ENABLED;
private boolean fileCachingEnabled = DEFAULT_FILE_CACHING_ENABLED;
private String fileCacheDir = DEFAULT_FILE_CACHING_DIR;
private String exactFileCacheDir = DEFAULT_EXACT_FILE_CACHING_DIR;

/**
* Default constructor
Expand Down Expand Up @@ -128,7 +136,7 @@ public FileSystemOptions setFileCachingEnabled(boolean fileCachingEnabled) {
}

/**
* @return the configured file cache dir
* @return the base name of the configured file cache dir. Vert.x will append a random value to this when determining the effective value
*/
public String getFileCacheDir() {
return this.fileCacheDir;
Expand All @@ -147,13 +155,34 @@ public FileSystemOptions setFileCacheDir(String fileCacheDir) {
return this;
}

/**
* @return the configured exact file cache dir to be used as is
*/
public String getExactFileCacheDir() {
return this.exactFileCacheDir;
}

/**
* When vert.x reads a file that is packaged with the application it gets
* extracted to this directory first and subsequent reads will use the extracted
* file to get better IO performance.
*
* @param exactFileCacheDir the value
* @return a reference to this, so the API can be used fluently
*/
public FileSystemOptions setExactFileCacheDir(String exactFileCacheDir) {
this.exactFileCacheDir = exactFileCacheDir;
return this;
}


@Override
public String toString() {
return "FileSystemOptions{" +
"classPathResolvingEnabled=" + classPathResolvingEnabled +
", fileCachingEnabled=" + fileCachingEnabled +
", fileCacheDir=" + fileCacheDir +
", exactFileCacheDir=" + exactFileCacheDir +
'}';
}
}
12 changes: 5 additions & 7 deletions vertx-core/src/main/java/io/vertx/core/file/impl/FileCache.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import java.io.InputStream;
import java.nio.file.FileAlreadyExistsException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.StandardCopyOption;
import java.nio.file.attribute.PosixFilePermission;
import java.nio.file.attribute.PosixFilePermissions;
Expand All @@ -27,8 +26,8 @@

public class FileCache {

static FileCache setupCache(String fileCacheDir) {
FileCache cache = new FileCache(setupCacheDir(fileCacheDir));
public static FileCache setupCache(String fileCacheDir, boolean isEffectiveValue) {
FileCache cache = new FileCache(setupCacheDir(fileCacheDir, isEffectiveValue));
// Add shutdown hook to delete on exit
cache.registerShutdownHook();
return cache;
Expand All @@ -37,16 +36,15 @@ static FileCache setupCache(String fileCacheDir) {
/**
* Prepares the cache directory to be used in the application.
*/
static File setupCacheDir(String fileCacheDir) {
static File setupCacheDir(String fileCacheDir, boolean isEffectiveValue) {
// 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();
File cacheDir = new File(cacheDirName);
File cacheDir = isEffectiveValue ? new File(fileCacheDir) : new File(fileCacheDir + "-" + UUID.randomUUID());
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this change, break the comment above? when isEffectiveValue is true, 2 vert.x instances will interfere with each other's cache. While this is probably ok for the same application, if 2 applications differ, then it could cause invalid states.

One example is (regardless if the 2 applications are the same or not) The moment the 1st terminates, it deletes the cache and would also mean it was deleted for the second, causing inconsistencies and errors.

// Create the cache directory
try {
if (Utils.isWindows()) {
Expand Down Expand Up @@ -220,7 +218,7 @@ private void fileNameCheck(File file) throws IOException {
}
}

private File getCacheDir() {
public File getCacheDir() {
File currentCacheDir = cacheDir;
if (currentCacheDir == null) {
throw new IllegalStateException("cacheDir has been removed. FileResolver is closing?");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,12 @@ public FileResolverImpl(FileSystemOptions fileSystemOptions) {
enableCPResolving = fileSystemOptions.isClassPathResolvingEnabled();

if (enableCPResolving) {
cache = FileCache.setupCache(fileSystemOptions.getFileCacheDir());
String exactFileCacheDir = fileSystemOptions.getExactFileCacheDir();
if (exactFileCacheDir != null) {
cache = FileCache.setupCache(exactFileCacheDir, true);
} else {
cache = FileCache.setupCache(fileSystemOptions.getFileCacheDir(), false);
}
} else {
cache = null;
}
Expand Down
18 changes: 18 additions & 0 deletions vertx-core/src/test/java/io/vertx/tests/file/FileCacheTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@
package io.vertx.tests.file;

import io.vertx.core.VertxException;
import io.vertx.core.file.FileSystemOptions;
import io.vertx.core.file.impl.FileCache;
import io.vertx.core.file.impl.FileResolverImpl;
import io.vertx.test.core.VertxTestBase;
import org.junit.Test;

Expand Down Expand Up @@ -40,4 +42,20 @@ public void testMutateCacheContentOnly() throws IOException {
assertEquals("protected", new String(Files.readAllBytes(other.toPath())));
}
}

@Test
public void testGetTheExactCacheDirWithoutHacks() throws IOException {
String cacheBaseDir;
try {
cacheBaseDir = new File(System.getProperty("java.io.tmpdir", ".") + File.separator + "vertx-cache").getCanonicalPath();
} catch (IOException e) {
throw new IllegalStateException("Cannot resolve the canonical path to the cache dir", e);
}

String cacheDir = FileCache.setupCache(cacheBaseDir + "-exact", true).getCacheDir().getCanonicalPath();
assertTrue(cacheDir.startsWith(cacheBaseDir + "-"));
// strip the remaining
String remaining = cacheDir.substring(cacheBaseDir.length() + 1);
assertEquals(remaining, "exact");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
package io.vertx.tests.file.cachedir;

import io.vertx.core.VertxOptions;
import io.vertx.core.file.FileSystemOptions;
import io.vertx.core.internal.VertxInternal;
import io.vertx.core.spi.file.FileResolver;
import io.vertx.test.core.VertxTestBase;
import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import org.junit.Assert;
import org.junit.Test;

public class ExactDirDoesNotExistTest extends VertxTestBase {

private final Path cacheBaseDir;

public ExactDirDoesNotExistTest() throws IOException {
cacheBaseDir = Files.createTempDirectory("cache-does-not-exist");
Files.deleteIfExists(cacheBaseDir);
Assert.assertFalse(Files.exists(cacheBaseDir));
}

@Override
protected VertxOptions getOptions() {
return new VertxOptions(super.getOptions())
.setFileSystemOptions(new FileSystemOptions().setFileCachingEnabled(true).setExactFileCacheDir(cacheBaseDir.toAbsolutePath().toString()));
}

@Test
public void test() throws IOException {
try (FileResolver fileResolver = ((VertxInternal) vertx).fileResolver()) {
File file = fileResolver.resolveFile("conf.json");
Assert.assertEquals(cacheBaseDir.resolve("conf.json").toFile(), file);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package io.vertx.tests.file.cachedir;

import io.vertx.core.Vertx;
import io.vertx.core.VertxOptions;
import io.vertx.core.file.FileSystemOptions;
import io.vertx.core.internal.VertxInternal;
import io.vertx.core.spi.file.FileResolver;
import io.vertx.test.core.VertxTestBase;
import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import org.junit.Assert;
import org.junit.Test;

public class ExactDirExistsButIsFileTest {

private final Path cacheBaseDir;

public ExactDirExistsButIsFileTest() throws IOException {
cacheBaseDir = Files.createTempDirectory("cache-exists-but-is-file");
Files.deleteIfExists(cacheBaseDir);
Files.createFile(cacheBaseDir);
Assert.assertTrue(Files.exists(cacheBaseDir));
Assert.assertFalse(Files.isDirectory(cacheBaseDir));
}

@Test
public void test() {
Assert.assertThrows(IllegalStateException.class, () -> {
Vertx.builder().with(new VertxOptions().setFileSystemOptions(new FileSystemOptions().setFileCachingEnabled(true).setExactFileCacheDir(cacheBaseDir.toAbsolutePath().toString()))).build();
});
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package io.vertx.tests.file.cachedir;

import io.vertx.core.VertxOptions;
import io.vertx.core.file.FileSystemOptions;
import io.vertx.core.internal.VertxInternal;
import io.vertx.core.spi.file.FileResolver;
import io.vertx.test.core.VertxTestBase;
import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import org.junit.Assert;
import org.junit.Test;

public class ExactDirExistsTest extends VertxTestBase {

private final Path cacheBaseDir;

public ExactDirExistsTest() throws IOException {
cacheBaseDir = Paths.get(System.getProperty("java.io.tmpdir", "."), "cache-exists");
Files.deleteIfExists(cacheBaseDir);
Files.createDirectories(cacheBaseDir);
Assert.assertTrue(Files.exists(cacheBaseDir));
Assert.assertTrue(Files.isDirectory(cacheBaseDir));
}

@Override
protected VertxOptions getOptions() {
return new VertxOptions(super.getOptions())
.setFileSystemOptions(new FileSystemOptions().setFileCachingEnabled(true).setExactFileCacheDir(cacheBaseDir.toAbsolutePath().toString()));
}

@Test
public void test() throws IOException {
try (FileResolver fileResolver = ((VertxInternal) vertx).fileResolver()) {
File file = fileResolver.resolveFile("conf.json");
Assert.assertEquals(cacheBaseDir.resolve("conf.json").toFile(), file);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package io.vertx.tests.file.cachedir;

import io.vertx.core.Vertx;
import io.vertx.core.VertxOptions;
import io.vertx.core.file.FileSystemOptions;
import io.vertx.core.internal.VertxInternal;
import io.vertx.core.spi.file.FileResolver;
import io.vertx.test.core.VertxTestBase;
import java.io.File;
import java.io.IOException;
import java.nio.file.FileSystemException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import org.junit.Assert;
import org.junit.Test;

public class ExactDirParentIsAFileTest {

private final Path cacheBaseDir;

public ExactDirParentIsAFileTest() throws IOException {
Path cacheBaseDirParent = Paths.get(System.getProperty("java.io.tmpdir", "."), "cache-parent");
Files.deleteIfExists(cacheBaseDirParent);
Files.createFile(cacheBaseDirParent);
Assert.assertTrue(Files.exists(cacheBaseDirParent));
Assert.assertFalse(Files.isDirectory(cacheBaseDirParent));
this.cacheBaseDir = cacheBaseDirParent.resolve("actual-cache");
Assert.assertFalse(Files.exists(cacheBaseDir));
}

@Test
public void test() {
Assert.assertThrows(IllegalStateException.class, () -> {
Vertx.builder().with(new VertxOptions().setFileSystemOptions(new FileSystemOptions().setFileCachingEnabled(true).setExactFileCacheDir(cacheBaseDir.toAbsolutePath().toString()))).build();
});
}
}
Loading